[PATCH net 00/12] net: systemport: Collection of fixes

[PATCH net 00/12] net: systemport: Collection of fixes

[Florian Fainelli]

This patch series contains a collection of fixes accumulated during a
LLM session.

Florian Fainelli (12):
  net: systemport: Fix buffer overflow in bcm_sysport_get_stats()
  net: systemport: Fix invalid dev_id argument in
    bcm_sysport_poll_controller()
  net: systemport: Fix NULL pointer dereference in
    bcm_sysport_fini_rx_ring()
  net: systemport: Fix missing phy-handle parsing for non-fixed PHYs
  net: systemport: Fix Wake-on-LAN RXCHK filter enable loop
  net: systemport: Fix RUNT MIB counter register offset calculation
  net: systemport: Fix potential packet length underflow in
    bcm_sysport_desc_rx()
  net: systemport: Fix out-of-bounds array accesses in DSA queue mapping
  net: systemport: Fix inverted error messages in bcm_sysport_stop()
  net: systemport: Fix unbalanced Wake-on-LAN clock disable in resume
  net: systemport: Update TDMA queue mapping dynamically on changeupper
  net: systemport: Complete resource teardown even on DMA disable
    timeout

 drivers/net/ethernet/broadcom/bcmsysport.c | 89 ++++++++++++++--------
 1 file changed, 58 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH net 01/12] net: systemport: Fix buffer overflow in bcm_sysport_get_stats()

[Florian Fainelli]

When running on SYSTEMPORT Lite, certain statistics are unsupported and
skipped during bcm_sysport_get_stats(). The variable 'j' tracks the
compacted index into the destination data buffer, whereas 'i' iterates
over all elements in bcm_sysport_gstrings_stats.

Because the buffer allocated by ethtool is sized only according to
bcm_sysport_get_sset_count(), storing values at data[i] instead of
data[j] writes past the allocated array bounds, leading to memory
corruption.

Fix this by writing to data[j] instead of data[i].

Fixes: 10377ba7673d ("net: systemport: Support 64bit statistics")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 4d06c6ba6641..db627cd15fb7 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -482,10 +482,10 @@ static void bcm_sysport_get_stats(struct net_device *dev,
 		    s->type == BCM_SYSPORT_STAT_NETDEV64) {
 			do {
 				start = u64_stats_fetch_begin(syncp);
-				data[i] = *(u64 *)p;
+				data[j] = *(u64 *)p;
 			} while (u64_stats_fetch_retry(syncp, start));
 		} else
-			data[i] = *(u32 *)p;
+			data[j] = *(u32 *)p;
 		j++;
 	}
 
-- 
2.34.1

Re: [PATCH net 01/12] net: systemport: Fix buffer overflow in bcm_sysport_get_stats()

[Nicolai Buchwitz]

On 22.9.2026 01:12, Florian Fainelli wrote:
> When running on SYSTEMPORT Lite, certain statistics are unsupported and
> skipped during bcm_sysport_get_stats(). The variable 'j' tracks the
> compacted index into the destination data buffer, whereas 'i' iterates
> over all elements in bcm_sysport_gstrings_stats.
> 
> Because the buffer allocated by ethtool is sized only according to
> bcm_sysport_get_sset_count(), storing values at data[i] instead of
> data[j] writes past the allocated array bounds, leading to memory
> corruption.
> 
> Fix this by writing to data[j] instead of data[i].
> 
> Fixes: 10377ba7673d ("net: systemport: Support 64bit statistics")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 4d06c6ba6641..db627cd15fb7 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -482,10 +482,10 @@ static void bcm_sysport_get_stats(struct 
> net_device *dev,
>  		    s->type == BCM_SYSPORT_STAT_NETDEV64) {
>  			do {
>  				start = u64_stats_fetch_begin(syncp);
> -				data[i] = *(u64 *)p;
> +				data[j] = *(u64 *)p;
>  			} while (u64_stats_fetch_retry(syncp, start));
>  		} else
> -			data[i] = *(u32 *)p;
> +			data[j] = *(u32 *)p;
>  		j++;
>  	}

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

Thanks,
Nicolai

[PATCH net 02/12] net: systemport: Fix invalid dev_id argument in bcm_sysport_poll_controller()

[Florian Fainelli]

Both bcm_sysport_rx_isr() and bcm_sysport_tx_isr() expect their second
argument (dev_id) to be a 'struct net_device *dev', as they call
netdev_priv(dev) to retrieve the private data structure.

bcm_sysport_poll_controller() was passing 'priv' instead of 'dev',
causing netdev_priv() inside the ISRs to treat 'priv' as a net_device,
leading to out-of-bounds pointer calculations and crashes when netconsole
or netpoll is triggered.

Fix this by passing 'dev' instead of 'priv' to the ISRs.

Fixes: 6cec4f5e00a3 ("net: systemport: Add netconsole support")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index db627cd15fb7..8328fe824d15 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1198,12 +1198,12 @@ static void bcm_sysport_poll_controller(struct net_device *dev)
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
 
 	disable_irq(priv->irq0);
-	bcm_sysport_rx_isr(priv->irq0, priv);
+	bcm_sysport_rx_isr(priv->irq0, dev);
 	enable_irq(priv->irq0);
 
 	if (!priv->is_lite) {
 		disable_irq(priv->irq1);
-		bcm_sysport_tx_isr(priv->irq1, priv);
+		bcm_sysport_tx_isr(priv->irq1, dev);
 		enable_irq(priv->irq1);
 	}
 }
-- 
2.34.1

Re: [PATCH net 02/12] net: systemport: Fix invalid dev_id argument in bcm_sysport_poll_controller()

[Nicolai Buchwitz]

On 22.9.2026 01:12, Florian Fainelli wrote:
> Both bcm_sysport_rx_isr() and bcm_sysport_tx_isr() expect their second
> argument (dev_id) to be a 'struct net_device *dev', as they call
> netdev_priv(dev) to retrieve the private data structure.
> 
> bcm_sysport_poll_controller() was passing 'priv' instead of 'dev',
> causing netdev_priv() inside the ISRs to treat 'priv' as a net_device,
> leading to out-of-bounds pointer calculations and crashes when 
> netconsole
> or netpoll is triggered.
> 
> Fix this by passing 'dev' instead of 'priv' to the ISRs.
> 
> Fixes: 6cec4f5e00a3 ("net: systemport: Add netconsole support")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index db627cd15fb7..8328fe824d15 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -1198,12 +1198,12 @@ static void bcm_sysport_poll_controller(struct 
> net_device *dev)
>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
> 
>  	disable_irq(priv->irq0);
> -	bcm_sysport_rx_isr(priv->irq0, priv);
> +	bcm_sysport_rx_isr(priv->irq0, dev);
>  	enable_irq(priv->irq0);
> 
>  	if (!priv->is_lite) {
>  		disable_irq(priv->irq1);
> -		bcm_sysport_tx_isr(priv->irq1, priv);
> +		bcm_sysport_tx_isr(priv->irq1, dev);
>  		enable_irq(priv->irq1);
>  	}
>  }

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

Thanks,
Nicolai

Re: [PATCH net 02/12] net: systemport: Fix invalid dev_id argument in bcm_sysport_poll_controller()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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).

- [High] bcm_sysport_poll_controller()
  (drivers/net/ethernet/broadcom/bcmsysport.c:1199-1211, post-patch)…
- [Medium] With the dev_id argument fixed, bcm_sysport_poll_controller()
  now performs real MMIO against the SYSTEMPORT INTRL2 block from…

--

net: systemport: Fix invalid dev_id argument in bcm_sysport_poll_controller()

This corrects bcm_sysport_poll_controller() so that it hands the
struct net_device pointer, rather than the driver private pointer, to
bcm_sysport_rx_isr() and bcm_sysport_tx_isr(), both of which call
netdev_priv() on their dev_id argument.

A couple of questions about the surrounding code in that callback below.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index db627cd15fb7a..8328fe824d152 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -1198,12 +1198,12 @@ static void bcm_sysport_poll_controller(struct net_device *dev)
>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>  
>  	disable_irq(priv->irq0);
> -	bcm_sysport_rx_isr(priv->irq0, priv);
> +	bcm_sysport_rx_isr(priv->irq0, dev);
>  	enable_irq(priv->irq0);

[Severity: High]
Can disable_irq() be called from this callback at all?  ndo_poll_controller
is invoked by netpoll with hard IRQs disabled and under rcu_read_lock():

net/core/netpoll.c:netpoll_send_skb() {
	local_irq_save(flags);
	...__netpoll_send_skb(np, skb);
}

/* call with IRQ disabled */
static netdev_tx_t __netpoll_send_skb(...)
{
	...
	lockdep_assert_irqs_disabled();
	...
			/* tickle device maybe there is some cleanup */
			netpoll_poll_dev(np->dev);
	...
		WARN_ONCE(!irqs_disabled(), ...);
}

and netpoll_poll_dev() then does:

	ops = dev->netdev_ops;
	if (ops->ndo_poll_controller)
		ops->ndo_poll_controller(dev);

	poll_napi(dev);

while disable_irq() is a sleeping API:

kernel/irq/manage.c:disable_irq() {
	might_sleep();
	if (!__disable_irq_nosync(irq))
		synchronize_irq(irq);
}

Does this produce a "sleeping function called from invalid context" splat with
CONFIG_DEBUG_ATOMIC_SLEEP on every netpoll poll of a SYSTEMPORT interface?

There also looks to be a hang case.  If the netconsole message being
transmitted originates from inside the handler itself, for example:

	if (unlikely(priv->irq0_stat == 0)) {
		netdev_warn(priv->netdev, "spurious RX interrupt\n");
		return IRQ_NONE;
	}

then irq0 is IRQD_IRQ_INPROGRESS on this CPU when synchronize_irq() runs:

	while (irqd_irq_inprogress(&desc->irq_data))
		cpu_relax();
...
	wait_event(desc->wait_for_threads, !atomic_read(&desc->threads_active));

With local IRQs disabled the in-progress handler can never complete, so does
this spin forever?  And with force-threaded handlers (threadirqs or
PREEMPT_RT), does the wait_event() schedule while atomic?

Given that netpoll_poll_dev() already calls poll_napi() and drives the
driver's NAPI instances, would removing ndo_poll_controller from
bcm_sysport_netdev_ops be preferable to fixing the argument?  At a minimum,
would disable_irq_nosync() be more appropriate here?

>  
>  	if (!priv->is_lite) {
>  		disable_irq(priv->irq1);
> -		bcm_sysport_tx_isr(priv->irq1, priv);
> +		bcm_sysport_tx_isr(priv->irq1, dev);
>  		enable_irq(priv->irq1);
>  	}
>  }

[Severity: Medium]
Now that the register base is correct, these hand-calls perform real MMIO
against the INTRL2 block from netpoll context.  Both handlers read the status
latch and acknowledge it:

bcm_sysport_rx_isr() {
	priv->irq0_stat = intrl2_0_readl(priv, INTRL2_CPU_STATUS) &
			  ~intrl2_0_readl(priv, INTRL2_CPU_MASK_STATUS);
	intrl2_0_writel(priv, priv->irq0_stat, INTRL2_CPU_CLEAR);

	if (unlikely(priv->irq0_stat == 0)) {
		netdev_warn(priv->netdev, "spurious RX interrupt\n");
		return IRQ_NONE;
	}
}

bcm_sysport_tx_isr() {
	intrl2_1_writel(priv, 0xffffffff, INTRL2_CPU_CLEAR);

	if (unlikely(priv->irq1_stat == 0)) {
		netdev_warn(priv->netdev, "spurious TX interrupt\n");
		return IRQ_NONE;
	}
}

When netpoll is only tickling a stopped TX queue there is typically nothing
latched, so does this emit "spurious RX interrupt" / "spurious TX interrupt"
from inside the netconsole transmit path, feeding those warnings back into the
same path?

A second concern, which I could not confirm from the tree since it depends on
the brcmstb interrupt controller latching semantics: events consumed and
cleared here happen outside hardirq context, so if a parent-level request is
still pending after enable_irq(), the real handler would run against an
already-cleared status latch, warn and return IRQ_NONE.  Could repeated
IRQ_NONE returns be accounted by note_interrupt() and eventually escalate to
"nobody cared" disabling of the SYSTEMPORT line?

Deleting the callback and relying on poll_napi() would avoid both of these.

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

[PATCH net 03/12] net: systemport: Fix NULL pointer dereference in bcm_sysport_fini_rx_ring()

[Florian Fainelli]

If allocation of priv->rx_cbs fails during bcm_sysport_init_rx_ring(),
error unwinding in bcm_sysport_open() calls bcm_sysport_fini_rx_ring().
Without checking if priv->rx_cbs is non-NULL, bcm_sysport_fini_rx_ring()
dereferences priv->rx_cbs, resulting in a NULL pointer dereference.

Add a check for !priv->rx_cbs at the beginning of
bcm_sysport_fini_rx_ring(), matching bcm_sysport_fini_tx_ring().

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 8328fe824d15..b91a57540f55 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1717,6 +1717,9 @@ static void bcm_sysport_fini_rx_ring(struct bcm_sysport_priv *priv)
 	if (!(reg & RDMA_DISABLED))
 		netdev_warn(priv->netdev, "RDMA not stopped!\n");
 
+	if (!priv->rx_cbs)
+		return;
+
 	for (i = 0; i < priv->num_rx_bds; i++) {
 		cb = &priv->rx_cbs[i];
 		if (dma_unmap_addr(cb, dma_addr))
-- 
2.34.1

Re: [PATCH net 03/12] net: systemport: Fix NULL pointer dereference in bcm_sysport_fini_rx_ring()

[Nicolai Buchwitz]

On 22.9.2026 01:12, Florian Fainelli wrote:
> If allocation of priv->rx_cbs fails during bcm_sysport_init_rx_ring(),
> error unwinding in bcm_sysport_open() calls bcm_sysport_fini_rx_ring().
> Without checking if priv->rx_cbs is non-NULL, 
> bcm_sysport_fini_rx_ring()
> dereferences priv->rx_cbs, resulting in a NULL pointer dereference.
> 
> Add a check for !priv->rx_cbs at the beginning of
> bcm_sysport_fini_rx_ring(), matching bcm_sysport_fini_tx_ring().
> 
> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet 
> MAC driver")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 8328fe824d15..b91a57540f55 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -1717,6 +1717,9 @@ static void bcm_sysport_fini_rx_ring(struct 
> bcm_sysport_priv *priv)
>  	if (!(reg & RDMA_DISABLED))
>  		netdev_warn(priv->netdev, "RDMA not stopped!\n");
> 
> +	if (!priv->rx_cbs)
> +		return;
> +
>  	for (i = 0; i < priv->num_rx_bds; i++) {
>  		cb = &priv->rx_cbs[i];
>  		if (dma_unmap_addr(cb, dma_addr))

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

Thanks,
Nicolai

[PATCH net 04/12] net: systemport: Fix missing phy-handle parsing for non-fixed PHYs

[Florian Fainelli]

In bcm_sysport_probe(), priv->phy_dn is only initialized if the device
tree node has a fixed-link configuration (of_phy_is_fixed_link). When
connecting to a discrete MDIO-attached PHY referenced via 'phy-handle',
priv->phy_dn remains NULL. This causes of_phy_connect() during
bcm_sysport_open() to fail with -ENODEV since of_phy_find_device(NULL)
returns NULL.

Fix this by parsing 'phy-handle' via of_parse_phandle() and falling back
to of_phy_is_fixed_link(). Ensure proper of_node_get() and of_node_put()
refcounting lifecycle on both error unwinding and module remove paths.

Fixes: 186534a3f832 ("net: systemport: use the new fixed PHY helpers")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index b91a57540f55..78b96b192185 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2527,17 +2527,19 @@ static int bcm_sysport_probe(struct platform_device *pdev)
 	if (ret)
 		priv->phy_interface = PHY_INTERFACE_MODE_GMII;
 
+	priv->phy_dn = of_parse_phandle(dn, "phy-handle", 0);
+
 	/* In the case of a fixed PHY, the DT node associated
 	 * to the PHY is the Ethernet MAC DT node.
 	 */
-	if (of_phy_is_fixed_link(dn)) {
+	if (!priv->phy_dn && of_phy_is_fixed_link(dn)) {
 		ret = of_phy_register_fixed_link(dn);
 		if (ret) {
 			dev_err(&pdev->dev, "failed to register fixed PHY\n");
 			goto err_free_netdev;
 		}
 
-		priv->phy_dn = dn;
+		priv->phy_dn = of_node_get(dn);
 	}
 
 	/* Initialize netdevice members */
@@ -2622,6 +2624,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
 err_deregister_fixed_link:
 	if (of_phy_is_fixed_link(dn))
 		of_phy_deregister_fixed_link(dn);
+	of_node_put(priv->phy_dn);
 err_free_netdev:
 	free_netdev(dev);
 	return ret;
@@ -2640,6 +2643,7 @@ static void bcm_sysport_remove(struct platform_device *pdev)
 	unregister_netdev(dev);
 	if (of_phy_is_fixed_link(dn))
 		of_phy_deregister_fixed_link(dn);
+	of_node_put(priv->phy_dn);
 	free_netdev(dev);
 	dev_set_drvdata(&pdev->dev, NULL);
 }
-- 
2.34.1

Re: [PATCH net 04/12] net: systemport: Fix missing phy-handle parsing for non-fixed PHYs

[Nicolai Buchwitz]

Hi Florian

On 22.9.2026 01:12, Florian Fainelli wrote:
> In bcm_sysport_probe(), priv->phy_dn is only initialized if the device
> tree node has a fixed-link configuration (of_phy_is_fixed_link). When
> connecting to a discrete MDIO-attached PHY referenced via 'phy-handle',
> priv->phy_dn remains NULL. This causes of_phy_connect() during
> bcm_sysport_open() to fail with -ENODEV since of_phy_find_device(NULL)
> returns NULL.
> 
> Fix this by parsing 'phy-handle' via of_parse_phandle() and falling 
> back
> to of_phy_is_fixed_link(). Ensure proper of_node_get() and 
> of_node_put()
> refcounting lifecycle on both error unwinding and module remove paths.
> 
> Fixes: 186534a3f832 ("net: systemport: use the new fixed PHY helpers")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---

> [...]

Not sure this qualifies as a fix, the driver never parsed phy-handle. 
Maybe net-next material?

Regards
Nicolai

Re: [PATCH net 04/12] net: systemport: Fix missing phy-handle parsing for non-fixed PHYs

[Florian Fainelli]

On 9/22/26 01:50, Nicolai Buchwitz wrote:
> Hi Florian
> 
> On 22.9.2026 01:12, Florian Fainelli wrote:
>> In bcm_sysport_probe(), priv->phy_dn is only initialized if the device
>> tree node has a fixed-link configuration (of_phy_is_fixed_link). When
>> connecting to a discrete MDIO-attached PHY referenced via 'phy-handle',
>> priv->phy_dn remains NULL. This causes of_phy_connect() during
>> bcm_sysport_open() to fail with -ENODEV since of_phy_find_device(NULL)
>> returns NULL.
>>
>> Fix this by parsing 'phy-handle' via of_parse_phandle() and falling back
>> to of_phy_is_fixed_link(). Ensure proper of_node_get() and of_node_put()
>> refcounting lifecycle on both error unwinding and module remove paths.
>>
>> Fixes: 186534a3f832 ("net: systemport: use the new fixed PHY helpers")
>> Assisted-by: LLM
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
> 
>> [...]
> 
> Not sure this qualifies as a fix, the driver never parsed phy-handle. 
> Maybe net-next material?

Good point, we've only ever used fixed-link so this is clearly not a bug 
fix.
-- 
Florian

[PATCH net 05/12] net: systemport: Fix Wake-on-LAN RXCHK filter enable loop

[Florian Fainelli]

In bcm_sysport_suspend_to_wol(), the loop enabling programmed RXCHK
filters in RXCHK_CONTROL used an auxiliary counter 'i' instead of the
actual set filter index 'index'.

When non-contiguous filters were configured (for example, if filter 0
was deleted and filter 1 remained), the code would enable bit
(RXCHK_BRCM_TAG_MATCH_SHIFT + 0) corresponding to filter 0 rather than
filter 1, causing Wake-on-LAN filter matching to fail.

Fix this by using the filter 'index' to set the appropriate match bit
in RXCHK_CONTROL.

Fixes: bb9051a2b230 ("net: systemport: Add support for WAKE_FILTER")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 78b96b192185..7f2e5e4efb8d 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2652,7 +2652,7 @@ static int bcm_sysport_suspend_to_wol(struct bcm_sysport_priv *priv)
 {
 	struct net_device *ndev = priv->netdev;
 	unsigned int timeout = 1000;
-	unsigned int index, i = 0;
+	unsigned int index;
 	u32 reg;
 
 	reg = umac_readl(priv, UMAC_MPD_CTRL);
@@ -2682,10 +2682,8 @@ static int bcm_sysport_suspend_to_wol(struct bcm_sysport_priv *priv)
 		reg = rxchk_readl(priv, RXCHK_CONTROL);
 		reg &= ~(RXCHK_BRCM_TAG_MATCH_MASK <<
 			 RXCHK_BRCM_TAG_MATCH_SHIFT);
-		for_each_set_bit(index, priv->filters, RXCHK_BRCM_TAG_MAX) {
-			reg |= BIT(RXCHK_BRCM_TAG_MATCH_SHIFT + i);
-			i++;
-		}
+		for_each_set_bit(index, priv->filters, RXCHK_BRCM_TAG_MAX)
+			reg |= BIT(RXCHK_BRCM_TAG_MATCH_SHIFT + index);
 		reg |= RXCHK_EN | RXCHK_BRCM_TAG_EN;
 		rxchk_writel(priv, reg, RXCHK_CONTROL);
 	}
-- 
2.34.1

Re: [PATCH net 05/12] net: systemport: Fix Wake-on-LAN RXCHK filter enable loop

[Nicolai Buchwitz]

On 22.9.2026 01:12, Florian Fainelli wrote:
> In bcm_sysport_suspend_to_wol(), the loop enabling programmed RXCHK
> filters in RXCHK_CONTROL used an auxiliary counter 'i' instead of the
> actual set filter index 'index'.
> 
> When non-contiguous filters were configured (for example, if filter 0
> was deleted and filter 1 remained), the code would enable bit
> (RXCHK_BRCM_TAG_MATCH_SHIFT + 0) corresponding to filter 0 rather than
> filter 1, causing Wake-on-LAN filter matching to fail.
> 
> Fix this by using the filter 'index' to set the appropriate match bit
> in RXCHK_CONTROL.
> 
> Fixes: bb9051a2b230 ("net: systemport: Add support for WAKE_FILTER")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 78b96b192185..7f2e5e4efb8d 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2652,7 +2652,7 @@ static int bcm_sysport_suspend_to_wol(struct 
> bcm_sysport_priv *priv)
>  {
>  	struct net_device *ndev = priv->netdev;
>  	unsigned int timeout = 1000;
> -	unsigned int index, i = 0;
> +	unsigned int index;
>  	u32 reg;
> 
>  	reg = umac_readl(priv, UMAC_MPD_CTRL);
> @@ -2682,10 +2682,8 @@ static int bcm_sysport_suspend_to_wol(struct 
> bcm_sysport_priv *priv)
>  		reg = rxchk_readl(priv, RXCHK_CONTROL);
>  		reg &= ~(RXCHK_BRCM_TAG_MATCH_MASK <<
>  			 RXCHK_BRCM_TAG_MATCH_SHIFT);
> -		for_each_set_bit(index, priv->filters, RXCHK_BRCM_TAG_MAX) {
> -			reg |= BIT(RXCHK_BRCM_TAG_MATCH_SHIFT + i);
> -			i++;
> -		}
> +		for_each_set_bit(index, priv->filters, RXCHK_BRCM_TAG_MAX)
> +			reg |= BIT(RXCHK_BRCM_TAG_MATCH_SHIFT + index);
>  		reg |= RXCHK_EN | RXCHK_BRCM_TAG_EN;
>  		rxchk_writel(priv, reg, RXCHK_CONTROL);
>  	}

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

Thanks,
Nicolai

[PATCH net 06/12] net: systemport: Fix RUNT MIB counter register offset calculation

[Florian Fainelli]

In UniMAC hardware, there is a 0xC byte gap between the RX MIB counters
and the TX MIB counters, and a second 0xC byte gap between the TX MIB
counters and the RX RUNT MIB counters.

In bcm_sysport_update_mib_counters(), 'offset' was only set to
UMAC_MIB_STAT_OFFSET (0xC) for all non-RX counters, omitting the second
0xC gap for BCM_SYSPORT_STAT_RUNT counters. As a result, all 4 RUNT MIB
counters were read from unmapped gap register space.

Fix this by setting offset to 2 * UMAC_MIB_STAT_OFFSET (0x18) when
reading BCM_SYSPORT_STAT_RUNT counters.

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 7f2e5e4efb8d..b69833d34bd5 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -391,8 +391,10 @@ static void bcm_sysport_update_mib_counters(struct bcm_sysport_priv *priv)
 			if (priv->is_lite)
 				continue;
 
-			if (s->type != BCM_SYSPORT_STAT_MIB_RX)
+			if (s->type == BCM_SYSPORT_STAT_MIB_TX)
 				offset = UMAC_MIB_STAT_OFFSET;
+			else if (s->type == BCM_SYSPORT_STAT_RUNT)
+				offset = 2 * UMAC_MIB_STAT_OFFSET;
 			val = umac_readl(priv, UMAC_MIB_START + j + offset);
 			break;
 		case BCM_SYSPORT_STAT_RXCHK:
-- 
2.34.1

Re: [PATCH net 06/12] net: systemport: Fix RUNT MIB counter register offset calculation

[Nicolai Buchwitz]

On 22.9.2026 01:12, Florian Fainelli wrote:
> In UniMAC hardware, there is a 0xC byte gap between the RX MIB counters
> and the TX MIB counters, and a second 0xC byte gap between the TX MIB
> counters and the RX RUNT MIB counters.
> 
> In bcm_sysport_update_mib_counters(), 'offset' was only set to
> UMAC_MIB_STAT_OFFSET (0xC) for all non-RX counters, omitting the second
> 0xC gap for BCM_SYSPORT_STAT_RUNT counters. As a result, all 4 RUNT MIB
> counters were read from unmapped gap register space.
> 
> Fix this by setting offset to 2 * UMAC_MIB_STAT_OFFSET (0x18) when
> reading BCM_SYSPORT_STAT_RUNT counters.
> 
> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet 
> MAC driver")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 7f2e5e4efb8d..b69833d34bd5 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -391,8 +391,10 @@ static void bcm_sysport_update_mib_counters(struct 
> bcm_sysport_priv *priv)
>  			if (priv->is_lite)
>  				continue;
> 
> -			if (s->type != BCM_SYSPORT_STAT_MIB_RX)
> +			if (s->type == BCM_SYSPORT_STAT_MIB_TX)
>  				offset = UMAC_MIB_STAT_OFFSET;
> +			else if (s->type == BCM_SYSPORT_STAT_RUNT)
> +				offset = 2 * UMAC_MIB_STAT_OFFSET;
>  			val = umac_readl(priv, UMAC_MIB_START + j + offset);
>  			break;
>  		case BCM_SYSPORT_STAT_RXCHK

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

Thanks,
Nicolai

Re: [PATCH net 06/12] net: systemport: Fix RUNT MIB counter register offset calculation

[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] Commit message inaccurately describes the pre-fix behaviour of
  bcm_sysport_update_mib_counters()…

--

net: systemport: Fix RUNT MIB counter register offset calculation

This adds the second 0xC UniMAC MIB gap to the register offset used when
reading the BCM_SYSPORT_STAT_RUNT counters in
bcm_sysport_update_mib_counters(), so offset becomes
2 * UMAC_MIB_STAT_OFFSET for those four entries.  The register offsets
produced by the new code look correct to me.

> In bcm_sysport_update_mib_counters(), 'offset' was only set to
> UMAC_MIB_STAT_OFFSET (0xC) for all non-RX counters, omitting the second
> 0xC gap for BCM_SYSPORT_STAT_RUNT counters. As a result, all 4 RUNT MIB
> counters were read from unmapped gap register space.

[Severity: Low]
Is "all 4 RUNT MIB counters were read from unmapped gap register space"
accurate?  By the driver's own table it looks like only three of the four
reads landed in the reserved gap.

bcm_sysport_gstrings_stats[] has 29 STAT_MIB_RX entries and 29
STAT_MIB_TX entries, all u32, so 'j' is 29 * 4 + 29 * 4 = 0xE8 by the time
the first STAT_RUNT entry is processed.  With the old offset of 0xC the
four reads were:

    UMAC_MIB_START(0x400) + 0xE8 + 0xC = 0x4F4, 0x4F8, 0x4FC, 0x500

The annotations in bcmsysport.h put mib.rx.rcrc at 0x470 and mib.tx.uc at
0x4F0, which places the second 0xC gap at 0x4F4-0x4FF and the start of the
RUNT block at 0x500.  So 0x4F4/0x4F8/0x4FC were gap space, but 0x500 was
the real first RUNT register.

That last read was stored into the fourth STAT_RUNT destination:

	STAT_RUNT("rx_runt_pkts", mib.rx_runt_cnt),
	STAT_RUNT("rx_runt_valid_fcs", mib.rx_runt_fcs),
	STAT_RUNT("rx_runt_inval_fcs_align", mib.rx_runt_fcs_align),
	STAT_RUNT("rx_runt_bytes", mib.rx_runt_bytes),

so rx_runt_bytes reported the runt packet count rather than gap data.

Could the changelog also state the user-visible symptom, i.e. that
ethtool -S reported bogus values for rx_runt_pkts, rx_runt_valid_fcs and
rx_runt_inval_fcs_align, and a mis-assigned value for rx_runt_bytes?  That
would help anyone deciding on a stable backport.

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

[PATCH net 07/12] net: systemport: Fix potential packet length underflow in bcm_sysport_desc_rx()

[Florian Fainelli]

In bcm_sysport_desc_rx(), the packet length 'len' extracted from the RSB
is only validated against RX_BUF_LENGTH. If a malformed or corrupted
frame is received with 'len' smaller than the prepended Receive Status
Block (sizeof(*rsb)) plus 2 padding bytes (and optional FCS), calling
skb_pull() will trigger a BUG() in __skb_pull() when pulling beyond
skb->len. Furthermore, subtracting (sizeof(*rsb) + 2) from 'len' (u16)
will underflow, resulting in corrupted packet stats and potential
out-of-bounds operations.

Fix this by ensuring 'len' is at least sizeof(*rsb) + 2 (plus
ETH_FCS_LEN if CRC forward is enabled) before proceeding.

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index b69833d34bd5..95cead1df160 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -781,8 +781,9 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
 			  p_index, priv->rx_c_index, priv->rx_read_ptr,
 			  len, status);
 
-		if (unlikely(len > RX_BUF_LENGTH)) {
-			netif_err(priv, rx_status, ndev, "oversized packet\n");
+		if (unlikely(len > RX_BUF_LENGTH ||
+			     len < sizeof(*rsb) + 2 + (priv->crc_fwd ? ETH_FCS_LEN : 0))) {
+			netif_err(priv, rx_status, ndev, "invalid packet size: %d\n", len);
 			ndev->stats.rx_length_errors++;
 			ndev->stats.rx_errors++;
 			dev_kfree_skb_any(skb);
-- 
2.34.1

Re: [PATCH net 07/12] net: systemport: Fix potential packet length underflow in bcm_sysport_desc_rx()

[Nicolai Buchwitz]

Hi Florian

On 22.9.2026 01:13, Florian Fainelli wrote:
> In bcm_sysport_desc_rx(), the packet length 'len' extracted from the 
> RSB
> is only validated against RX_BUF_LENGTH. If a malformed or corrupted
> frame is received with 'len' smaller than the prepended Receive Status
> Block (sizeof(*rsb)) plus 2 padding bytes (and optional FCS), calling
> skb_pull() will trigger a BUG() in __skb_pull() when pulling beyond
> skb->len. Furthermore, subtracting (sizeof(*rsb) + 2) from 'len' (u16)
> will underflow, resulting in corrupted packet stats and potential
> out-of-bounds operations.

AFAIU skb_pull() returns NULL for len > skb->len and never reaches
__skb_pull(), so it doesn't reach BUG()?

I agree on the underflow and the wrong rx_bytes issue though.

> [...]

Thanks
Nicolai

Re: [PATCH net 07/12] net: systemport: Fix potential packet length underflow in bcm_sysport_desc_rx()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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 mischaracterizes the failure mode it claims
  to fix.
- [Medium] The new minimum-length test in bcm_sysport_desc_rx()
  (drivers/net/ethernet/broadcom/bcmsysport.c:784-785) only requires…

--

net: systemport: Fix potential packet length underflow in bcm_sysport_desc_rx()

The patch adds a lower bound to the RSB-reported packet length check in
bcm_sysport_desc_rx(), requiring len to be at least sizeof(*rsb) + 2,
plus ETH_FCS_LEN when CRC forwarding is enabled. Frames failing the new
check are counted as length errors and dropped.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index b69833d34bd5b..95cead1df1607 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -781,8 +781,9 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
>  			  p_index, priv->rx_c_index, priv->rx_read_ptr,
>  			  len, status);
>  
> -		if (unlikely(len > RX_BUF_LENGTH)) {
> -			netif_err(priv, rx_status, ndev, "oversized packet\n");
> +		if (unlikely(len > RX_BUF_LENGTH ||
> +			     len < sizeof(*rsb) + 2 + (priv->crc_fwd ? ETH_FCS_LEN : 0))) {

[Severity: Medium]
Should this lower bound also reserve room for the Ethernet header that the
same function consumes a few lines later?

With crc_fwd clear, any len in the range 10..23 passes the new test. The
code that follows then does:

drivers/net/ethernet/broadcom/bcmsysport.c:bcm_sysport_desc_rx() {
	...
	skb_put(skb, len);
	...
	skb_pull(skb, sizeof(*rsb) + 2);
	len -= (sizeof(*rsb) + 2);
	processed_bytes += len;

	/* UniMAC may forward CRC */
	if (priv->crc_fwd) {
		skb_trim(skb, len - ETH_FCS_LEN);
		len -= ETH_FCS_LEN;
	}

	skb->protocol = eth_type_trans(skb, ndev);
	...
}

For len == 12 the skb reaching eth_type_trans() has skb->len == 2. With
crc_fwd set and len == 14, skb_trim(skb, 0) leaves a zero-length skb.

In that case eth_type_trans() reads the MAC header without checking that
ETH_HLEN bytes are present:

net/ethernet/eth.c:eth_type_trans() {
	...
	eth = eth_skb_pull_mac(skb);
	eth_skb_pkt_type(skb, dev);
	...
}

include/linux/etherdevice.h:eth_skb_pull_mac() {
	struct ethhdr *eth = (struct ethhdr *)skb->data;

	skb_pull_inline(skb, ETH_HLEN);
	return eth;
}

skb_pull_inline() returns NULL for skb->len < ETH_HLEN and leaves
skb->data untouched, but the return value is discarded, so eth->h_dest
and eth->h_proto are read past skb->len. Those bytes are inside the
2048-byte RX_BUF_LENGTH allocation from __netdev_alloc_skb() in
bcm_sysport_rx_refill(), which is not zeroed and which DMA only filled
with len bytes.

Can this make skb->pkt_type and skb->protocol derive from stale buffer
contents, and leak the 2-byte protocol value to userspace as sll_protocol
for AF_PACKET SOCK_RAW readers in packet_recvmsg()?

The MAC header is also never stripped in this case, yet the frame is
counted in ndev->stats.rx_packets/rx_bytes and stats64 as a good
reception and handed to napi_gro_receive() with mac/network offsets that
upper layers will parse at the wrong offset.

Would sizeof(*rsb) + 2 + ETH_HLEN (or an ETH_ZLEN based bound), plus
ETH_FCS_LEN when crc_fwd is set, be the bound that actually covers the
parsing done in this function? The same incomplete pattern exists in
bcmgenet (len < GENET_RSB_PAD), so this may be intentional copy-over, but
nothing here establishes that sub-ETH_HLEN lengths cannot be reported.

> +			netif_err(priv, rx_status, ndev, "invalid packet size: %d\n", len);
>  			ndev->stats.rx_length_errors++;
>  			ndev->stats.rx_errors++;
>  			dev_kfree_skb_any(skb);

[Severity: Medium]
Is the failure mode described in the commit message accurate? The message
says:

    calling skb_pull() will trigger a BUG() in __skb_pull() when pulling
    beyond skb->len

skb_pull() in net/core/skbuff.c is a wrapper around skb_pull_inline():

include/linux/skbuff.h:skb_pull_inline() {
	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
}

So an over-long pull returns NULL without touching the skb and never
reaches __skb_pull(). The BUG() inside __skb_pull() is conditional:

include/linux/skbuff.h:__skb_pull() {
	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
		...
		BUG();
	}
	...
}

These RX skbs come from __netdev_alloc_skb(RX_BUF_LENGTH) in
bcm_sysport_rx_refill() and are purely linear, so skb->data_len is 0 and
that condition is unreachable.

The message also mentions "potential out-of-bounds operations". The
reachable pre-patch effects look like the u16 wrap in

	len -= (sizeof(*rsb) + 2);

which corrupts ndev->stats.rx_bytes, stats64->rx_bytes and
priv->dim.bytes (the latter feeding the DIM coalescing logic), plus an
skb passed to eth_type_trans()/napi_gro_receive() with the Receive Status
Block and 2 pad bytes still present because the ignored skb_pull() return
meant nothing was stripped.

Since this carries a Fixes: tag and will be considered for stable, could
the changelog describe the stat corruption and unstripped RSB instead of
a BUG() and out-of-bounds accesses?

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

[PATCH net 08/12] net: systemport: Fix out-of-bounds array accesses in DSA queue mapping

[Florian Fainelli]

The priv->ring_map array has a fixed size of (DSA_MAX_PORTS * 8). In
bcm_sysport_select_queue(), bcm_sysport_map_queues(), and
bcm_sysport_unmap_queues(), indices calculated as
(qp + port * num_tx_queues) were accessed without checking against
ARRAY_SIZE(priv->ring_map). If unusual port or queue configurations are
encountered, this could lead to out-of-bounds array accesses.

Additionally, on SYSTEMPORT Lite, netif_set_real_num_tx_queues() was
called with slave_dev->num_tx_queues / 2, which could evaluate to 0 if
slave_dev->num_tx_queues is 1, causing netif_set_real_num_tx_queues() to
fail with -EINVAL.

Fix these by clamping the real number of queues to at least 1 and adding
bounds checks on priv->ring_map.

Fixes: d156576362c0 ("net: systemport: Establish lower/upper queue mapping")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 95cead1df160..130545cce045 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2273,7 +2273,7 @@ static u16 bcm_sysport_select_queue(struct net_device *dev, struct sk_buff *skb,
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
 	u16 queue = skb_get_queue_mapping(skb);
 	struct bcm_sysport_tx_ring *tx_ring;
-	unsigned int q, port;
+	unsigned int q, port, index;
 
 	if (!netdev_uses_dsa(dev))
 		return netdev_pick_tx(dev, skb, NULL);
@@ -2281,8 +2281,11 @@ static u16 bcm_sysport_select_queue(struct net_device *dev, struct sk_buff *skb,
 	/* DSA tagging layer will have configured the correct queue */
 	q = BRCM_TAG_GET_QUEUE(queue);
 	port = BRCM_TAG_GET_PORT(queue);
-	tx_ring = priv->ring_map[q + port * priv->per_port_num_tx_queues];
+	index = q + port * priv->per_port_num_tx_queues;
+	if (unlikely(index >= ARRAY_SIZE(priv->ring_map)))
+		return netdev_pick_tx(dev, skb, NULL);
 
+	tx_ring = priv->ring_map[index];
 	if (unlikely(!tx_ring))
 		return netdev_pick_tx(dev, skb, NULL);
 
@@ -2329,7 +2332,8 @@ static int bcm_sysport_map_queues(struct net_device *dev,
 	 */
 	if (priv->is_lite)
 		netif_set_real_num_tx_queues(slave_dev,
-					     slave_dev->num_tx_queues / 2);
+					     max_t(unsigned int, 1,
+						   slave_dev->num_tx_queues / 2));
 
 	num_tx_queues = slave_dev->real_num_tx_queues;
 
@@ -2352,7 +2356,8 @@ static int bcm_sysport_map_queues(struct net_device *dev,
 		ring->switch_queue = qp;
 		ring->switch_port = port;
 		ring->inspect = true;
-		priv->ring_map[qp + port * num_tx_queues] = ring;
+		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
+			priv->ring_map[qp + port * num_tx_queues] = ring;
 		qp++;
 	}
 
@@ -2383,7 +2388,8 @@ static int bcm_sysport_unmap_queues(struct net_device *dev,
 
 		ring->inspect = false;
 		qp = ring->switch_queue;
-		priv->ring_map[qp + port * num_tx_queues] = NULL;
+		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
+			priv->ring_map[qp + port * num_tx_queues] = NULL;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH net 08/12] net: systemport: Fix out-of-bounds array accesses in DSA queue mapping

[Nicolai Buchwitz]

Hi Florian

On 22.9.2026 01:13, Florian Fainelli wrote:
> The priv->ring_map array has a fixed size of (DSA_MAX_PORTS * 8). In
> bcm_sysport_select_queue(), bcm_sysport_map_queues(), and
> bcm_sysport_unmap_queues(), indices calculated as
> (qp + port * num_tx_queues) were accessed without checking against
> ARRAY_SIZE(priv->ring_map). If unusual port or queue configurations are
> encountered, this could lead to out-of-bounds array accesses.
> 
> Additionally, on SYSTEMPORT Lite, netif_set_real_num_tx_queues() was
> called with slave_dev->num_tx_queues / 2, which could evaluate to 0 if
> slave_dev->num_tx_queues is 1, causing netif_set_real_num_tx_queues() 
> to
> fail with -EINVAL.
> 
> Fix these by clamping the real number of queues to at least 1 and 
> adding
> bounds checks on priv->ring_map.
> 
> Fixes: d156576362c0 ("net: systemport: Establish lower/upper queue 
> mapping")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 95cead1df160..130545cce045 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c

> [...]

> @@ -2352,7 +2356,8 @@ static int bcm_sysport_map_queues(struct 
> net_device *dev,
>  		ring->switch_queue = qp;
>  		ring->switch_port = port;
>  		ring->inspect = true;
> -		priv->ring_map[qp + port * num_tx_queues] = ring;
> +		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
> +			priv->ring_map[qp + port * num_tx_queues] = ring;

Out of range rings would still have inspect = true but no ring_map
entry, so select_queue() can never pick them.

Bail out before marking the ring?

> [...]

Thanks,
Nicolai

Re: [PATCH net 08/12] net: systemport: Fix out-of-bounds array accesses in DSA queue mapping

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (2) · 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] In bcm_sysport_map_queues()
  (drivers/net/ethernet/broadcom/bcmsysport.c:2356-2361) the new bounds…

Pre-existing issues:
- [Medium] bcm_sysport_unmap_queues()
  (drivers/net/ethernet/broadcom/bcmsysport.c:2367-2396) lacks the 'if…
- [Medium] priv->per_port_num_tx_queues and priv->ring_map[] are written
  by bcm_sysport_map_queues()/bcm_sysport_unmap_queues() (process…

--

net: systemport: Fix out-of-bounds array accesses in DSA queue mapping

This adds bounds checks against ARRAY_SIZE(priv->ring_map) in
bcm_sysport_select_queue(), bcm_sysport_map_queues() and
bcm_sysport_unmap_queues(), and clamps the SYSTEMPORT Lite call to
netif_set_real_num_tx_queues() to a minimum of one queue.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 95cead1df1607..130545cce045c 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2273,7 +2273,7 @@ static u16 bcm_sysport_select_queue(struct net_device *dev, struct sk_buff *skb,
>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>  	u16 queue = skb_get_queue_mapping(skb);
>  	struct bcm_sysport_tx_ring *tx_ring;
> -	unsigned int q, port;
> +	unsigned int q, port, index;
>  
>  	if (!netdev_uses_dsa(dev))
>  		return netdev_pick_tx(dev, skb, NULL);
> @@ -2281,8 +2281,11 @@ static u16 bcm_sysport_select_queue(struct net_device *dev, struct sk_buff *skb,
>  	/* DSA tagging layer will have configured the correct queue */
>  	q = BRCM_TAG_GET_QUEUE(queue);
>  	port = BRCM_TAG_GET_PORT(queue);
> -	tx_ring = priv->ring_map[q + port * priv->per_port_num_tx_queues];
> +	index = q + port * priv->per_port_num_tx_queues;
> +	if (unlikely(index >= ARRAY_SIZE(priv->ring_map)))
> +		return netdev_pick_tx(dev, skb, NULL);
>  
> +	tx_ring = priv->ring_map[index];
>  	if (unlikely(!tx_ring))
>  		return netdev_pick_tx(dev, skb, NULL);
>  

[Severity: Medium]

This isn't a race introduced by this patch, but since the new check reads
the same two fields, is it worth noting that bcm_sysport_select_queue()
combines priv->per_port_num_tx_queues and priv->ring_map[] with plain
loads on the lockless transmit path?

The writers run in process context under rtnl via
bcm_sysport_netdevice_event(), and bcm_sysport_map_queues() publishes the
new stride before it repopulates the table:

	priv->per_port_num_tx_queues = num_tx_queues;

	for (q = 0, qp = 0; q < dev->num_tx_queues && qp < num_tx_queues;
	     q++) {
		...
		priv->ring_map[qp + port * num_tx_queues] = ring;

There is no READ_ONCE/WRITE_ONCE, no RCU and no synchronize_net() between
the writers and bcm_sysport_select_queue(), so a transmit in flight during
a DSA reconfiguration (dsa_user_create, dsa_user_destroy or
dsa_user_change_conduit) can observe the new stride with the old table
contents, or a ring whose TDMA_DESC_RING_MAPPING no longer matches. The
ring pointers themselves stay alive for the netdev lifetime, so this looks
like mis-steering rather than a use-after-free. The new bounds check keeps
the index in range but does it make the stride and the table mutually
consistent?

> @@ -2329,7 +2332,8 @@ static int bcm_sysport_map_queues(struct net_device *dev,
>  	 */
>  	if (priv->is_lite)
>  		netif_set_real_num_tx_queues(slave_dev,
> -					     slave_dev->num_tx_queues / 2);
> +					     max_t(unsigned int, 1,
> +						   slave_dev->num_tx_queues / 2));
>  
>  	num_tx_queues = slave_dev->real_num_tx_queues;
>  
> @@ -2352,7 +2356,8 @@ static int bcm_sysport_map_queues(struct net_device *dev,
>  		ring->switch_queue = qp;
>  		ring->switch_port = port;
>  		ring->inspect = true;
> -		priv->ring_map[qp + port * num_tx_queues] = ring;
> +		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
> +			priv->ring_map[qp + port * num_tx_queues] = ring;
>  		qp++;
>  	}
>  

[Severity: Medium]

When the index is out of range here, only the software record is dropped:
ring->switch_queue, ring->switch_port and ring->inspect are still
committed, qp is still advanced, and bcm_sysport_map_queues() still
returns 0 (NOTIFY_OK). Does that leave hardware and software state
inconsistent?

bcm_sysport_init_tx_ring() programs the hardware mapping purely from those
ring fields on every open and resume:

	reg = tdma_readl(priv, TDMA_DESC_RING_MAPPING(index));
	reg &= ~(RING_QID_MASK | RING_PORT_ID_MASK << RING_PORT_ID_SHIFT);
	if (ring->inspect) {
		reg |= ring->switch_queue & RING_QID_MASK;
		reg |= ring->switch_port << RING_PORT_ID_SHIFT;
	} else {
		reg |= RING_IGNORE_STATUS;
	}

So the ring is bound by hardware to a switch (port, queue) pair that
bcm_sysport_select_queue() can never resolve, and those frames fall back
to netdev_pick_tx() where they can land on a ring programmed for a
different switch port and queue.

Since ring->inspect stays true, later bcm_sysport_map_queues() calls for
other ports skip the ring:

		if (ring->inspect)
			continue;

which permanently consumes the ring for a mapping that was never
recorded. Would skipping the whole iteration (not setting inspect, not
advancing qp, not programming the register), or warning and failing, be
closer to the intent than dropping only the ring_map store?

For what it's worth, I could not find an in-tree configuration that
reaches this branch today: ARRAY_SIZE(priv->ring_map) is 96, the maximum
ds->num_tx_queues is 8 and bcm_sf2 clamps num_ports to DSA_MAX_PORTS,
giving a largest index of 95.

> @@ -2383,7 +2388,8 @@ static int bcm_sysport_unmap_queues(struct net_device *dev,
>  
>  		ring->inspect = false;
>  		qp = ring->switch_queue;
> -		priv->ring_map[qp + port * num_tx_queues] = NULL;
> +		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
> +			priv->ring_map[qp + port * num_tx_queues] = NULL;
>  	}
>  
>  	return 0;

[Severity: Medium]

Same question in the other direction: ring->inspect is cleared
unconditionally while the ring_map clear can now be skipped. Can that
leave a stale non-NULL entry in priv->ring_map[] that
bcm_sysport_select_queue() keeps returning for a ring that is no longer
set up for switch queue inspection?

[Severity: Medium]

This is a pre-existing issue and not something this patch introduces, but
while looking at the index computation in bcm_sysport_unmap_queues(): this
function has no equivalent of the guard that bcm_sysport_map_queues()
starts with:

	if (dp->ds->index)
		return 0;

Every DSA user netdev becomes an upper of the conduit regardless of its
switch index, in dsa_user_create():

	ret = netdev_upper_dev_link(conduit, user_dev, NULL);

and it is unlinked on destroy and on a conduit change via
dsa_user_change_conduit():

	netdev_upper_dev_unlink(old_conduit, dev);

bcm_sysport_netdevice_event() forwards every non-linking CHANGEUPPER of a
DSA user device to bcm_sysport_unmap_queues(). So for a user port of a
cascaded switch, where map_queues() never created a mapping, unmap_queues()
still runs with port = dp->index and num_tx_queues taken from that foreign
netdev, and matches rings with:

		if (ring->switch_port != port)
			continue;
		if (!ring->inspect)
			continue;

Those are exactly the rings mapped for the directly attached switch's port
of the same index. Can this clear inspect for a still-live port, and, when
the cascaded switch advertises a different queue count than switch 0,
write NULL into a ring_map slot that belongs to a different (port, queue)
while the de-inspected ring's own slot stays populated? The new bounds
check would not catch this since the index is in range, just computed from
the wrong stride.

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

[PATCH net 09/12] net: systemport: Fix inverted error messages in bcm_sysport_stop()

[Florian Fainelli]

In bcm_sysport_stop(), the error messages printed when tdma_enable_set()
and rdma_enable_set() time out were inverted: the failure of
tdma_enable_set() logged 'timeout disabling RDMA' and the failure of
rdma_enable_set() logged 'timeout disabling TDMA'.

Swap the error messages so they correctly describe the failing engine.

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 130545cce045..384423b312ca 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2089,7 +2089,7 @@ static int bcm_sysport_stop(struct net_device *dev)
 
 	ret = tdma_enable_set(priv, 0);
 	if (ret) {
-		netdev_err(dev, "timeout disabling RDMA\n");
+		netdev_err(dev, "timeout disabling TDMA\n");
 		return ret;
 	}
 
@@ -2098,7 +2098,7 @@ static int bcm_sysport_stop(struct net_device *dev)
 
 	ret = rdma_enable_set(priv, 0);
 	if (ret) {
-		netdev_err(dev, "timeout disabling TDMA\n");
+		netdev_err(dev, "timeout disabling RDMA\n");
 		return ret;
 	}
 
-- 
2.34.1

Re: [PATCH net 09/12] net: systemport: Fix inverted error messages in bcm_sysport_stop()

[Nicolai Buchwitz]

On 22.9.2026 01:13, Florian Fainelli wrote:
> In bcm_sysport_stop(), the error messages printed when 
> tdma_enable_set()
> and rdma_enable_set() time out were inverted: the failure of
> tdma_enable_set() logged 'timeout disabling RDMA' and the failure of
> rdma_enable_set() logged 'timeout disabling TDMA'.
> 
> Swap the error messages so they correctly describe the failing engine.
> 
> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet 
> MAC driver")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 130545cce045..384423b312ca 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2089,7 +2089,7 @@ static int bcm_sysport_stop(struct net_device 
> *dev)
> 
>  	ret = tdma_enable_set(priv, 0);
>  	if (ret) {
> -		netdev_err(dev, "timeout disabling RDMA\n");
> +		netdev_err(dev, "timeout disabling TDMA\n");
>  		return ret;
>  	}
> 
> @@ -2098,7 +2098,7 @@ static int bcm_sysport_stop(struct net_device 
> *dev)
> 
>  	ret = rdma_enable_set(priv, 0);
>  	if (ret) {
> -		netdev_err(dev, "timeout disabling TDMA\n");
> +		netdev_err(dev, "timeout disabling RDMA\n");
>  		return ret;
>  	}

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

Thanks,
Nicolai

[PATCH net 10/12] net: systemport: Fix unbalanced Wake-on-LAN clock disable in resume

[Florian Fainelli]

In bcm_sysport_suspend(), the Wake-on-LAN clock (priv->wol_clk) is only
prepared and enabled if both device_may_wakeup(d) and priv->wolopts are
true.

In bcm_sysport_resume(), however, clk_disable_unprepare(priv->wol_clk)
was called whenever priv->wolopts was non-zero, regardless of
device_may_wakeup(d). If the system entered suspend with Wake-on-LAN
disabled at the device level (e.g., via sysfs wakeup control), this
resulted in an unbalanced clk_disable_unprepare() call on resume.

Fix this by mirroring the suspend check in bcm_sysport_resume().

Fixes: 6328a126896e ("net: systemport: Manage Wake-on-LAN clock")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 384423b312ca..dd5a7c9dd90f 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2803,7 +2803,7 @@ static int __maybe_unused bcm_sysport_resume(struct device *d)
 		return ret;
 	}
 
-	if (priv->wolopts)
+	if (device_may_wakeup(d) && priv->wolopts)
 		clk_disable_unprepare(priv->wol_clk);
 
 	umac_reset(priv);
-- 
2.34.1

Re: [PATCH net 10/12] net: systemport: Fix unbalanced Wake-on-LAN clock disable in resume

[Nicolai Buchwitz]

On 22.9.2026 01:13, Florian Fainelli wrote:
> In bcm_sysport_suspend(), the Wake-on-LAN clock (priv->wol_clk) is only
> prepared and enabled if both device_may_wakeup(d) and priv->wolopts are
> true.
> 
> In bcm_sysport_resume(), however, clk_disable_unprepare(priv->wol_clk)
> was called whenever priv->wolopts was non-zero, regardless of
> device_may_wakeup(d). If the system entered suspend with Wake-on-LAN
> disabled at the device level (e.g., via sysfs wakeup control), this
> resulted in an unbalanced clk_disable_unprepare() call on resume.
> 
> Fix this by mirroring the suspend check in bcm_sysport_resume().
> 
> Fixes: 6328a126896e ("net: systemport: Manage Wake-on-LAN clock")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 384423b312ca..dd5a7c9dd90f 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2803,7 +2803,7 @@ static int __maybe_unused 
> bcm_sysport_resume(struct device *d)
>  		return ret;
>  	}
> 
> -	if (priv->wolopts)
> +	if (device_may_wakeup(d) && priv->wolopts)
>  		clk_disable_unprepare(priv->wol_clk);
> 
>  	umac_reset(priv);

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

Thanks,
Nicolai

Re: [PATCH net 10/12] net: systemport: Fix unbalanced Wake-on-LAN clock disable in resume

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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] bcm_sysport_suspend()
  (drivers/net/ethernet/broadcom/bcmsysport.c:2780-2783) discards the…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it defeats
  the balance the patch aims for on the suspend error path: when…
- [Low] This problem wasn't introduced by this patch, but it leaves the
  driver internally inconsistent about the exact condition the patch…

--

net: systemport: Fix unbalanced Wake-on-LAN clock disable in resume

The patch changes bcm_sysport_resume() so that the Wake-on-LAN clock is
only disabled when both device_may_wakeup(d) and priv->wolopts are true,
matching the condition used in bcm_sysport_suspend() when the clock is
prepared and enabled.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 384423b312ca2..dd5a7c9dd90fb 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2803,7 +2803,7 @@ static int __maybe_unused bcm_sysport_resume(struct device *d)
>  		return ret;
>  	}
>  
> -	if (priv->wolopts)
> +	if (device_may_wakeup(d) && priv->wolopts)
>  		clk_disable_unprepare(priv->wol_clk);

[Severity: Medium]
The new condition describes what suspend intended to do rather than what it
actually achieved.  In bcm_sysport_suspend() the return value of the enable
is discarded:

	/* Get prepared for Wake-on-LAN */
	if (device_may_wakeup(d) && priv->wolopts) {
		clk_prepare_enable(priv->wol_clk);
		ret = bcm_sysport_suspend_to_wol(priv);
	}

	clk_disable_unprepare(priv->clk);

	return ret;

priv->wol_clk is a real clock whenever the DT provides sw_sysportwol, since
bcm_sysport_probe() obtains it with devm_clk_get_optional(&pdev->dev,
"sw_sysportwol"), so it is NULL only when the clock is absent.

If clk_prepare_enable(priv->wol_clk) fails, bcm_sysport_suspend_to_wol()
still runs and can return 0, because the UMAC/RBUF/RXCHK registers it pokes
are clocked by priv->clk, which is only disabled after that block.  Suspend
therefore returns 0, the PM core calls bcm_sysport_resume(), and the line
above drops a prepare/enable reference that was never taken, which should
trip the clk core WARN(core->enable_count == 0) and
WARN(core->prepare_count == 0) checks.

So can the unbalanced clk_disable_unprepare() the commit message describes
still happen here?  Would latching the real outcome, for example a flag set
only when clk_prepare_enable() returned 0, or checking and propagating that
error, be a more complete fix than re-evaluating the predicate?

There is also a diagnostic side to the ignored return: the system suspends
believing WoL is armed while the WoL block is unclocked, so the wake event
can be lost silently.  Every other clk_prepare_enable() in this file checks
its return, including the one immediately above this hunk in
bcm_sysport_resume().

[Severity: Medium]
This isn't a bug introduced by this patch, but it affects the same balance
the patch is trying to establish.  When bcm_sysport_suspend_to_wol() fails,
bcm_sysport_suspend() returns the error with priv->wol_clk still prepared
and enabled, and there is no unwind for that:

	if (device_may_wakeup(d) && priv->wolopts) {
		clk_prepare_enable(priv->wol_clk);
		ret = bcm_sysport_suspend_to_wol(priv);
	}

	clk_disable_unprepare(priv->clk);

	return ret;

The failure comes from the RBUF WoL-mode poll in
bcm_sysport_suspend_to_wol():

	/* Do not leave the UniMAC RBUF matching only MPD packets */
	if (!timeout) {
		mpd_enable_set(priv, false);
		netif_err(priv, wol, ndev, "failed to enter WOL mode\n");
		return -ETIMEDOUT;
	}

The PM core sets dev->power.is_suspended only when the suspend callback
returned 0, and device_resume() bails out early with:

	if (!dev->power.is_suspended)
		goto Complete;

Since the clk_disable_unprepare(priv->wol_clk) edited by this patch is the
only disable site for that clock in the driver, does the sw_sysportwol
clock stay enabled forever after a failed suspend, with the enable count
growing on each further failed attempt?

On the WAKE_FILTER path the -ETIMEDOUT leg clears only MPD_EN, while
RBUF_ACPI_EN(_LITE) and RXCHK_EN | RXCHK_BRCM_TAG_EN stay set, and
bcm_sysport_resume_from_wol() never runs to clear them.  Should the suspend
error path also release wol_clk and roll back the RBUF/RXCHK programming,
rather than relying on resume mirroring the predicate?

[Severity: Low]
This is a pre-existing issue, but this change makes
device_may_wakeup(d) && priv->wolopts the driver's definition of "WoL is
armed for this suspend" in both bcm_sysport_suspend() and
bcm_sysport_resume(), while a third site in bcm_sysport_suspend() still
uses priv->wolopts alone:

	/* Flush RX pipe */
	if (!priv->wolopts)
		topctrl_writel(priv, RX_FLUSH, RX_FLUSH_CNTL);

In exactly the configuration this commit addresses, wakeup disabled at the
device level via /sys/.../power/wakeup while priv->wolopts is still
non-zero because the ethtool WoL options were never cleared, suspend skips
the RX_FLUSH write as if WoL were armed, yet bcm_sysport_suspend_to_wol()
is never called, the MPD/ACPI/RXCHK matching is never enabled and the WoL
clock is never enabled.

Should this flush decision use the same condition, so the RX pipe is not
left unflushed in a suspend where nothing is armed to match or consume
incoming traffic until bcm_sysport_resume() rewrites RX_FLUSH_CNTL?

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

[PATCH net 11/12] net: systemport: Update TDMA queue mapping dynamically on changeupper

[Florian Fainelli]

When DSA upper devices are dynamically attached or detached while the
master SYSTEMPORT interface is already up and running (netif_running()),
bcm_sysport_map_queues() and bcm_sysport_unmap_queues() updated the
internal software mappings but did not update the TDMA_DESC_RING_MAPPING
hardware registers, because programming was previously deferred until
bcm_sysport_init_tx_ring().

Update TDMA_DESC_RING_MAPPING registers immediately if netif_running()
is true during map_queues and unmap_queues.

Fixes: 1593cd40d785 ("net: systemport: use standard netdevice notifier to detect DSA presence")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 23 +++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index dd5a7c9dd90f..e5bb7fa84fda 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2315,6 +2315,7 @@ static int bcm_sysport_map_queues(struct net_device *dev,
 	struct bcm_sysport_tx_ring *ring;
 	unsigned int num_tx_queues;
 	unsigned int q, qp, port;
+	u32 reg;
 
 	/* We can't be setting up queue inspection for non directly attached
 	 * switches
@@ -2350,14 +2351,21 @@ static int bcm_sysport_map_queues(struct net_device *dev,
 		if (ring->inspect)
 			continue;
 
-		/* Just remember the mapping actual programming done
-		 * during bcm_sysport_init_tx_ring
-		 */
 		ring->switch_queue = qp;
 		ring->switch_port = port;
 		ring->inspect = true;
 		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
 			priv->ring_map[qp + port * num_tx_queues] = ring;
+
+		if (netif_running(dev)) {
+			reg = tdma_readl(priv, TDMA_DESC_RING_MAPPING(q));
+			reg &= ~(RING_QID_MASK |
+				 RING_PORT_ID_MASK << RING_PORT_ID_SHIFT |
+				 RING_IGNORE_STATUS);
+			reg |= (qp & RING_QID_MASK);
+			reg |= (port << RING_PORT_ID_SHIFT);
+			tdma_writel(priv, reg, TDMA_DESC_RING_MAPPING(q));
+		}
 		qp++;
 	}
 
@@ -2372,6 +2380,7 @@ static int bcm_sysport_unmap_queues(struct net_device *dev,
 	struct bcm_sysport_tx_ring *ring;
 	unsigned int num_tx_queues;
 	unsigned int q, qp, port;
+	u32 reg;
 
 	port = dp->index;
 
@@ -2390,6 +2399,14 @@ static int bcm_sysport_unmap_queues(struct net_device *dev,
 		qp = ring->switch_queue;
 		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
 			priv->ring_map[qp + port * num_tx_queues] = NULL;
+
+		if (netif_running(dev)) {
+			reg = tdma_readl(priv, TDMA_DESC_RING_MAPPING(q));
+			reg &= ~(RING_QID_MASK |
+				 RING_PORT_ID_MASK << RING_PORT_ID_SHIFT);
+			reg |= RING_IGNORE_STATUS;
+			tdma_writel(priv, reg, TDMA_DESC_RING_MAPPING(q));
+		}
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH net 11/12] net: systemport: Update TDMA queue mapping dynamically on changeupper

[Nicolai Buchwitz]

Hi Florian

On 22.9.2026 01:13, Florian Fainelli wrote:
> When DSA upper devices are dynamically attached or detached while the
> master SYSTEMPORT interface is already up and running 
> (netif_running()),
> bcm_sysport_map_queues() and bcm_sysport_unmap_queues() updated the
> internal software mappings but did not update the 
> TDMA_DESC_RING_MAPPING
> hardware registers, because programming was previously deferred until
> bcm_sysport_init_tx_ring().
> 
> Update TDMA_DESC_RING_MAPPING registers immediately if netif_running()
> is true during map_queues and unmap_queues.
> 
> Fixes: 1593cd40d785 ("net: systemport: use standard netdevice notifier 
> to detect DSA presence")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 23 +++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index dd5a7c9dd90f..e5bb7fa84fda 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c

> [...]

> @@ -2390,6 +2399,14 @@ static int bcm_sysport_unmap_queues(struct 
> net_device *dev,
>  		qp = ring->switch_queue;
>  		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
>  			priv->ring_map[qp + port * num_tx_queues] = NULL;
> +
> +		if (netif_running(dev)) {
> +			reg = tdma_readl(priv, TDMA_DESC_RING_MAPPING(q));
> +			reg &= ~(RING_QID_MASK |
> +				 RING_PORT_ID_MASK << RING_PORT_ID_SHIFT);
> +			reg |= RING_IGNORE_STATUS;
> +			tdma_writel(priv, reg, TDMA_DESC_RING_MAPPING(q));
> +		}

RING_IGNORE_STATUS is never cleared in init_tx_rings(), so

     echo <sf2-node> > /sys/bus/platform/drivers/brcm-sf2/unbind
     ip link set eth0 down
     echo <sf2-node> > /sys/bus/platform/drivers/brcm-sf2/bind
     ip link set eth0 up

keeps the bit set from unmap. Clear it in init_tx_ring() too?

> [...]

Thanks,
Nicolai

Re: [PATCH net 11/12] net: systemport: Update TDMA queue mapping dynamically on changeupper

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 3 (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).

- [High] bcm_sysport_unmap_queues() now latches RING_IGNORE_STATUS into
  TDMA_DESC_RING_MAPPING(q) at runtime, but the only other writer of…
- [Medium] bcm_sysport_unmap_queues()
  (drivers/net/ethernet/broadcom/bcmsysport.c:2374-2412) lacks the…
- [Medium] netif_running(dev) is used as the sole precondition for the
  new tdma_readl()/tdma_writel() accesses, but netif_running()…
- [Medium] net: systemport: TX ring software mapping exposed before
  hardware is configured

Pre-existing issues:
- [Medium] In bcm_sysport_map_queues() the newly added register write
  shifts the DSA port index into the 3-bit RING_PORT_ID field without…

--

net: systemport: Update TDMA queue mapping dynamically on changeupper

This commit makes bcm_sysport_map_queues() and bcm_sysport_unmap_queues()
program the TDMA_DESC_RING_MAPPING registers right away when the conduit
interface is already running, instead of deferring all programming to
bcm_sysport_init_tx_ring().

A few questions about the new register programming below.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index dd5a7c9dd90fb..e5bb7fa84fda5 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2350,14 +2351,21 @@ static int bcm_sysport_map_queues(struct net_device *dev,
>  		if (ring->inspect)
>  			continue;
>  
> -		/* Just remember the mapping actual programming done
> -		 * during bcm_sysport_init_tx_ring
> -		 */
>  		ring->switch_queue = qp;
>  		ring->switch_port = port;
>  		ring->inspect = true;
>  		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
>  			priv->ring_map[qp + port * num_tx_queues] = ring;
> +
> +		if (netif_running(dev)) {

[Severity: Medium]
Is netif_running() sufficient on its own to guarantee the SYSTEMPORT
register block is clocked here (and in the matching check added to
bcm_sysport_unmap_queues())?

bcm_sysport_suspend() only bails out when the interface is not running,
and then detaches and gates the clock:

	if (!netif_running(dev))
		return 0;

	netif_device_detach(dev);
	...
	clk_disable_unprepare(priv->clk);

netif_device_detach() clears __LINK_STATE_PRESENT, not
__LINK_STATE_START, so netif_running() stays true for the whole suspended
window while priv->clk is off.  The same state persists indefinitely after
a failed bcm_sysport_resume(), where every error path ends in:

out_free_tx_rings:
	for (i = 0; i < dev->num_tx_queues; i++)
		bcm_sysport_fini_tx_ring(priv, i);
	clk_disable_unprepare(priv->clk);
	return ret;

with no netif_device_attach().

The PM callbacks do not take rtnl_lock, so they are not serialized against
the rtnl-protected notifier path
bcm_sysport_netdevice_event()->bcm_sysport_unmap_queues().  A DSA switch
driver unbind in that window (dsa_user_destroy()->netdev_upper_dev_unlink())
would reach the new tdma_readl()/tdma_writel() sequence with the block
clock-gated.  Before this patch neither function touched MMIO.  Would
pairing netif_running(dev) with netif_device_present(dev) here close that
window?

> +			reg = tdma_readl(priv, TDMA_DESC_RING_MAPPING(q));
> +			reg &= ~(RING_QID_MASK |
> +				 RING_PORT_ID_MASK << RING_PORT_ID_SHIFT |
> +				 RING_IGNORE_STATUS);
> +			reg |= (qp & RING_QID_MASK);
> +			reg |= (port << RING_PORT_ID_SHIFT);

[Severity: Medium]
The new register programming happens after the ring has already
been published to the TX path.  ring->inspect = true and
priv->ring_map[qp + port * num_tx_queues] = ring are stored first,
and only then does the netif_running() block read/modify/write
TDMA_DESC_RING_MAPPING(q).

bcm_sysport_select_queue() looks the ring up purely from software
state:

	tx_ring = priv->ring_map[index];
	if (unlikely(!tx_ring))
		return netdev_pick_tx(dev, skb, NULL);
	return tx_ring->index;

The notifier path runs under rtnl_lock, but the transmit path does
not, so between the ring_map[] store and the tdma_writel() a
concurrent xmit can already be steered onto ring q while
TDMA_DESC_RING_MAPPING(q) still holds the previous contents -
either the RING_IGNORE_STATUS that bcm_sysport_init_tx_ring() sets
in its else branch for non-inspected rings, or the one the new code
in bcm_sysport_unmap_queues() writes, plus a stale QID/port from an
earlier mapping.  Frames going out in that window are transmitted
with the switch port/queue status ignored, i.e. the switch
backpressure this mapping exists to honour is bypassed and the
frames can be dropped in the switch if the target queue is
congested.

Note bcm_sysport_unmap_queues() has the safe ordering for its
direction (software mapping torn down first, hardware told to ignore
status afterwards); map_queues() is the one publishing the ring
before the hardware agrees.  Would it be enough to move the
netif_running() register update ahead of the ring->inspect and
priv->ring_map[] assignments, or do you also need the ring stopped
(ring->lock / netif_tx_stop_queue()) around the update to make the
software and hardware view flip atomically for an in-flight xmit?

[Severity: Medium]
This isn't a bug introduced by this patch - bcm_sysport_init_tx_ring() has
the identical unmasked encoding:

	reg |= ring->switch_queue & RING_QID_MASK;
	reg |= ring->switch_port << RING_PORT_ID_SHIFT;

but since the new code replicates it into the live reconfiguration path:
should port be masked with RING_PORT_ID_MASK the way qp is masked with
RING_QID_MASK?

The field is only three bits wide and sits right next to the status bits:

#define  RING_PORT_ID_SHIFT		3
#define  RING_PORT_ID_MASK		0x7
#define  RING_IGNORE_STATUS		(1 << 6)
#define  RING_FAILOVER_EN		(1 << 7)
#define  RING_CREDIT_SHIFT		8

port comes from dp->index and is only filtered for the tree index
(if (dp->ds->index) return 0;), never bounded to 7, while the driver sizes
priv->ring_map[] as DSA_MAX_PORTS * 8 and guards with
qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map).  For port == 8 the
shifted value is exactly 0x40, which re-sets the RING_IGNORE_STATUS bit the
preceding mask just cleared and programs switch port 0; port >= 16 would
also reach RING_FAILOVER_EN and the credit field.

> +			tdma_writel(priv, reg, TDMA_DESC_RING_MAPPING(q));
> +		}
>  		qp++;
>  	}
>  
> @@ -2372,6 +2380,7 @@ static int bcm_sysport_unmap_queues(struct net_device *dev,
>  	struct bcm_sysport_tx_ring *ring;
>  	unsigned int num_tx_queues;
>  	unsigned int q, qp, port;
> +	u32 reg;
>  
>  	port = dp->index;
>  
> @@ -2390,6 +2399,14 @@ static int bcm_sysport_unmap_queues(struct net_device *dev,
>  		qp = ring->switch_queue;
>  		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
>  			priv->ring_map[qp + port * num_tx_queues] = NULL;
> +
> +		if (netif_running(dev)) {
> +			reg = tdma_readl(priv, TDMA_DESC_RING_MAPPING(q));
> +			reg &= ~(RING_QID_MASK |
> +				 RING_PORT_ID_MASK << RING_PORT_ID_SHIFT);
> +			reg |= RING_IGNORE_STATUS;
> +			tdma_writel(priv, reg, TDMA_DESC_RING_MAPPING(q));
> +		}

[Severity: High]
Can the RING_IGNORE_STATUS bit set here become sticky?

The only other writer of that register, bcm_sysport_init_tx_ring(), leaves
RING_IGNORE_STATUS out of its clear mask:

	reg = tdma_readl(priv, TDMA_DESC_RING_MAPPING(index));
	reg &= ~(RING_QID_MASK | RING_PORT_ID_MASK << RING_PORT_ID_SHIFT);
	if (ring->inspect) {
		reg |= ring->switch_queue & RING_QID_MASK;
		reg |= ring->switch_port << RING_PORT_ID_SHIFT;
	} else {
		reg |= RING_IGNORE_STATUS;
	}
	tdma_writel(priv, reg, TDMA_DESC_RING_MAPPING(index));

bcm_sysport_fini_tx_ring() does not reset the mapping register, umac_reset()
only resets UniMAC and topctrl_flush() only flushes the RX/TX pipes, so the
bit appears to survive ifdown/ifup and suspend/resume.

That gives this sequence:

1. DSA user linked while the conduit runs: map_queues() programs QID/port
   and clears RING_IGNORE_STATUS.
2. DSA user unlinked while running: the new code above sets
   RING_IGNORE_STATUS.
3. ifdown.
4. DSA user re-linked while down: netif_running() is false, so only
   ring->inspect and priv->ring_map[] are updated.
5. ifup: bcm_sysport_init_tx_ring() takes the ring->inspect branch,
   programs QID/port, but cannot clear RING_IGNORE_STATUS.

Software then treats the ring as inspected and steers Brcm-tagged frames to
it:

	tx_ring = priv->ring_map[index];
	if (unlikely(!tx_ring))
		return netdev_pick_tx(dev, skb, NULL);
	return tx_ring->index;

while hardware still ignores the switch port/queue status.  Step 2 wrote
nothing before this patch.

The same end state also seems reachable when the interface is first opened
with no DSA upper (init_tx_ring()'s else branch sets the bit on every ring)
and a DSA user is attached while the interface is down, which is the
software/hardware desync this commit aims to close.  Would deriving
RING_IGNORE_STATUS from ring->inspect at both sites - i.e. adding
RING_IGNORE_STATUS to init_tx_ring()'s clear mask, or factoring out one
shared mapping helper - handle both cases?

[Severity: Medium]
Should bcm_sysport_unmap_queues() also carry the switch-index filter that
bcm_sysport_map_queues() has?

map_queues() refuses non directly attached switches:

	/* We can't be setting up queue inspection for non directly attached
	 * switches
	 */
	if (dp->ds->index)
		return 0;

unmap_queues() has no such check and selects rings only by port index:

	if (ring->switch_port != port)
		continue;

In a multi-switch tree sharing this conduit, a user port on a downstream
switch (ds->index != 0) can have the same dp->index as a mapped port on the
directly attached switch.  The notifier forwards every DSA user unlink on
this conduit without filtering on the switch index:

	if (!dsa_user_dev_check(info->upper_dev))
		return NOTIFY_DONE;
	if (info->linking)
		ret = bcm_sysport_map_queues(dev, info->upper_dev);
	else
		ret = bcm_sysport_unmap_queues(dev, info->upper_dev);

Previously that mis-selection only cleared software state (ring->inspect,
priv->ring_map[]).  With this patch, when the conduit is running, it also
rewrites those live rings' TDMA_DESC_RING_MAPPING registers, dropping the
QID/port binding and setting RING_IGNORE_STATUS for a port that is still
attached and still mapped on switch 0.

>  	}
>  
>  	return 0;

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

[PATCH net 12/12] net: systemport: Complete resource teardown even on DMA disable timeout

[Florian Fainelli]

In bcm_sysport_stop(), if tdma_enable_set() or rdma_enable_set() timed
out, the function returned early with an error code. However, ndo_stop()
callers in the networking core ignore error returns, leaving the
software ring structures allocated, interrupts registered, PHY
connected, and clock enabled. If the interface was subsequently brought
up again, request_irq() and PHY connection would fail or leak.

Ensure all software ring teardown, interrupt freeing, PHY
disconnection, and clock disabling steps are executed regardless of DMA
disable timeouts.

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index e5bb7fa84fda..2b064da4eb7c 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2088,19 +2088,15 @@ static int bcm_sysport_stop(struct net_device *dev)
 	umac_enable_set(priv, CMD_RX_EN, 0);
 
 	ret = tdma_enable_set(priv, 0);
-	if (ret) {
+	if (ret)
 		netdev_err(dev, "timeout disabling TDMA\n");
-		return ret;
-	}
 
 	/* Wait for a maximum packet size to be drained */
 	usleep_range(2000, 3000);
 
 	ret = rdma_enable_set(priv, 0);
-	if (ret) {
+	if (ret)
 		netdev_err(dev, "timeout disabling RDMA\n");
-		return ret;
-	}
 
 	/* Disable UniMAC TX */
 	umac_enable_set(priv, CMD_TX_EN, 0);
-- 
2.34.1

Re: [PATCH net 12/12] net: systemport: Complete resource teardown even on DMA disable timeout

[Nicolai Buchwitz]

Hi Florian

On 22.9.2026 01:13, Florian Fainelli wrote:
> In bcm_sysport_stop(), if tdma_enable_set() or rdma_enable_set() timed
> out, the function returned early with an error code. However, 
> ndo_stop()
> callers in the networking core ignore error returns, leaving the
> software ring structures allocated, interrupts registered, PHY
> connected, and clock enabled. If the interface was subsequently brought
> up again, request_irq() and PHY connection would fail or leak.
> 
> Ensure all software ring teardown, interrupt freeing, PHY
> disconnection, and clock disabling steps are executed regardless of DMA
> disable timeouts.
> 
> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet 
> MAC driver")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index e5bb7fa84fda..2b064da4eb7c 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2088,19 +2088,15 @@ static int bcm_sysport_stop(struct net_device 
> *dev)
>  	umac_enable_set(priv, CMD_RX_EN, 0);
> 
>  	ret = tdma_enable_set(priv, 0);
> -	if (ret) {
> +	if (ret)
>  		netdev_err(dev, "timeout disabling TDMA\n");
> -		return ret;
> -	}
> 
>  	/* Wait for a maximum packet size to be drained */
>  	usleep_range(2000, 3000);
> 
>  	ret = rdma_enable_set(priv, 0);
> -	if (ret) {
> +	if (ret)
>  		netdev_err(dev, "timeout disabling RDMA\n");
> -		return ret;
> -	}

If TDMA or RDMA don't stop, the rings get freed while the hardware may
still use them. Maybe topctrl_flush() before freeing?

The same pattern would probably apply to bcm_sysport_suspend().

> [...]

Thanks,
Nicolai

Re: [PATCH net 12/12] net: systemport: Complete resource teardown even on DMA disable timeout

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 (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).

- [High] bcm_sysport_stop() now unmaps and frees the RX/TX DMA buffers
  even when the DMA engines are known to be still running.
- [Medium] The unconditional teardown path now reaches
  clk_disable_unprepare(priv->clk)…

Pre-existing issues:
- [Medium] The same early-return-on-DMA-disable-timeout pattern remains
  in bcm_sysport_suspend(): 'ret = rdma_enable_set(priv, 0); if (ret) {…

--

net: systemport: Complete resource teardown even on DMA disable timeout

bcm_sysport_stop() used to return early when tdma_enable_set() or
rdma_enable_set() timed out, which left the rings, interrupts, PHY and
clock in place because ndo_stop() return values are ignored. The patch
turns both early returns into plain error prints so the rest of the
teardown always runs.

A few questions on the new flow.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index e5bb7fa84fda5..2b064da4eb7c0 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2088,19 +2088,15 @@ static int bcm_sysport_stop(struct net_device *dev)
>  	umac_enable_set(priv, CMD_RX_EN, 0);
>  
>  	ret = tdma_enable_set(priv, 0);
> -	if (ret) {
> +	if (ret)
>  		netdev_err(dev, "timeout disabling TDMA\n");
> -		return ret;
> -	}
>  
>  	/* Wait for a maximum packet size to be drained */
>  	usleep_range(2000, 3000);
>  
>  	ret = rdma_enable_set(priv, 0);
> -	if (ret) {
> +	if (ret)
>  		netdev_err(dev, "timeout disabling RDMA\n");
> -		return ret;
> -	}

[Severity: High]
Can the device still DMA into the freed RX buffers here?

rdma_enable_set() only returns -ETIMEDOUT after polling RDMA_STATUS for
roughly a second without ever seeing RDMA_DISABLED, so the error return is
evidence that the engine has not quiesced. With the return gone,
bcm_sysport_stop() continues into:

	/* Free RX/TX rings SW structures */
	for (i = 0; i < dev->num_tx_queues; i++)
		bcm_sysport_fini_tx_ring(priv, i);
	bcm_sysport_fini_rx_ring(priv);

and bcm_sysport_fini_rx_ring() only warns about exactly this state before
releasing everything:

	/* Caller should ensure RDMA is disabled */
	reg = rdma_readl(priv, RDMA_STATUS);
	if (!(reg & RDMA_DISABLED))
		netdev_warn(priv->netdev, "RDMA not stopped!\n");

	if (!priv->rx_cbs)
		return;

	for (i = 0; i < priv->num_rx_bds; i++) {
		cb = &priv->rx_cbs[i];
		if (dma_unmap_addr(cb, dma_addr))
			dma_unmap_single(&priv->pdev->dev,
					 dma_unmap_addr(cb, dma_addr),
					 RX_BUF_LENGTH, DMA_FROM_DEVICE);
		bcm_sysport_free_cb(cb);
	}

	kfree(priv->rx_cbs);

bcm_sysport_fini_tx_ring() has the same "TDMA not stopped!" warning followed
by bcm_sysport_tx_clean() and kfree(ring->cbs).

umac_enable_set(priv, CMD_RX_EN, 0) stops the UniMAC, not the RDMA/TDMA
descriptor engines, and bcm_sysport_stop() does no TOPCTRL flush and no
block reset, unlike bcm_sysport_suspend() which writes RX_FLUSH/TX_FLUSH.
Would it be better to force the block quiescent on the timeout path first,
for example via topctrl_flush() and/or a block reset, before unmapping and
freeing the buffers? Alternatively, release the IRQs, PHY and clock while
keeping the DMA-mapped buffers owned by the driver, so the engine cannot
write into memory that has been returned to the allocator.

>  
>  	/* Disable UniMAC TX */
>  	umac_enable_set(priv, CMD_TX_EN, 0);

[Severity: Medium]
The commit message says "If the interface was subsequently brought up again,
request_irq() and PHY connection would fail or leak", which suggests the
interface is usable again after the fix. Does the re-open path actually
resynchronize the engines?

bcm_sysport_stop() now reaches clk_disable_unprepare(priv->clk) with
RDMA_EN/TDMA_EN never acknowledged as cleared, and no state is recorded to
say the DMA was stuck. On the next open, umac_reset() returns immediately
for priv->is_lite and otherwise only pulses CMD_SW_RESET, and
topctrl_flush() only pulses RX_FLUSH/TX_FLUSH; neither clears RDMA_EN or
TDMA_EN. bcm_sysport_init_rx_ring() then does:

	/* Initialize HW, ensure RDMA is disabled */
	reg = rdma_readl(priv, RDMA_STATUS);
	if (!(reg & RDMA_DISABLED))
		rdma_enable_set(priv, 0);

	rdma_writel(priv, 0, RDMA_WRITE_PTR_LO);
	rdma_writel(priv, 0, RDMA_WRITE_PTR_HI);
	rdma_writel(priv, 0, RDMA_PROD_INDEX);
	rdma_writel(priv, 0, RDMA_CONS_INDEX);

The rdma_enable_set() result is ignored and the pointer registers are zeroed
along with priv->rx_c_index and priv->rx_read_ptr, so if the engine is still
live its write pointer keeps advancing from the old position and the
software bookkeeping diverges from the hardware ring.
bcm_sysport_init_tx_ring() never samples TDMA_STATUS before rewriting
TDMA_DESC_RING_HEAD_TAIL_PTR/COUNT/PROD_CONS_INDEX, and bcm_sysport_open()
follows with:

	/* Initialize linked-list */
	tdma_writel(priv, TDMA_LL_RAM_INIT_BUSY, TDMA_STATUS);

Also, the later rdma_enable_set(priv, 1)/tdma_enable_set(priv, 1) in open()
return 0 immediately when the engine is already enabled, so open() reports
success on stale hardware state. Would recording a "DMA stuck" condition in
stop() and forcing a block reset on the next open make the re-open claim in
the commit message hold?

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but should
bcm_sysport_suspend() be converted the same way for consistency? It still
has both early returns:

	ret = rdma_enable_set(priv, 0);
	if (ret) {
		netdev_err(dev, "RDMA timeout!\n");
		return ret;
	}
	...
	ret = tdma_enable_set(priv, 0);
	if (ret) {
		netdev_err(dev, "TDMA timeout!\n");
		return ret;
	}

Both return before bcm_sysport_fini_tx_ring()/bcm_sysport_fini_rx_ring() and
before clk_disable_unprepare(priv->clk), while netif_device_detach(),
bcm_sysport_netif_stop() and phy_suspend() have already run and are not
undone. On a DMA timeout the suspend is aborted with the interface detached
(so netif_device_present() is false and a later open returns -ENODEV), the
PHY suspended, the ring cb arrays and their mapped SKBs still allocated, and
a clk prepare/enable reference still held.

After this patch the driver handles the same hardware failure in two
different ways, and the suspend path additionally flushes RX via
topctrl_writel(RX_FLUSH, RX_FLUSH_CNTL) and disables RDMA before TDMA,
which stop() does not. Is the divergence intentional?

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