Some registers may be modified by parallel execution contexts and require protections to prevent corruption.
A review of the driver revealed the need for these additional protections.
Doug Berger (3): net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() net: bcmgenet: synchronize UMAC_CMD access
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 14 +++++++++++++- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 2 ++ drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 6 ++++++ drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 ++++ 4 files changed, 25 insertions(+), 1 deletion(-)
[ Upstream commit d85cf67a339685beae1d0aee27b7f61da95455be ]
The EXT_RGMII_OOB_CTRL register can be written from different contexts. It is predominantly written from the adjust_link handler which is synchronized by the phydev->lock, but can also be written from a different context when configuring the mii in bcmgenet_mii_config().
The chances of contention are quite low, but it is conceivable that adjust_link could occur during resume when WoL is enabled so use the phydev->lock synchronizer in bcmgenet_mii_config() to be sure.
Fixes: afe3f907d20f ("net: bcmgenet: power on MII block for all MII modes") Cc: stable@vger.kernel.org Signed-off-by: Doug Berger opendmb@gmail.com Acked-by: Florian Fainelli florian.fainelli@broadcom.com Signed-off-by: David S. Miller davem@davemloft.net --- drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index 56fad34461f7..d0b59d1f6c73 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -269,6 +269,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) * block for the interface to work */ if (priv->ext_phy) { + mutex_lock(&phydev->lock); reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL); reg |= id_mode_dis; if (GENET_IS_V1(priv) || GENET_IS_V2(priv) || GENET_IS_V3(priv)) @@ -276,6 +277,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) else reg |= RGMII_MODE_EN; bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL); + mutex_unlock(&phydev->lock); }
if (init)
[ Upstream commit 2dbe5f19368caae63b1f59f5bc2af78c7d522b3a ]
The ndo_set_rx_mode function is synchronized with the netif_addr_lock spinlock and BHs disabled. Since this function is also invoked directly from the driver the same synchronization should be applied.
Fixes: 72f96347628e ("net: bcmgenet: set Rx mode before starting netif") Cc: stable@vger.kernel.org Signed-off-by: Doug Berger opendmb@gmail.com Acked-by: Florian Fainelli florian.fainelli@broadcom.com Signed-off-by: David S. Miller davem@davemloft.net --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index bf52bd643846..e34df8da65e7 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -2874,7 +2874,9 @@ static void bcmgenet_netif_start(struct net_device *dev) struct bcmgenet_priv *priv = netdev_priv(dev);
/* Start the network engine */ + netif_addr_lock_bh(dev); bcmgenet_set_rx_mode(dev); + netif_addr_unlock_bh(dev); bcmgenet_enable_rx_napi(priv);
umac_enable_set(priv, CMD_TX_EN | CMD_RX_EN, true);
[ Upstream commit 0d5e2a82232605b337972fb2c7d0cbc46898aca1 ]
The UMAC_CMD register is written from different execution contexts and has insufficient synchronization protections to prevent possible corruption. Of particular concern are the acceses from the phy_device delayed work context used by the adjust_link call and the BH context that may be used by the ndo_set_rx_mode call.
A spinlock is added to the driver to protect contended register accesses (i.e. reg_lock) and it is used to synchronize accesses to UMAC_CMD.
Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file") Cc: stable@vger.kernel.org Signed-off-by: Doug Berger opendmb@gmail.com Acked-by: Florian Fainelli florian.fainelli@broadcom.com Signed-off-by: David S. Miller davem@davemloft.net --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 12 +++++++++++- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 2 ++ drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 6 ++++++ drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 ++ 4 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index e34df8da65e7..da9df1d3662b 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1990,14 +1990,18 @@ static void umac_enable_set(struct bcmgenet_priv *priv, u32 mask, bool enable) { u32 reg;
+ spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); - if (reg & CMD_SW_RESET) + if (reg & CMD_SW_RESET) { + spin_unlock_bh(&priv->reg_lock); return; + } if (enable) reg |= mask; else reg &= ~mask; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock);
/* UniMAC stops on a packet boundary, wait for a full-size packet * to be processed @@ -2013,8 +2017,10 @@ static void reset_umac(struct bcmgenet_priv *priv) udelay(10);
/* issue soft reset and disable MAC while updating its registers */ + spin_lock_bh(&priv->reg_lock); bcmgenet_umac_writel(priv, CMD_SW_RESET, UMAC_CMD); udelay(2); + spin_unlock_bh(&priv->reg_lock); }
static void bcmgenet_intr_disable(struct bcmgenet_priv *priv) @@ -3140,16 +3146,19 @@ static void bcmgenet_set_rx_mode(struct net_device *dev) * 3. The number of filters needed exceeds the number filters * supported by the hardware. */ + spin_lock(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); if ((dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) || (nfilter > MAX_MDF_FILTER)) { reg |= CMD_PROMISC; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock(&priv->reg_lock); bcmgenet_umac_writel(priv, 0, UMAC_MDF_CTRL); return; } else { reg &= ~CMD_PROMISC; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock(&priv->reg_lock); }
/* update MDF filter */ @@ -3507,6 +3516,7 @@ static int bcmgenet_probe(struct platform_device *pdev) goto err; }
+ spin_lock_init(&priv->reg_lock); spin_lock_init(&priv->lock);
SET_NETDEV_DEV(dev, &pdev->dev); diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h index 29bf256d13f6..9efc503a9c8b 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h @@ -608,6 +608,8 @@ struct bcmgenet_rx_ring { /* device context */ struct bcmgenet_priv { void __iomem *base; + /* reg_lock: lock to serialize access to shared registers */ + spinlock_t reg_lock; enum bcmgenet_version version; struct net_device *dev;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c index 8ebca6bf300e..973275d116b6 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c @@ -133,6 +133,7 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, }
/* Can't suspend with WoL if MAC is still in reset */ + spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); if (reg & CMD_SW_RESET) reg &= ~CMD_SW_RESET; @@ -140,6 +141,7 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, /* disable RX */ reg &= ~CMD_RX_EN; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock); mdelay(10);
reg = bcmgenet_umac_readl(priv, UMAC_MPD_CTRL); @@ -163,6 +165,7 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, retries);
/* Enable CRC forward */ + spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); priv->crc_fwd_en = 1; reg |= CMD_CRC_FWD; @@ -170,6 +173,7 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, /* Receiver must be enabled for WOL MP detection */ reg |= CMD_RX_EN; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock);
return 0; } @@ -191,8 +195,10 @@ void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv, bcmgenet_umac_writel(priv, reg, UMAC_MPD_CTRL);
/* Disable CRC Forward */ + spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); reg &= ~CMD_CRC_FWD; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock); priv->crc_fwd_en = 0; } diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index d0b59d1f6c73..bd532e5b9f73 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -91,6 +91,7 @@ void bcmgenet_mii_setup(struct net_device *dev) reg |= RGMII_LINK; bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL);
+ spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); reg &= ~((CMD_SPEED_MASK << CMD_SPEED_SHIFT) | CMD_HD_EN | @@ -103,6 +104,7 @@ void bcmgenet_mii_setup(struct net_device *dev) reg |= CMD_TX_EN | CMD_RX_EN; } bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock);
priv->eee.eee_active = phy_init_eee(phydev, 0) >= 0; bcmgenet_eee_enable_set(dev,
On Thu, May 16, 2024 at 04:01:48PM -0700, Doug Berger wrote:
Some registers may be modified by parallel execution contexts and require protections to prevent corruption.
A review of the driver revealed the need for these additional protections.
Doug Berger (3): net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() net: bcmgenet: synchronize UMAC_CMD access
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 14 +++++++++++++- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 2 ++ drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 6 ++++++ drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 ++++ 4 files changed, 25 insertions(+), 1 deletion(-)
-- These commits are dependent on the previously submitted: [PATCH stable 5.4 0/2] net: bcmgenet: revisit MAC reset
2.34.1
All now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org