[PATCH net 0/8] net: bcmgenet: Collection of bug fixes

[PATCH net 0/8] net: bcmgenet: Collection of bug fixes

[Florian Fainelli]

This patch series contains a collection of bug fixes found during a
LLM-assisted coding session.

Florian Fainelli (8):
  net: bcmgenet: fix 64-bit RTNL stats reading in ethtool on 32-bit
    systems
  net: bcmgenet: initialize u64 stats seq counter for all queues
  net: bcmgenet: do not skip WoL power up on GENET V1
  net: bcmgenet: clean up RX NAPI on bcmgenet_init_rx_queues failure
  net: bcmgenet: acquire ring lock with BH disabled in
    bcmgenet_dump_tx_queue
  net: bcmgenet: validate Ethernet address in bcmgenet_set_mac_addr
  net: bcmgenet: zero-initialize Transmit Status Block in
    bcmgenet_add_tsb
  net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT

 .../net/ethernet/broadcom/genet/bcmgenet.c    | 34 ++++++++++++-------
 1 file changed, 21 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH net 1/8] net: bcmgenet: fix 64-bit RTNL stats reading in ethtool on 32-bit systems

[Florian Fainelli]

When bcmgenet was converted to 64-bit statistics, STAT_RTNL members were
switched to point into struct rtnl_link_stats64, whose fields are 64-bit
(__u64) regardless of architecture.

However, bcmgenet_get_ethtool_stats() retained a legacy check:
  if (sizeof(unsigned long) != sizeof(u32) &&
      s->stat_sizeof == sizeof(unsigned long))

On 32-bit systems, sizeof(unsigned long) == sizeof(u32), causing this
condition to evaluate to false. As a result, 64-bit RTNL stats fields were
read via *(u32 *)p. On 32-bit Big-Endian systems (such as MIPS BE), this
reads the high 32 bits and returns 0 until the counter exceeds 4GB; on
32-bit Little-Endian systems (such as 32-bit ARM), the value is truncated
to 32 bits.

Fix this by checking if s->stat_sizeof == sizeof(u64) so 64-bit fields are
always read as 64-bit values.

Fixes: 59aa6e3072aa ("net: bcmgenet: switch to use 64bit statistics")
Assisted-by: LLM
Co-authored-by: Cursor <cursoragent@cursor.com>
Change-Id: I3e9f2f8d4fd136392148d6c0cd4eb40f7ccbe4ef
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index b916080f4ff1..7b089de9484e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1346,9 +1346,8 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
 				p = (char *)&stats64;
 
 			p += s->stat_offset;
-			if (sizeof(unsigned long) != sizeof(u32) &&
-				s->stat_sizeof == sizeof(unsigned long))
-				data[i] = *(unsigned long *)p;
+			if (s->stat_sizeof == sizeof(u64))
+				data[i] = *(u64 *)p;
 			else
 				data[i] = *(u32 *)p;
 		}
-- 
2.34.1

[PATCH net 2/8] net: bcmgenet: initialize u64 stats seq counter for all queues

[Florian Fainelli]

bcmgenet_gstrings_stats statically defines ethtool statistics for queues
0 through GENET_MAX_MQ_CNT (4). However, bcmgenet_probe() only initialized
the u64_stats_sync seq counter up to priv->hw_params->rx_queues and
priv->hw_params->tx_queues.

Since priv->hw_params->rx_queues is 0 across all hardware versions (and
priv->hw_params->tx_queues is 0 on GENET V1), rings 1..4 have uninitialized
u64_stats_sync structures. When ethtool -S is run on 32-bit kernels,
bcmgenet_get_ethtool_stats() reads stats from rx_rings[1..4], causing
lockdep warnings due to the uninitialized sequence counters.

Initialize the sequence counters for all GENET_MAX_MQ_CNT + 1 queues.

Fixes: ffc2c8c4a714 ("net: bcmgenet: Initialize u64 stats seq counter")
Assisted-by: LLM
Co-authored-by: Cursor <cursoragent@cursor.com>
Change-Id: I6c3debbd9fa5e7a151789fccf3d9a38db184f434
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 7b089de9484e..055e1362173b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -4134,10 +4134,10 @@ static int bcmgenet_probe(struct platform_device *pdev)
 		priv->rx_rings[i].rx_max_coalesced_frames = 1;
 
 	/* Initialize u64 stats seq counter for 32bit machines */
-	for (i = 0; i <= priv->hw_params->rx_queues; i++)
+	for (i = 0; i <= GENET_MAX_MQ_CNT; i++) {
 		u64_stats_init(&priv->rx_rings[i].stats64.syncp);
-	for (i = 0; i <= priv->hw_params->tx_queues; i++)
 		u64_stats_init(&priv->tx_rings[i].stats64.syncp);
+	}
 
 	/* libphy will determine the link state */
 	netif_carrier_off(dev);
-- 
2.34.1

[PATCH net 3/8] net: bcmgenet: do not skip WoL power up on GENET V1

[Florian Fainelli]

bcmgenet_power_up() had an early check for bcmgenet_has_ext(priv) before
dispatching by power mode. GENET V1 does not have the EXT block (unlike
GENET V2+), which causes bcmgenet_power_up() to immediately return 0.

As a consequence, when waking up from GENET_POWER_WOL_MAGIC on GENET V1,
bcmgenet_wol_power_up_cfg() is never invoked to disable the WoL clock,
clear wake event masks, and restore normal PHY and MAC operations.

Move the bcmgenet_has_ext() checks to the GENET_POWER_PASSIVE and
GENET_POWER_CABLE_SENSE cases where the EXT registers are actually
accessed, allowing GENET_POWER_WOL_MAGIC cleanup to execute on all
hardware versions.

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Assisted-by: LLM
Co-authored-by: Cursor <cursoragent@cursor.com>
Change-Id: Id6c85b2790191786aa535d71fea357d1cd49a2ec
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 055e1362173b..bfb74dca55d6 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1762,13 +1762,12 @@ static int bcmgenet_power_up(struct bcmgenet_priv *priv,
 	int ret = 0;
 	u32 reg;
 
-	if (!bcmgenet_has_ext(priv))
-		return ret;
-
-	reg = bcmgenet_ext_readl(priv, EXT_EXT_PWR_MGMT);
-
 	switch (mode) {
 	case GENET_POWER_PASSIVE:
+		if (!bcmgenet_has_ext(priv))
+			break;
+
+		reg = bcmgenet_ext_readl(priv, EXT_EXT_PWR_MGMT);
 		reg &= ~(EXT_PWR_DOWN_DLL | EXT_PWR_DOWN_BIAS |
 			 EXT_ENERGY_DET_MASK);
 		if (GENET_IS_V5(priv) && !bcmgenet_has_ephy_16nm(priv)) {
@@ -1792,8 +1791,12 @@ static int bcmgenet_power_up(struct bcmgenet_priv *priv,
 		break;
 
 	case GENET_POWER_CABLE_SENSE:
+		if (!bcmgenet_has_ext(priv))
+			break;
+
 		/* enable APD */
 		if (!GENET_IS_V5(priv)) {
+			reg = bcmgenet_ext_readl(priv, EXT_EXT_PWR_MGMT);
 			reg |= EXT_PWR_DN_EN_LD;
 			bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
 		}
-- 
2.34.1

[PATCH net 4/8] net: bcmgenet: clean up RX NAPI on bcmgenet_init_rx_queues failure

[Florian Fainelli]

bcmgenet_init_rx_ring() registers a NAPI instance for each RX ring via
netif_napi_add(). If bcmgenet_init_rx_queues() fails while initializing
subsequent rings, bcmgenet_init_dma() frees the RX buffers and page pools
but fails to clean up already registered NAPI instances with
bcmgenet_fini_rx_napi(). This leaves stale NAPI structs on the net_device
napi_list.

Call bcmgenet_fini_rx_napi() in the error handling path of
bcmgenet_init_dma() when bcmgenet_init_rx_queues() fails.

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Assisted-by: LLM
Co-authored-by: Cursor <cursoragent@cursor.com>
Change-Id: I45982e0e198a44f3f56b1300f462870462b81108
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index bfb74dca55d6..ef155a170fa6 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3167,6 +3167,7 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx)
 	ret = bcmgenet_init_rx_queues(priv->dev);
 	if (ret) {
 		netdev_err(priv->dev, "failed to initialize Rx queues\n");
+		bcmgenet_fini_rx_napi(priv);
 		bcmgenet_free_rx_buffers(priv);
 		bcmgenet_destroy_rx_page_pools(priv);
 		kfree(priv->rx_cbs);
-- 
2.34.1

[PATCH net 5/8] net: bcmgenet: acquire ring lock with BH disabled in bcmgenet_dump_tx_queue

[Florian Fainelli]

bcmgenet_dump_tx_queue() is called from bcmgenet_timeout() in process or
timer context and acquires ring->lock using spin_lock(). If a softirq
such as TX NAPI (bcmgenet_tx_poll()) fires on the same CPU while the lock
is held, it will deadlock trying to acquire ring->lock.

Use spin_lock_bh() and spin_unlock_bh() in bcmgenet_dump_tx_queue(),
matching bcmgenet_tx_reclaim().

Fixes: 13ea657806cf ("net: bcmgenet: improve TX timeout")
Assisted-by: LLM
Co-authored-by: Cursor <cursoragent@cursor.com>
Change-Id: I0c4c6392590b2d16abd44f75d3f6974f3e1b874b
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index ef155a170fa6..799da63e3a4e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3508,14 +3508,14 @@ static void bcmgenet_dump_tx_queue(struct bcmgenet_tx_ring *ring)
 
 	txq = netdev_get_tx_queue(priv->dev, ring->index);
 
-	spin_lock(&ring->lock);
+	spin_lock_bh(&ring->lock);
 	intsts = ~bcmgenet_intrl2_1_readl(priv, INTRL2_CPU_MASK_STATUS);
 	intmsk = 1 << ring->index;
 	c_index = bcmgenet_tdma_ring_readl(priv, ring->index, TDMA_CONS_INDEX);
 	p_index = bcmgenet_tdma_ring_readl(priv, ring->index, TDMA_PROD_INDEX);
 	txq_stopped = netif_tx_queue_stopped(txq);
 	free_bds = ring->free_bds;
-	spin_unlock(&ring->lock);
+	spin_unlock_bh(&ring->lock);
 
 	netif_err(priv, tx_err, priv->dev, "Ring %d queue %d status summary\n"
 		  "TX queue status: %s, interrupts: %s\n"
-- 
2.34.1

[PATCH net 6/8] net: bcmgenet: validate Ethernet address in bcmgenet_set_mac_addr

[Florian Fainelli]

bcmgenet_set_mac_addr() did not check whether the provided MAC address is a
valid Ethernet address before applying it. Userspace could configure an
invalid address (such as all zeroes or a multicast address) while the
interface is down.

Add a call to is_valid_ether_addr() and return -EADDRNOTAVAIL if the MAC
address is not valid.

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Assisted-by: LLM
Co-authored-by: Cursor <cursoragent@cursor.com>
Change-Id: I1cae9178ff2c9243510556f42a194058c10aea7e
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 799da63e3a4e..90ca4f220cc7 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3635,6 +3635,9 @@ static int bcmgenet_set_mac_addr(struct net_device *dev, void *p)
 	if (netif_running(dev))
 		return -EBUSY;
 
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
 	eth_hw_addr_set(dev, addr->sa_data);
 
 	return 0;
-- 
2.34.1

[PATCH net 7/8] net: bcmgenet: zero-initialize Transmit Status Block in bcmgenet_add_tsb

[Florian Fainelli]

bcmgenet_add_tsb() pushes 64 bytes of headroom onto the skb to form the
Transmit Status Block (TSB) but only assigns status->tx_csum_info when
skb->ip_summed == CHECKSUM_PARTIAL. For packets without checksum offload,
or for other fields within struct status_64, the status block contains
uninitialized stack/heap memory from previous skb operations.

Zero-initialize the status block with memset() after pushing headroom.

Fixes: 9a9ba2a4aaaa ("net: bcmgenet: always enable status blocks")
Assisted-by: LLM
Co-authored-by: Cursor <cursoragent@cursor.com>
Change-Id: I6935e24408ea10f68eb61dce4cced9b0ead5c005
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 90ca4f220cc7..3a6a2f075959 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2084,6 +2084,7 @@ static struct sk_buff *bcmgenet_add_tsb(struct net_device *dev,
 
 	skb_push(skb, sizeof(*status));
 	status = (struct status_64 *)skb->data;
+	memset(status, 0, sizeof(*status));
 
 	if (skb->ip_summed  == CHECKSUM_PARTIAL) {
 		ip_ver = skb->protocol;
-- 
2.34.1

[PATCH net 8/8] net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT

[Florian Fainelli]

bcmgenet_get_coalesce() reads DMA_RING0_TIMEOUT to calculate
rx_coalesce_usecs without masking out bits outside DMA_TIMEOUT_MASK
(16 bits). If upper bits are non-zero or contain status/flags, the
computed value of rx_coalesce_usecs returned to userspace via ethtool
becomes corrupted.

Mask the register read with DMA_TIMEOUT_MASK before computing the
timeout in microseconds.

Fixes: 4a29645bfe6c ("net: bcmgenet: Implement RX coalescing control knobs")
Assisted-by: LLM
Co-authored-by: Cursor <cursoragent@cursor.com>
Change-Id: I4c5b5019e764dbd0c7e51bd8c7365939a4736e44
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 3a6a2f075959..b15e64a484b4 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -852,7 +852,8 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
 	ec->rx_max_coalesced_frames =
 		bcmgenet_rdma_ring_readl(priv, 0, DMA_MBUF_DONE_THRESH);
 	ec->rx_coalesce_usecs =
-		bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT) * 8192 / 1000;
+		(bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT) &
+		 DMA_TIMEOUT_MASK) * 8192 / 1000;
 
 	for (i = 0; i <= priv->hw_params->rx_queues; i++) {
 		ring = &priv->rx_rings[i];
-- 
2.34.1

Re: [PATCH net 0/8] net: bcmgenet: Collection of bug fixes

[Florian Fainelli]

On 9/17/2026 5:07 PM, Florian Fainelli wrote:
> This patch series contains a collection of bug fixes found during a
> LLM-assisted coding session.

My bad, I forgot to remove some tags while the patches were sent out, 
will wait for feedback and repost.

> 
> Florian Fainelli (8):
>    net: bcmgenet: fix 64-bit RTNL stats reading in ethtool on 32-bit
>      systems
>    net: bcmgenet: initialize u64 stats seq counter for all queues
>    net: bcmgenet: do not skip WoL power up on GENET V1
>    net: bcmgenet: clean up RX NAPI on bcmgenet_init_rx_queues failure
>    net: bcmgenet: acquire ring lock with BH disabled in
>      bcmgenet_dump_tx_queue
>    net: bcmgenet: validate Ethernet address in bcmgenet_set_mac_addr
>    net: bcmgenet: zero-initialize Transmit Status Block in
>      bcmgenet_add_tsb
>    net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT
> 
>   .../net/ethernet/broadcom/genet/bcmgenet.c    | 34 ++++++++++++-------
>   1 file changed, 21 insertions(+), 13 deletions(-)
> 

-- 
Florian

Re: [PATCH net 1/8] net: bcmgenet: fix 64-bit RTNL stats reading in ethtool on 32-bit systems

[Nicolai Buchwitz]

On 18.9.2026 02:07, Florian Fainelli wrote:
> When bcmgenet was converted to 64-bit statistics, STAT_RTNL members 
> were
> switched to point into struct rtnl_link_stats64, whose fields are 
> 64-bit
> (__u64) regardless of architecture.
> 
> However, bcmgenet_get_ethtool_stats() retained a legacy check:
>   if (sizeof(unsigned long) != sizeof(u32) &&
>       s->stat_sizeof == sizeof(unsigned long))
> 
> On 32-bit systems, sizeof(unsigned long) == sizeof(u32), causing this
> condition to evaluate to false. As a result, 64-bit RTNL stats fields 
> were
> read via *(u32 *)p. On 32-bit Big-Endian systems (such as MIPS BE), 
> this
> reads the high 32 bits and returns 0 until the counter exceeds 4GB; on
> 32-bit Little-Endian systems (such as 32-bit ARM), the value is 
> truncated
> to 32 bits.
> 
> Fix this by checking if s->stat_sizeof == sizeof(u64) so 64-bit fields 
> are
> always read as 64-bit values.
> 
> Fixes: 59aa6e3072aa ("net: bcmgenet: switch to use 64bit statistics")
> Assisted-by: LLM
> Co-authored-by: Cursor <cursoragent@cursor.com>
> Change-Id: I3e9f2f8d4fd136392148d6c0cd4eb40f7ccbe4ef
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index b916080f4ff1..7b089de9484e 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1346,9 +1346,8 @@ static void bcmgenet_get_ethtool_stats(struct 
> net_device *dev,
>  				p = (char *)&stats64;
> 
>  			p += s->stat_offset;
> -			if (sizeof(unsigned long) != sizeof(u32) &&
> -				s->stat_sizeof == sizeof(unsigned long))
> -				data[i] = *(unsigned long *)p;
> +			if (s->stat_sizeof == sizeof(u64))
> +				data[i] = *(u64 *)p;
>  			else
>  				data[i] = *(u32 *)p;
>  		}

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net 2/8] net: bcmgenet: initialize u64 stats seq counter for all queues

[Nicolai Buchwitz]

On 18.9.2026 02:07, Florian Fainelli wrote:
> bcmgenet_gstrings_stats statically defines ethtool statistics for 
> queues
> 0 through GENET_MAX_MQ_CNT (4). However, bcmgenet_probe() only 
> initialized
> the u64_stats_sync seq counter up to priv->hw_params->rx_queues and
> priv->hw_params->tx_queues.
> 
> Since priv->hw_params->rx_queues is 0 across all hardware versions (and
> priv->hw_params->tx_queues is 0 on GENET V1), rings 1..4 have 
> uninitialized
> u64_stats_sync structures. When ethtool -S is run on 32-bit kernels,
> bcmgenet_get_ethtool_stats() reads stats from rx_rings[1..4], causing
> lockdep warnings due to the uninitialized sequence counters.
> 
> Initialize the sequence counters for all GENET_MAX_MQ_CNT + 1 queues.
> 
> Fixes: ffc2c8c4a714 ("net: bcmgenet: Initialize u64 stats seq counter")
> Assisted-by: LLM
> Co-authored-by: Cursor <cursoragent@cursor.com>
> Change-Id: I6c3debbd9fa5e7a151789fccf3d9a38db184f434
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 7b089de9484e..055e1362173b 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -4134,10 +4134,10 @@ static int bcmgenet_probe(struct 
> platform_device *pdev)
>  		priv->rx_rings[i].rx_max_coalesced_frames = 1;
> 
>  	/* Initialize u64 stats seq counter for 32bit machines */
> -	for (i = 0; i <= priv->hw_params->rx_queues; i++)
> +	for (i = 0; i <= GENET_MAX_MQ_CNT; i++) {
>  		u64_stats_init(&priv->rx_rings[i].stats64.syncp);
> -	for (i = 0; i <= priv->hw_params->tx_queues; i++)
>  		u64_stats_init(&priv->tx_rings[i].stats64.syncp);
> +	}
> 
>  	/* libphy will determine the link state */
>  	netif_carrier_off(dev);

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net 8/8] net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT

[Nicolai Buchwitz]

On 18.9.2026 02:07, Florian Fainelli wrote:
> bcmgenet_get_coalesce() reads DMA_RING0_TIMEOUT to calculate
> rx_coalesce_usecs without masking out bits outside DMA_TIMEOUT_MASK
> (16 bits). If upper bits are non-zero or contain status/flags, the
> computed value of rx_coalesce_usecs returned to userspace via ethtool
> becomes corrupted.
> 
> Mask the register read with DMA_TIMEOUT_MASK before computing the
> timeout in microseconds.
> 
> Fixes: 4a29645bfe6c ("net: bcmgenet: Implement RX coalescing control 
> knobs")
> Assisted-by: LLM
> Co-authored-by: Cursor <cursoragent@cursor.com>
> Change-Id: I4c5b5019e764dbd0c7e51bd8c7365939a4736e44
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 3a6a2f075959..b15e64a484b4 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -852,7 +852,8 @@ static int bcmgenet_get_coalesce(struct net_device 
> *dev,
>  	ec->rx_max_coalesced_frames =
>  		bcmgenet_rdma_ring_readl(priv, 0, DMA_MBUF_DONE_THRESH);
>  	ec->rx_coalesce_usecs =
> -		bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT) * 8192 / 1000;
> +		(bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT) &
> +		 DMA_TIMEOUT_MASK) * 8192 / 1000;
> 
>  	for (i = 0; i <= priv->hw_params->rx_queues; i++) {
>  		ring = &priv->rx_rings[i];

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net 3/8] net: bcmgenet: do not skip WoL power up on GENET V1

[Nicolai Buchwitz]

On 18.9.2026 02:07, Florian Fainelli wrote:
> bcmgenet_power_up() had an early check for bcmgenet_has_ext(priv) 
> before
> dispatching by power mode. GENET V1 does not have the EXT block (unlike
> GENET V2+), which causes bcmgenet_power_up() to immediately return 0.
> 
> As a consequence, when waking up from GENET_POWER_WOL_MAGIC on GENET 
> V1,
> bcmgenet_wol_power_up_cfg() is never invoked to disable the WoL clock,
> clear wake event masks, and restore normal PHY and MAC operations.
> 
> Move the bcmgenet_has_ext() checks to the GENET_POWER_PASSIVE and
> GENET_POWER_CABLE_SENSE cases where the EXT registers are actually
> accessed, allowing GENET_POWER_WOL_MAGIC cleanup to execute on all
> hardware versions.
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")

c3ae64ae0c08 ("net: bcmgenet: handle GENET_POWER_WOL_MAGIC") ?

> [...]

With the correct Fixes tag:

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net 4/8] net: bcmgenet: clean up RX NAPI on bcmgenet_init_rx_queues failure

[Nicolai Buchwitz]

Hi Florian

On 18.9.2026 02:07, Florian Fainelli wrote:
> bcmgenet_init_rx_ring() registers a NAPI instance for each RX ring via
> netif_napi_add(). If bcmgenet_init_rx_queues() fails while initializing
> subsequent rings, bcmgenet_init_dma() frees the RX buffers and page 
> pools
> but fails to clean up already registered NAPI instances with
> bcmgenet_fini_rx_napi(). This leaves stale NAPI structs on the 
> net_device
> napi_list.
> 
> Call bcmgenet_fini_rx_napi() in the error handling path of
> bcmgenet_init_dma() when bcmgenet_init_rx_queues() fails.
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Assisted-by: LLM
> Co-authored-by: Cursor <cursoragent@cursor.com>
> Change-Id: I45982e0e198a44f3f56b1300f462870462b81108
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index bfb74dca55d6..ef155a170fa6 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3167,6 +3167,7 @@ static int bcmgenet_init_dma(struct bcmgenet_priv 
> *priv, bool flush_rx)
>  	ret = bcmgenet_init_rx_queues(priv->dev);
>  	if (ret) {
>  		netdev_err(priv->dev, "failed to initialize Rx queues\n");
> +		bcmgenet_fini_rx_napi(priv);


napi->dev is still NULL here, bcmgenet_init_rx_ring() only fails before
netif_napi_add().  netif_napi_del() does netdev_lock(napi->dev), so this
oopses on -ENOMEM at first open.

rx_queues is 0 in all hw_params anyway, so there is nothing to clean up.
Drop this one?

>  		bcmgenet_free_rx_buffers(priv);
>  		bcmgenet_destroy_rx_page_pools(priv);
>  		kfree(priv->rx_cbs);

Thanks,
Nicolai

Re: [PATCH net 5/8] net: bcmgenet: acquire ring lock with BH disabled in bcmgenet_dump_tx_queue

[Nicolai Buchwitz]

Hi Florian

On 18.9.2026 02:07, Florian Fainelli wrote:
> bcmgenet_dump_tx_queue() is called from bcmgenet_timeout() in process 
> or
> timer context and acquires ring->lock using spin_lock(). If a softirq
> such as TX NAPI (bcmgenet_tx_poll()) fires on the same CPU while the 
> lock
> is held, it will deadlock trying to acquire ring->lock.

AFAIU bcmgenet_timeout() only runs from dev_watchdog(), so BH is already
disabled and no deadlock?

> 
> Use spin_lock_bh() and spin_unlock_bh() in bcmgenet_dump_tx_queue(),
> matching bcmgenet_tx_reclaim().
> 
> Fixes: 13ea657806cf ("net: bcmgenet: improve TX timeout")
> Assisted-by: LLM
> Co-authored-by: Cursor <cursoragent@cursor.com>
> Change-Id: I0c4c6392590b2d16abd44f75d3f6974f3e1b874b
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index ef155a170fa6..799da63e3a4e 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3508,14 +3508,14 @@ static void bcmgenet_dump_tx_queue(struct 
> bcmgenet_tx_ring *ring)
> 
>  	txq = netdev_get_tx_queue(priv->dev, ring->index);
> 
> -	spin_lock(&ring->lock);
> +	spin_lock_bh(&ring->lock);
>  	intsts = ~bcmgenet_intrl2_1_readl(priv, INTRL2_CPU_MASK_STATUS);
>  	intmsk = 1 << ring->index;
>  	c_index = bcmgenet_tdma_ring_readl(priv, ring->index, 
> TDMA_CONS_INDEX);
>  	p_index = bcmgenet_tdma_ring_readl(priv, ring->index, 
> TDMA_PROD_INDEX);
>  	txq_stopped = netif_tx_queue_stopped(txq);
>  	free_bds = ring->free_bds;
> -	spin_unlock(&ring->lock);
> +	spin_unlock_bh(&ring->lock);
> 
>  	netif_err(priv, tx_err, priv->dev, "Ring %d queue %d status 
> summary\n"
>  		  "TX queue status: %s, interrupts: %s\n"

Regards,
Nicolai

Re: [PATCH net 6/8] net: bcmgenet: validate Ethernet address in bcmgenet_set_mac_addr

[Nicolai Buchwitz]

On 18.9.2026 02:07, Florian Fainelli wrote:
> bcmgenet_set_mac_addr() did not check whether the provided MAC address 
> is a
> valid Ethernet address before applying it. Userspace could configure an
> invalid address (such as all zeroes or a multicast address) while the
> interface is down.
> 
> Add a call to is_valid_ether_addr() and return -EADDRNOTAVAIL if the 
> MAC
> address is not valid.
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Assisted-by: LLM
> Co-authored-by: Cursor <cursoragent@cursor.com>
> Change-Id: I1cae9178ff2c9243510556f42a194058c10aea7e
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 799da63e3a4e..90ca4f220cc7 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3635,6 +3635,9 @@ static int bcmgenet_set_mac_addr(struct 
> net_device *dev, void *p)
>  	if (netif_running(dev))
>  		return -EBUSY;
> 
> +	if (!is_valid_ether_addr(addr->sa_data))
> +		return -EADDRNOTAVAIL;
> +
>  	eth_hw_addr_set(dev, addr->sa_data);
> 
>  	return 0;

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net 7/8] net: bcmgenet: zero-initialize Transmit Status Block in bcmgenet_add_tsb

[Nicolai Buchwitz]

Hi Florian

On 18.9.2026 02:07, Florian Fainelli wrote:
> bcmgenet_add_tsb() pushes 64 bytes of headroom onto the skb to form the
> Transmit Status Block (TSB) but only assigns status->tx_csum_info when
> skb->ip_summed == CHECKSUM_PARTIAL. For packets without checksum 
> offload,
> or for other fields within struct status_64, the status block contains
> uninitialized stack/heap memory from previous skb operations.

The MAC only reads tx_csum_info when the descriptor has DMA_TX_DO_CSUM,
and bcmgenet_xmit() sets that only for CHECKSUM_PARTIAL.
The stale value seems to be never used?

Did you hit an actual failure?  Otherwise this would be a 64 byte memset
on every transmitted packet.

> [...]

Thanks,
Nicolai

Re: [PATCH net 4/8] net: bcmgenet: clean up RX NAPI on bcmgenet_init_rx_queues failure

[Florian Fainelli]

On 9/18/26 03:43, Nicolai Buchwitz wrote:
> Hi Florian
> 
> On 18.9.2026 02:07, Florian Fainelli wrote:
>> bcmgenet_init_rx_ring() registers a NAPI instance for each RX ring via
>> netif_napi_add(). If bcmgenet_init_rx_queues() fails while initializing
>> subsequent rings, bcmgenet_init_dma() frees the RX buffers and page pools
>> but fails to clean up already registered NAPI instances with
>> bcmgenet_fini_rx_napi(). This leaves stale NAPI structs on the net_device
>> napi_list.
>>
>> Call bcmgenet_fini_rx_napi() in the error handling path of
>> bcmgenet_init_dma() when bcmgenet_init_rx_queues() fails.
>>
>> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>> Assisted-by: LLM
>> Co-authored-by: Cursor <cursoragent@cursor.com>
>> Change-Id: I45982e0e198a44f3f56b1300f462870462b81108
>> ---
>>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/ 
>> net/ethernet/broadcom/genet/bcmgenet.c
>> index bfb74dca55d6..ef155a170fa6 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -3167,6 +3167,7 @@ static int bcmgenet_init_dma(struct 
>> bcmgenet_priv *priv, bool flush_rx)
>>      ret = bcmgenet_init_rx_queues(priv->dev);
>>      if (ret) {
>>          netdev_err(priv->dev, "failed to initialize Rx queues\n");
>> +        bcmgenet_fini_rx_napi(priv);
> 
> 
> napi->dev is still NULL here, bcmgenet_init_rx_ring() only fails before
> netif_napi_add().  netif_napi_del() does netdev_lock(napi->dev), so this
> oopses on -ENOMEM at first open.
> 
> rx_queues is 0 in all hw_params anyway, so there is nothing to clean up.
> Drop this one?

Yes, that's a good point, thanks!
-- 
Florian

Re: [PATCH net 5/8] net: bcmgenet: acquire ring lock with BH disabled in bcmgenet_dump_tx_queue

[Florian Fainelli]

On 9/18/26 03:57, Nicolai Buchwitz wrote:
> Hi Florian
> 
> On 18.9.2026 02:07, Florian Fainelli wrote:
>> bcmgenet_dump_tx_queue() is called from bcmgenet_timeout() in process or
>> timer context and acquires ring->lock using spin_lock(). If a softirq
>> such as TX NAPI (bcmgenet_tx_poll()) fires on the same CPU while the lock
>> is held, it will deadlock trying to acquire ring->lock.
> 
> AFAIU bcmgenet_timeout() only runs from dev_watchdog(), so BH is already
> disabled and no deadlock?

Yes indeed, not sure what I was on with that.
-- 
Florian

Re: [PATCH net 7/8] net: bcmgenet: zero-initialize Transmit Status Block in bcmgenet_add_tsb

[Florian Fainelli]

On 9/18/26 04:03, Nicolai Buchwitz wrote:
> Hi Florian
> 
> On 18.9.2026 02:07, Florian Fainelli wrote:
>> bcmgenet_add_tsb() pushes 64 bytes of headroom onto the skb to form the
>> Transmit Status Block (TSB) but only assigns status->tx_csum_info when
>> skb->ip_summed == CHECKSUM_PARTIAL. For packets without checksum offload,
>> or for other fields within struct status_64, the status block contains
>> uninitialized stack/heap memory from previous skb operations.
> 
> The MAC only reads tx_csum_info when the descriptor has DMA_TX_DO_CSUM,
> and bcmgenet_xmit() sets that only for CHECKSUM_PARTIAL.
> The stale value seems to be never used?
> 
> Did you hit an actual failure?  Otherwise this would be a 64 byte memset
> on every transmitted packet.

Nope this is entirely theoretical, and one could argue that this could 
create a slight drop in performance, happy to drop that one as well 
since we never really experienced any actual issue.
-- 
Florian

Re: [PATCH net 1/8] net: bcmgenet: fix 64-bit RTNL stats reading in ethtool on 32-bit systems

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message of "net: bcmgenet: fix 64-bit RTNL stats
  reading in ethtool on 32-bit systems" has no Signed-off-by: line, so…

--

net: bcmgenet: fix 64-bit RTNL stats reading in ethtool on 32-bit systems

This changes the width test in bcmgenet_get_ethtool_stats() from a check
against sizeof(unsigned long) to a check against sizeof(u64), so that
STAT_RTNL entries pointing into struct rtnl_link_stats64 are read as
64-bit values on 32-bit architectures too.

No Link: tags are present in the commit header.

The code change itself looks fine; my only comments are about the trailer
block of the commit message:

>     Fixes: 59aa6e3072aa ("net: bcmgenet: switch to use 64bit statistics")
>     Assisted-by: LLM
>     Co-authored-by: Cursor <cursoragent@cursor.com>
>     Change-Id: I3e9f2f8d4fd136392148d6c0cd4eb40f7ccbe4ef

[Severity: Medium]
There is no Signed-off-by: line in this commit, so the Developer's
Certificate of Origin chain described in
Documentation/process/submitting-patches.rst is missing.  Can this be
applied as posted, or does it need a respin with a Signed-off-by: from the
author?

The remaining trailers also look unusual for an upstream posting:

  - Change-Id: I3e9f2f8d4fd136392148d6c0cd4eb40f7ccbe4ef is a Gerrit-internal
    identifier and is normally stripped before submission.
  - Assisted-by: LLM is not a trailer the kernel uses.
  - Co-authored-by: Cursor <cursoragent@cursor.com> does not match the
    kernel convention, which is Co-developed-by: immediately followed by
    that co-developer's own Signed-off-by:.  Can an agent address provide
    such a sign-off?

The same trailer block, with no Signed-off-by:, appears in all eight commits
of the series range 46bc52d1..e6810d7c, so no later patch in the series
corrects it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com

Re: [PATCH net 2/8] net: bcmgenet: initialize u64 stats seq counter for all queues

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit has no Signed-off-by line and carries tool artifacts
  that must not reach upstream: `Change-Id:…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but writers to
  the same `tx_rings[i].stats64.syncp` are not mutually exclusive, which…

--

net: bcmgenet: initialize u64 stats seq counter for all queues

bcmgenet_probe() only initialized the u64_stats_sync seq counters up to
priv->hw_params->rx_queues and priv->hw_params->tx_queues, leaving the
seq counters for rings 1..4 uninitialized while ethtool -S still reads
them.  The patch initializes both rx_rings[] and tx_rings[] seq counters
for all GENET_MAX_MQ_CNT + 1 entries.

A couple of questions below, one about the trailer block and one about
the locking around the tx ring syncp that this loop initializes.

>     Fixes: ffc2c8c4a714 ("net: bcmgenet: Initialize u64 stats seq counter")
>     Assisted-by: LLM
>     Co-authored-by: Cursor <cursoragent@cursor.com>
>     Change-Id: I6c3debbd9fa5e7a151789fccf3d9a38db184f434

[Severity: Low]
The trailer block has no Signed-off-by line from the author or submitter,
which Documentation/process/submitting-patches.rst lists as mandatory for
the Developer's Certificate of Origin.  Was it dropped by accident?

The Gerrit Change-Id line is also still present, along with the
Assisted-by and Co-authored-by: Cursor trailers.  Should the Change-Id be
stripped before posting, since stable backport tooling parses this
Fixes:/Signed-off-by trailer block?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 7b089de9484e3..055e1362173b4 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -4134,10 +4134,10 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  		priv->rx_rings[i].rx_max_coalesced_frames = 1;
>  
>  	/* Initialize u64 stats seq counter for 32bit machines */
> -	for (i = 0; i <= priv->hw_params->rx_queues; i++)
> +	for (i = 0; i <= GENET_MAX_MQ_CNT; i++) {
>  		u64_stats_init(&priv->rx_rings[i].stats64.syncp);
> -	for (i = 0; i <= priv->hw_params->tx_queues; i++)
>  		u64_stats_init(&priv->tx_rings[i].stats64.syncp);
> +	}

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the tx
syncp being initialized here: are the writers to a given
tx_rings[i].stats64.syncp actually mutually exclusive on 32-bit?

The NAPI-side writer in __bcmgenet_tx_reclaim() runs with ring->lock
held, taken by bcmgenet_tx_poll() and bcmgenet_tx_reclaim():

	spin_lock_bh(&ring->lock);
	released = __bcmgenet_tx_reclaim(dev, ring);
	...
	u64_stats_update_begin(&stats->syncp);
	u64_stats_add(&stats->packets, pkts_compl);
	u64_stats_add(&stats->bytes, bytes_compl);
	u64_stats_update_end(&stats->syncp);

bcmgenet_timeout() writes the same syncp after bcmgenet_tx_reclaim() has
already dropped that lock:

	bcmgenet_tx_reclaim(dev, ring, true);
	...
	txq_trans_cond_update(txq);

	BCMGENET_STATS64_INC((&ring->stats64), errors);

The timeout callback is invoked from dev_watchdog() as:

	netif_freeze_queues(dev);
	dev->netdev_ops->ndo_tx_timeout(dev, i);
	netif_unfreeze_queues(dev);

which stops queueing but does not quiesce the TX NAPI, so
bcmgenet_tx_poll() for the same ring can be running on another CPU.

On 32-bit, __u64_stats_update_begin() is only:

	preempt_disable_nested();
	write_seqcount_begin(&syncp->seq);

so there is no cross-CPU exclusion and the sequence increment is a plain
read-modify-write.  Can interleaved writers here hand readers such as
bcmgenet_get_stats64() and bcmgenet_get_ethtool_stats() torn 64-bit
values, or leave the sequence permanently odd so that later readers spin
in read_seqcount_begin()?

Would moving the errors increment under ring->lock be appropriate?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com

Re: [PATCH net 3/8] net: bcmgenet: do not skip WoL power up on GENET V1

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The submitted commit "net: bcmgenet: do not skip WoL power up on
  GENET V1" (d8dc741c84b2) carries no Signed-off-by: line.

--

net: bcmgenet: do not skip WoL power up on GENET V1

The patch moves the bcmgenet_has_ext() test out of the top of
bcmgenet_power_up() and into the GENET_POWER_PASSIVE and
GENET_POWER_CABLE_SENSE cases, so that GENET_POWER_WOL_MAGIC reaches
bcmgenet_wol_power_up_cfg() on GENET V1 where the EXT block is absent.

The only comment below is about the commit message trailers, so the diff
hunks are not quoted.

>     Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>     Assisted-by: LLM
>     Co-authored-by: Cursor <cursoragent@cursor.com>
>     Change-Id: Id6c85b2790191786aa535d71fea357d1cd49a2ec

[Severity: Low]
This isn't a code bug, but the trailer block of this commit has no
Signed-off-by: line. Without it the Developer's Certificate of Origin is
not asserted, so can this be applied by a netdev maintainer as posted?

The block also ends with:

    Change-Id: Id6c85b2790191786aa535d71fea357d1cd49a2ec

Change-Id is a Gerrit-local artifact. Should it be stripped before posting
so it does not end up in kernel history?

Since the change carries a Fixes: tag and is therefore a stable backport
candidate, the authorship and sign-off trailers matter for the tooling that
picks it up. The same trailer pattern appears on the other commits of this
series, so would it make sense to fix them all together in a v2?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com

Re: [PATCH net 4/8] net: bcmgenet: clean up RX NAPI on bcmgenet_init_rx_queues failure

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] New error path in bcmgenet_init_dma() calls
  bcmgenet_fini_rx_napi(priv) after bcmgenet_init_rx_queues() failed.
- [Medium] The commit message's premise is not reachable in any supported
  configuration: it claims that 'If bcmgenet_init_rx_queues() fails…
- [Low] The commit message trailer block contains a Gerrit-generated
  'Change-Id: I45982e0e198a44f3f56b1300f462870462b81108' line.

--

net: bcmgenet: clean up RX NAPI on bcmgenet_init_rx_queues failure

This adds a bcmgenet_fini_rx_napi(priv) call to the error path of
bcmgenet_init_dma() when bcmgenet_init_rx_queues() fails, with the goal of
removing NAPI instances that were already registered by earlier iterations
of the ring initialization loop.  A few questions below about the added
call and about the commit message.

> net: bcmgenet: clean up RX NAPI on bcmgenet_init_rx_queues failure
>
> bcmgenet_init_rx_ring() registers a NAPI instance for each RX ring via
> netif_napi_add(). If bcmgenet_init_rx_queues() fails while initializing
> subsequent rings, bcmgenet_init_dma() frees the RX buffers and page pools
> but fails to clean up already registered NAPI instances with
> bcmgenet_fini_rx_napi(). This leaves stale NAPI structs on the net_device
> napi_list.

[Severity: Medium]
Is the scenario quoted above reachable on any supported hardware?  All four
hardware parameter tables (bcmgenet_hw_params_v1 through
bcmgenet_hw_params_v4) set:

	.rx_queues = 0,
	.rx_bds_per_q = 0,

and bcmgenet_set_hw_params() only ever selects one of those constant
tables, so the loop in bcmgenet_init_rx_queues():

	for (i = 0; i <= priv->hw_params->rx_queues; i++) {
		ret = bcmgenet_init_rx_ring(priv, i, end - start, start, end);
		if (ret)
			return ret;

runs exactly once, for i == 0.  With only one RX ring there is no
"subsequent rings" case, and both failure returns in
bcmgenet_init_rx_ring() happen before netif_napi_add() runs, so no NAPI
instance is registered when the single ring fails.

Given that, should the Fixes: 1c1008c793fa tag be dropped so this is not
backported to stable as a fix for a leak that cannot happen, and the change
described as preparation for multiple RX rings instead?  Or is there a
configuration where rx_queues is non-zero that I am missing?

> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Assisted-by: LLM
> Co-authored-by: Cursor <cursoragent@cursor.com>
> Change-Id: I45982e0e198a44f3f56b1300f462870462b81108

[Severity: Low]
The Gerrit Change-Id trailer should be removed before submitting upstream;
scripts/checkpatch.pl flags it as an error.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index bfb74dca55d6f..ef155a170fa62 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3167,6 +3167,7 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx)
>  	ret = bcmgenet_init_rx_queues(priv->dev);
>  	if (ret) {
>  		netdev_err(priv->dev, "failed to initialize Rx queues\n");
> +		bcmgenet_fini_rx_napi(priv);

[Severity: High]
Can this dereference a NULL net_device on the first open after probe?

bcmgenet_fini_rx_napi() deletes every ring's NAPI unconditionally, with no
check that netif_napi_add() ever ran for that ring:

	for (i = 0; i <= priv->hw_params->rx_queues; ++i) {
		ring = &priv->rx_rings[i];
		netif_napi_del(&ring->napi);
	}

In bcmgenet_init_rx_ring(), both failure returns are taken before the NAPI
instance is registered:

	ret = bcmgenet_rx_ring_create_pool(priv, ring);
	if (ret)
		return ret;

	ret = bcmgenet_alloc_rx_buffers(priv, ring);
	if (ret) {
		...
		return ret;
	}
	...
	/* Initialize Rx NAPI */
	netif_napi_add(priv->dev, &ring->napi, bcmgenet_rx_poll);

priv->rx_rings[] lives inside the zeroed netdev_priv() area, so on the first
bcmgenet_open() ring->napi.dev is still NULL for a ring that failed.
netif_napi_del() does not tolerate that, because the netdev lock is taken
through napi->dev before the NAPI_STATE_LISTED check can bail out:

include/linux/netdevice.h:__netif_napi_del() {
	netdev_lock(napi->dev);
	__netif_napi_del_locked(napi);
	netdev_unlock(napi->dev);
}

net/core/dev.c:__netif_napi_del_locked() {
	netdev_assert_locked(napi->dev);

	if (!test_and_clear_bit(NAPI_STATE_LISTED, &napi->state))
		return;
	...
}

So an -ENOMEM from page_pool_create() or bcmgenet_alloc_rx_buffers() on
ring 0 during "ip link set eth0 up" would reach
netif_napi_del(&priv->rx_rings[0].napi) with napi.dev == NULL and fault
inside mutex_lock().

>  		bcmgenet_free_rx_buffers(priv);
>  		bcmgenet_destroy_rx_page_pools(priv);
>  		kfree(priv->rx_cbs);

As a side effect of the above, would the rest of this cleanup
(bcmgenet_free_rx_buffers(), bcmgenet_destroy_rx_page_pools(),
kfree(priv->rx_cbs), kfree(priv->tx_cbs)) also be skipped, since the oops
happens before they run?

Would it work better to unwind only the NAPI instances that were actually
registered, for example by deleting indices below the failing one inside
bcmgenet_init_rx_queues(), or by skipping rings where ring->napi.dev is
NULL?

This call is still present at the tip of the series, so it does not look
like a later patch changes it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com

Re: [PATCH net 5/8] net: bcmgenet: acquire ring lock with BH disabled in bcmgenet_dump_tx_queue

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message's justification for converting
  spin_lock()/spin_unlock() to spin_lock_bh()/spin_unlock_bh() in…

Pre-existing issues:
- [Low] bcmgenet_dump_tx_queue()
  (drivers/net/ethernet/broadcom/genet/bcmgenet.c:3500-3535) snapshots…

--

net: bcmgenet: acquire ring lock with BH disabled in bcmgenet_dump_tx_queue

This converts the ring->lock acquisition in bcmgenet_dump_tx_queue() from
spin_lock()/spin_unlock() to spin_lock_bh()/spin_unlock_bh(), with the
stated goal of avoiding a self-deadlock against TX NAPI on the same CPU.
It carries a Fixes tag pointing at 13ea657806cf ("net: bcmgenet: improve
TX timeout").

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index ef155a170fa62..799da63e3a4e4 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

> bcmgenet_dump_tx_queue() is called from bcmgenet_timeout() in process or
> timer context and acquires ring->lock using spin_lock(). If a softirq
> such as TX NAPI (bcmgenet_tx_poll()) fires on the same CPU while the lock
> is held, it will deadlock trying to acquire ring->lock.

[Severity: Medium]
Can bcmgenet_dump_tx_queue() actually run in process context?

The only caller is bcmgenet_timeout(), which is only installed as
.ndo_tx_timeout in the driver's netdev_ops.  The only generic caller of
ndo_tx_timeout is dev_watchdog() in net/sched/sch_generic.c:

static void dev_watchdog(struct timer_list *t)
{
	...
			netif_freeze_queues(dev);
			dev->netdev_ops->ndo_tx_timeout(dev, i);
			netif_unfreeze_queues(dev);

dev_watchdog() is a timer_list callback, so it already runs in
TIMER_SOFTIRQ context with BH disabled.  Since softirq handlers do not
nest on the same CPU, can bcmgenet_tx_poll() (NET_RX softirq) ever
preempt the lock holder here and deadlock as described?

If not, the call chain is:

dev_watchdog()			/* timer softirq, BH already disabled */
  ndo_tx_timeout()
    bcmgenet_timeout()
      bcmgenet_dump_tx_queue()	/* spin_lock_bh() nests local_bh_disable() */

which would make this change a functional no-op rather than a deadlock
fix.  No hardirq handler takes ring->lock either (bcmgenet_isr1 only
touches priv->lock and the interrupt status), so a stronger variant does
not look necessary either.

Would it make sense to either drop the patch, or respin it as a
consistency-only change without the Fixes tag, so that the no-op is not
backported to stable under a deadlock rationale?

> @@ -3508,14 +3508,14 @@ static void bcmgenet_dump_tx_queue(struct bcmgenet_tx_ring *ring)
>  
>  	txq = netdev_get_tx_queue(priv->dev, ring->index);
>  
> -	spin_lock(&ring->lock);
> +	spin_lock_bh(&ring->lock);
>  	intsts = ~bcmgenet_intrl2_1_readl(priv, INTRL2_CPU_MASK_STATUS);
>  	intmsk = 1 << ring->index;
>  	c_index = bcmgenet_tdma_ring_readl(priv, ring->index, TDMA_CONS_INDEX);
>  	p_index = bcmgenet_tdma_ring_readl(priv, ring->index, TDMA_PROD_INDEX);
>  	txq_stopped = netif_tx_queue_stopped(txq);
>  	free_bds = ring->free_bds;
> -	spin_unlock(&ring->lock);
> +	spin_unlock_bh(&ring->lock);
>  
>  	netif_err(priv, tx_err, priv->dev, "Ring %d queue %d status summary\n"
>  		  "TX queue status: %s, interrupts: %s\n"

[Severity: Low]
This isn't a bug introduced by this patch, but while the critical section
is being touched: only free_bds and the two hardware indices are captured
under ring->lock, while the remaining software state is read after the
unlock in the netif_err() argument list:

	free_bds = ring->free_bds;
	spin_unlock_bh(&ring->lock);
	...
		  ring->prod_index, p_index & DMA_P_INDEX_MASK,
		  ring->c_index, c_index & DMA_C_INDEX_MASK,
		  ring->clean_ptr, ring->write_ptr,

__bcmgenet_tx_reclaim() updates those same fields under ring->lock:

	ring->clean_ptr = ...
	...
	ring->free_bds += txbds_processed;
	ring->c_index = c_index;

and it is reachable from bcmgenet_tx_poll() on another CPU, since
netif_freeze_queues() in dev_watchdog() only blocks the xmit path and does
not quiesce TX NAPI.  Can the printed summary then mix free_bds from one
instant with c_index/clean_ptr from a later one, giving an inconsistent
picture of the stall being debugged?  Would moving the remaining reads
inside the critical section into locals be preferable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com

Re: [PATCH net 6/8] net: bcmgenet: validate Ethernet address in bcmgenet_set_mac_addr

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Submission hygiene: the commit message of "net: bcmgenet:
  validate Ethernet address in bcmgenet_set_mac_addr" (2c00a16cb1e7)…

--

net: bcmgenet: validate Ethernet address in bcmgenet_set_mac_addr

This adds an is_valid_ether_addr() check to bcmgenet_set_mac_addr() so that
zero and multicast addresses are rejected with -EADDRNOTAVAIL instead of
being programmed while the interface is down.

The only comment below concerns the commit message trailers, so the diff
hunk is not quoted.

>     Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>     Assisted-by: LLM
>     Co-authored-by: Cursor <cursoragent@cursor.com>
>     Change-Id: I1cae9178ff2c9243510556f42a194058c10aea7e

[Severity: Low]
This isn't a bug in the code, but two things about this trailer block stand
out.

First, the Change-Id line:

    Change-Id: I1cae9178ff2c9243510556f42a194058c10aea7e

That is a Gerrit-specific tag with no meaning in Linux git history.  Could
it be dropped before reposting, as described in
Documentation/process/submitting-patches.rst?

Second, there is no Signed-off-by line from the author or from the listed
co-author, so the Developer's Certificate of Origin is not asserted and the
patch cannot be applied as posted.  Could a respin add the Signed-off-by
and handle the co-author attribution accordingly?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com

Re: [PATCH net 7/8] net: bcmgenet: zero-initialize Transmit Status Block in bcmgenet_add_tsb

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] bcmgenet_add_tsb() now unconditionally writes 64 bytes into
  the skb head (memset(status, 0, sizeof(*status)) at…
- [Low] The commit carries no Signed-off-by: line and includes
  tool/Gerrit-specific trailers.

--

net: bcmgenet: zero-initialize Transmit Status Block in bcmgenet_add_tsb

This adds a memset() of the 64-byte Transmit Status Block after the
skb_push() in bcmgenet_add_tsb(), so that fields other than
tx_csum_info, and the whole block for packets without CHECKSUM_PARTIAL,
are no longer left holding whatever was in the skb headroom before.

Two observations below, one on the trailer block and one on the new write.

>     Fixes: 9a9ba2a4aaaa ("net: bcmgenet: always enable status blocks")
>     Assisted-by: LLM
>     Co-authored-by: Cursor <cursoragent@cursor.com>
>     Change-Id: I6935e24408ea10f68eb61dce4cced9b0ead5c005

[Severity: Low]
This isn't a code bug, but the trailer block has no Signed-off-by: line,
and it still carries a Gerrit Change-Id:.

Documentation/process/submitting-patches.rst requires the Signed-off-by
(Developer's Certificate of Origin), and Change-Id is normally stripped
before posting, so as-is checkpatch/b4 and the netdev CI will reject this.

Can the Signed-off-by be added and the Change-Id dropped, here and in the
rest of the series?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 90ca4f220cc7c..3a6a2f0759598 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2084,6 +2084,7 @@ static struct sk_buff *bcmgenet_add_tsb(struct net_device *dev,
>  
>  	skb_push(skb, sizeof(*status));
>  	status = (struct status_64 *)skb->data;
> +	memset(status, 0, sizeof(*status));

[Severity: Medium]
Can this memset() scribble over headroom that is shared with a clone?

The only unsharing step in bcmgenet_add_tsb() is keyed off available
headroom, not off ownership of the head:

	if (unlikely(skb_headroom(skb) < sizeof(*status))) {
		new_skb = skb_realloc_headroom(skb, sizeof(*status));
		...
	}

	skb_push(skb, sizeof(*status));
	status = (struct status_64 *)skb->data;
	memset(status, 0, sizeof(*status));

A cloned skb that already has 64 bytes of headroom skips the realloc
entirely, and that is the common case because bcmgenet_probe() does:

	dev->needed_headroom += 64;

bcmgenet_xmit() calls bcmgenet_add_tsb() without any copy-on-write of its
own, so the memset() lands in memory shared with the other clone.

Clones of one head do get transmitted on two devices concurrently, for
example bond_xmit_broadcast():

	skb2 = skb_clone(skb, GFP_ATOMIC);

and tcf_mirred_to_dev():

	skb_to_send = skb_clone(skb, GFP_ATOMIC);

If the second device's transmit path pushes its own header (VLAN tag, DSA
tag, tunnel or USB-ethernet header) into the same headroom bytes, do the
two writes overlap and corrupt one of the frames?

Before this patch the driver wrote 4 bytes (tx_csum_info) and only for
CHECKSUM_PARTIAL packets; the memset() extends that to all 64 bytes for
every packet, which widens the overlap considerably.

Would skb_cow_head(skb, sizeof(*status)) before the skb_push(), with the
allocation failure handled like the existing skb_realloc_headroom() path,
be preferable here? That is what the sibling driver does in
bcmasp_csum_offload():

	ret = skb_cow_head(skb, sizeof(*offload));
	if (ret < 0) {
		intf->mib.tx_realloc_offload_failed++;
		goto help;
	}

The missing copy-on-write predates this patch and the same pattern exists
in bcm_sysport_insert_tsb(), so it is not newly introduced, but the larger
write makes it more visible.

>  
>  	if (skb->ip_summed  == CHECKSUM_PARTIAL) {
>  		ip_ver = skb->protocol;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com

Re: [PATCH net 8/8] net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Submission metadata is not upstream-acceptable: the commit has no
  Signed-off-by line (mandatory DCO certification per…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patched
  conversion line makes it visible: bcmgenet_get_coalesce() can report…

--

net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT

This patch makes bcmgenet_get_coalesce() mask the DMA_RING0_TIMEOUT
register read with DMA_TIMEOUT_MASK before converting the tick count
into microseconds, so bits outside the 16-bit timeout field no longer
affect the rx_coalesce_usecs value reported to userspace.

Two observations below, one on the commit trailers and one on the
get/set round trip through this conversion.

[Severity: Low]
Looking at the trailers in the commit message:

    Fixes: 4a29645bfe6c ("net: bcmgenet: Implement RX coalescing control knobs")
    Assisted-by: LLM
    Co-authored-by: Cursor <cursoragent@cursor.com>
    Change-Id: I4c5b5019e764dbd0c7e51bd8c7365939a4736e44

There is no Signed-off-by line, which
Documentation/process/submitting-patches.rst requires for the DCO
certification.

Should the Gerrit "Change-Id:" trailer and the
"Co-authored-by: Cursor <cursoragent@cursor.com>" trailer be dropped as
well, given a tool account cannot certify the DCO?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 3a6a2f0759598..b15e64a484b43 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -852,7 +852,8 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
>  	ec->rx_max_coalesced_frames =
>  		bcmgenet_rdma_ring_readl(priv, 0, DMA_MBUF_DONE_THRESH);
>  	ec->rx_coalesce_usecs =
> -		bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT) * 8192 / 1000;
> +		(bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT) &
> +		 DMA_TIMEOUT_MASK) * 8192 / 1000;

[Severity: Medium]
This isn't a bug introduced by this patch, but since the conversion on
this line is being touched: can the value reported here be rejected by
bcmgenet_set_coalesce()?

bcmgenet_set_coalesce() accepts up to (DMA_TIMEOUT_MASK * 8) + 1, i.e.
524281 us:

	if (ec->tx_max_coalesced_frames > DMA_INTR_THRESHOLD_MASK ||
	    ec->tx_max_coalesced_frames == 0 ||
	    ec->rx_max_coalesced_frames > DMA_INTR_THRESHOLD_MASK ||
	    ec->rx_coalesce_usecs > (DMA_TIMEOUT_MASK * 8) + 1)
		return -EINVAL;

bcmgenet_set_rx_coalesce() then rounds that up:

	reg |= DIV_ROUND_UP(usecs * 1000, 8192);

8192 * 63999 = 524279808, which is below 524281000, so 524281 us is
programmed as 64000 ticks.

Reading it back through the patched conversion gives
64000 * 8192 / 1000 = 524288, which is larger than the 524281 the
setter accepts.

Since the ethtool utility does ETHTOOL_GCOALESCE, modifies one field and
then issues ETHTOOL_SCOALESCE with the full structure, after
"ethtool -C eth0 rx-usecs 524281" a later "ethtool -C eth0 rx-frames N"
would return -EINVAL until rx-usecs is lowered.

Would it make sense to derive the range check in
bcmgenet_set_coalesce() from the same tick conversion and rounding used
by bcmgenet_set_rx_coalesce() and this getter, so the round trip is
consistent?

>  
>  	for (i = 0; i <= priv->hw_params->rx_queues; i++) {
>  		ring = &priv->rx_rings[i];

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com