[PATCH net v2 00/10] net: systemport: Collection of fixes

[PATCH net v2 00/10] net: systemport: Collection of fixes

[Florian Fainelli]

Changes in v2:

- dropped patch 4 and target it at net-next
- dropped patch 11 which needs more thinking as to what is the
  appropriate DMA teardown sequence wrt. topctrl_flush
- addressed Nicolai's feedback
- added Nicolai's Reviewed-by tags where provided

Florian Fainelli (10):
  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 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

 drivers/net/ethernet/broadcom/bcmsysport.c | 73 +++++++++++++++-------
 1 file changed, 50 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH net v2 01/10] 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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

[PATCH net v2 02/10] 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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

[PATCH net v2 03/10] 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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

[PATCH net v2 04/10] 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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 b91a57540f55..ec43aab11790 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2648,7 +2648,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);
@@ -2678,10 +2678,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

[PATCH net v2 05/10] 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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 ec43aab11790..9c1b515dc8cc 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

[PATCH net v2 06/10] 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).

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 9c1b515dc8cc..ed4337af58da 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

[PATCH net v2 07/10] 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 ed4337af58da..4efcefd33b78 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

[PATCH net v2 08/10] 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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 4efcefd33b78..11b2cb4cc792 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

[PATCH net v2 09/10] 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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 11b2cb4cc792..dedd49ad6c4e 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2799,7 +2799,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

[PATCH net v2 10/10] 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 dedd49ad6c4e..ca1b86953dba 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 v2 00/10] net: systemport: Collection of fixes

[Jakub Kicinski]

On Tue, 22 Sep 2026 16:24:30 -0700 Florian Fainelli wrote:
> - dropped patch 4 and target it at net-next
> - dropped patch 11 which needs more thinking as to what is the
>   appropriate DMA teardown sequence wrt. topctrl_flush
> - addressed Nicolai's feedback
> - added Nicolai's Reviewed-by tags where provided

Hi, our patch limit is 15, please don't have more than 15 patches
outstanding against any tree. As you can imagine everyone has LLM
access now. The number of outstanding patches in pw is 640 at the 
time of writing. Please.

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

[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

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] Now that bcm_sysport_poll_controller() passes a valid dev, the
  polled calls to bcm_sysport_rx_isr() and bcm_sysport_tx_isr() actually…

Pre-existing issues:
- [Medium] bcm_sysport_poll_controller()
  (drivers/net/ethernet/broadcom/bcmsysport.c:1196-1209) calls…

--

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

This commit makes bcm_sysport_poll_controller() pass dev instead of priv
to bcm_sysport_rx_isr() and bcm_sysport_tx_isr(). Both ISRs call
netdev_priv() on their dev_id argument, so passing priv crashed them when
netconsole or netpoll ran.

> 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);

[Severity: Medium]
This is a pre-existing issue, but is it safe to call disable_irq() here?

The only caller of ndo_poll_controller is netpoll_poll_dev(). It is called
from the retry loop in __netpoll_send_skb(), and that loop runs with IRQs
disabled:

net/core/netpoll.c:__netpoll_send_skb() {
    ...
	lockdep_assert_irqs_disabled();
    ...
}

netpoll_send_skb() calls local_irq_save() before this. For netconsole,
target_list_lock is also held.

disable_irq() can sleep:

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

With CONFIG_DEBUG_ATOMIC_SLEEP, would this print "sleeping function called
from invalid context" each time netpoll has to poll the device because TX
is busy or the queue is stopped?

The handlers are requested with flags 0. With threadirqs or PREEMPT_RT
they are force-threaded, so synchronize_irq() can wait_event() on the IRQ
thread with IRQs disabled.

Before this patch the path oopsed. Now it completes, so netconsole users on
this NIC would hit this problem next. No later patch in the series changes
the disable_irq() calls.

The driver already does TX completion in NAPI. netpoll_poll_dev() also
calls poll_napi() right after the callback:

	ops->ndo_poll_controller(dev);

	poll_napi(dev);

Could bcm_sysport_poll_controller() be removed entirely, as was done for
other NAPI drivers? Or could it at least use disable_hardirq(), as
e1000_netpoll() does?

> -	bcm_sysport_rx_isr(priv->irq0, priv);
> +	bcm_sysport_rx_isr(priv->irq0, dev);

[Severity: Medium]
Now that the ISR calls here actually run, will the masked status usually
be zero?

On the previous netpoll_poll_dev() iteration, poll_napi() already cleared
the status. bcm_sysport_desc_rx() clears INTRL2_0_RDMA_MBDONE, and
__bcm_sysport_tx_reclaim() clears each ring's INTRL2_1 bit:

	intrl2_1_writel(ring->priv, BIT(ring->index), INTRL2_CPU_CLEAR);

Sources whose NAPI is scheduled are also masked. In that case
bcm_sysport_rx_isr() takes this path:

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

On non-Lite hardware, bcm_sysport_tx_isr() prints "spurious TX interrupt"
in the same way.

While TX is busy, the retry loop in __netpoll_send_skb() calls
netpoll_poll_dev() up to jiffies_to_usecs(1)/USEC_PER_POLL times. Could
that fill the log with unratelimited spurious interrupt warnings? That is
up to 40 at HZ=1000 and 400 at HZ=100 each time txq goes from empty to
busy.

Each warning is also a new printk that netconsole tries to send over the
same congested interface.

Should the polled path skip this warning or ratelimit it, or should the
poll_controller be removed?

>  	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);
>  	}
>  }

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

Re: [PATCH net v2 05/10] 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] The commit message for "net: systemport: Fix RUNT MIB counter
  register offset calculation" says the old code read all 4 RUNT MIB…

--

net: systemport: Fix RUNT MIB counter register offset calculation

This commit changes bcm_sysport_update_mib_counters() so that RUNT MIB
counters are read with an offset of 2 * UMAC_MIB_STAT_OFFSET. This covers
both the RX-to-TX gap and the TX-to-RUNT gap in the UniMAC MIB block.

> 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?

The loop reaches the RUNT entries in bcm_sysport_gstrings_stats after 29
STAT_MIB_RX and 29 STAT_MIB_TX u32 entries, so j is 0xE8 at that point.
With the old offset of 0xC, the four RUNT reads went to:

    UMAC_MIB_START + 0xE8 + 0xC = 0x4F4  rx_runt_pkts
    UMAC_MIB_START + 0xEC + 0xC = 0x4F8  rx_runt_valid_fcs
    UMAC_MIB_START + 0xF0 + 0xC = 0x4FC  rx_runt_inval_fcs_align
    UMAC_MIB_START + 0xF4 + 0xC = 0x500  rx_runt_bytes

The TX-to-RUNT gap covers only 0x4F4-0x4FF, so only the first three reads
fell in the gap.

The fourth read, for rx_runt_bytes, went to 0x500. That is the real
rx_runt_pkts register. So before this fix, ethtool -S reported the runt
packet count as rx_runt_bytes, not a gap value.

Could the commit message say that three counters read gap space and
rx_runt_bytes showed the rx_runt_pkts value? The code change itself looks
correct and matches the 2 * BCMGENET_STAT_OFFSET handling in bcmgenet.

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

Re: [PATCH net v2 06/10] 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: 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] The new minimum length in bcm_sysport_desc_rx() does not
  include ETH_HLEN.
- [Low] The commit message for "net: systemport: Fix potential packet
  length underflow in bcm_sysport_desc_rx()" has an unfinished sentence,…

--

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

This patch adds a lower bound check to bcm_sysport_desc_rx() for the
length reported in the RSB. The length must now cover the Receive Status
Block, the 2 byte pad and, when CRC forwarding is enabled, the FCS.
Shorter frames are dropped and counted as rx_length_errors.

> 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).

[Severity: Low]
This isn't a bug, but the second sentence here is unfinished. It doesn't
say what happens in that case.

Is "a malformed or corrupted frame is received" accurate? rbuf_init()
always enables the RSB and 4 byte alignment:

rbuf_init() {
    ...
	reg |= RBUF_4B_ALGN | RBUF_RSB_EN;
    ...
}

So every length the MAC reports already includes the 8 byte RSB and the
2 pad bytes. The driver also never sets CMD_RUNT_FILTER_DIS, so the
UniMAC runt filter stays on.

It looks like this can only happen if the hardware misreports the length,
not because of a frame on the wire.

> Furthermore, subtracting (sizeof(*rsb) + 2) from 'len' (u16) will
> underflow, resulting in corrupted packet stats and potential
> out-of-bounds operations.

Does "potential out-of-bounds operations" overstate the impact before this
patch? When len < 10, skb_pull() goes through skb_pull_inline():

	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);

so skb->data doesn't move.

The u16 len then wraps to 65526 or more. That inflates processed_bytes,
priv->dim.bytes, ndev->stats.rx_bytes and stats64->rx_bytes.

skb_trim() does nothing. eth_type_trans() reads the RSB as the Ethernet
header, past skb->tail but still inside the 2048 byte RX buffer. Nothing
seems to write out of bounds.

Could the message describe this as a guard against the hardware
misreporting the length? It could also list the actual consequences:
corrupted byte counters and a garbage runt skb passed up the stack.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 9c1b515dc8cc9..ed4337af58da1 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 include ETH_HLEN? As written, a reported len
of 10 to 23 (or 14 to 27 with crc_fwd) passes the check. The rest of
bcm_sysport_desc_rx() then does this:

		skb_put(skb, len);
		...
		skb_pull(skb, sizeof(*rsb) + 2);
		len -= (sizeof(*rsb) + 2);
		...
		if (priv->crc_fwd) {
			skb_trim(skb, len - ETH_FCS_LEN);
		...
		skb->protocol = eth_type_trans(skb, ndev);

After that, skb->len can be anywhere from 0 to 13.

In the call chain eth_type_trans()->eth_skb_pull_mac(), the result of the
pull is ignored:

	struct ethhdr *eth = (struct ethhdr *)skb->data;

	skb_pull_inline(skb, ETH_HLEN);
	return eth;

skb_pull_inline() refuses to pull 14 bytes from the shorter skb, but the
header pointer is still returned. eth_skb_pkt_type() then reads
eth->h_dest, and eth_type_trans() reads eth->h_proto. Both reads are past
skb->tail, in bytes of the RX_BUF_LENGTH buffer that the device didn't
write for this transfer.

Can this set skb->protocol and skb->pkt_type from stale memory? AF_PACKET
listeners see those values as sll_protocol and sll_pkttype.

The runt skb is also counted in rx_packets and rx_bytes and handed to
napi_gro_receive(). Its skb->len is below ETH_HLEN, and skb->data was
never moved past the MAC header.

This is the same kind of hardware misreport the patch guards against. The
eth_type_trans() read past the data is also the only out-of-bounds
operation on this path that the commit message could be referring to.
Would a bound like this close the gap?

	len < sizeof(*rsb) + 2 + ETH_HLEN + (priv->crc_fwd ? ETH_FCS_LEN : 0)

[ ... ]

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

Re: [PATCH net v2 07/10] net: systemport: Fix out-of-bounds array accesses in DSA queue mapping

[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 (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] In bcm_sysport_map_queues()
  (drivers/net/ethernet/broadcom/bcmsysport.c), the new bounds check…
- [Low] The commit message understates the trigger.

Pre-existing issues:
- [Low] bcm_sysport_select_queue() runs in the TX datapath without RTNL.

--

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

This patch adds bounds checks against ARRAY_SIZE(priv->ring_map) in
bcm_sysport_select_queue(), bcm_sysport_map_queues() and
bcm_sysport_unmap_queues(). It also clamps the argument that SYSTEMPORT
Lite passes to netif_set_real_num_tx_queues() so it is never below 1.

> 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.

[Severity: Low]

Is "unusual port or queue configurations" the right trigger to describe
here?

With the in-tree switch drivers, ds->num_tx_queues is at most 8 and
bcm_sf2 user ports are below 9. So the map/unmap index stays under 96.

The read in bcm_sysport_select_queue() looks reachable with ordinary
traffic, though. It decodes queue_mapping for every skb once
netdev_uses_dsa(dev) is true. But only brcm_tag_xmit_ll() in
net/dsa/tag_brcm.c writes the port/queue encoding:

    skb_set_queue_mapping(skb, BRCM_TAG_SET_PORT_QUEUE(dp->index, queue));

Other skbs sent on the conduit carry unrelated values. Two examples:

  - forwarded skbs that still have the ingress queue from
    skb_record_rx_queue() (rxq + 1), e.g. from multiqueue tun/veth or
    NICs with many RX queues
  - tc-BPF writes to __sk_buff->queue_mapping

If the low byte of queue_mapping is 96 or more, the lookup reads past the
end of ring_map[], which is the last member of priv. The loaded value is
then dereferenced as tx_ring->index.

The code change covers this case. Could the commit message say that
ordinary traffic can hit it, so stable backporters can judge the urgency?

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index ed4337af58da1..4efcefd33b781 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c

[ ... ]

> @@ -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: Low]

This is a pre-existing issue and was not introduced by this patch.
bcm_sysport_select_queue() runs in the TX path without RTNL. It reads
priv->per_port_num_tx_queues and priv->ring_map[index] with plain loads.

At the same time, bcm_sysport_map_queues() and bcm_sysport_unmap_queues()
write those fields with plain stores. They run from the NETDEV_CHANGEUPPER
notifier and hold only RTNL. For example, dsa_user_destroy() unlinks
without first draining in-flight transmits:

    netif_carrier_off(user_dev);
    rtnl_lock();
    netdev_upper_dev_unlink(conduit, user_dev);

The new index >= ARRAY_SIZE() check and the existing !tx_ring check both
assume each value is read only once. Without READ_ONCE()/WRITE_ONCE(), can
the compiler reload per_port_num_tx_queues or ring_map[index] after the
check?

Would a follow-up be worthwhile that adds READ_ONCE() in
bcm_sysport_select_queue() and WRITE_ONCE() in the map/unmap paths?

[ ... ]

> @@ -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 qp + port * num_tx_queues is out of range, does this leave the ring
half claimed? By this point ring->switch_queue, ring->switch_port and
ring->inspect are already set, and qp++ still runs. Only the ring_map[]
store is skipped.

The index goes out of range when num_tx_queues is above 8, or when the
port index is 12 or more with 8 queues.

On every open, bcm_sysport_init_tx_ring() programs the hardware from
those fields:

	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 switch port/queue still flow-controls the ring. The later commit
"net: systemport: Update TDMA queue mapping dynamically on changeupper"
also has bcm_sysport_map_queues() program this right away, with
RING_IGNORE_STATUS cleared.

However, no ring_map[] entry points at the ring. For that (port, queue),
bcm_sysport_select_queue() hits the new bounds check and falls back to
netdev_pick_tx(). That can hash onto any ring, including rings that
inspect other ports/queues.

Could this bring back the flow-control cross-talk and head-of-line
blocking that ring_map[] exists to prevent?

While the port stays linked, later bcm_sysport_map_queues() calls for
other ports also skip the claimed ring because of this check:

		if (ring->inspect)
			continue;

Would it be better to check the index before claiming the ring, and then
skip or reject the mapping?

The commit message also doesn't mention this half-claimed state. It is
still present at the end of the series.

[ ... ]

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

Re: [PATCH net v2 09/10] 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 (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).

- [Medium] The unbalanced wol_clk disable on resume that this patch
  targets can still happen when enabling the clock fails.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is serious.
- [Medium] The timeout check in bcm_sysport_suspend_to_wol() is inverted.

--

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

bcm_sysport_suspend() prepares and enables the Wake-on-LAN clock only
when both device_may_wakeup() and priv->wolopts are true. However,
bcm_sysport_resume() disabled it whenever priv->wolopts was set. This
patch makes resume use the same check as suspend so that
clk_disable_unprepare() is not called unbalanced.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 11b2cb4cc7927..dedd49ad6c4ef 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2799,7 +2799,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]
Does this fully balance the wol_clk enable and disable? In
bcm_sysport_suspend(), the return value of clk_prepare_enable() is
ignored:

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

When clk_prepare_enable() fails, it undoes its own prepare, which leaves
wol_clk neither prepared nor enabled. Suspend still returns 0 as long as
bcm_sysport_suspend_to_wol() succeeds.

On resume, the new check above is true again. clk_disable_unprepare()
then runs on a clock that was never enabled. The common clock framework
would warn with "already disabled" and "already unprepared". WoL would
also be armed without its clock for that cycle.

Should the return value be checked in suspend, and then either skip WoL
or record in a flag that resume tests whether the clock was enabled?

>  
>  	umac_reset(priv);

[Severity: High]
This isn't a bug introduced by this patch, but there is a related wol_clk
imbalance in the suspend error path. When bcm_sysport_suspend_to_wol()
returns -ETIMEDOUT, bcm_sysport_suspend() has already enabled wol_clk. It
still gates priv->clk and then returns the error:

    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;

After a failed ->suspend(), the PM core leaves dev->power.is_suspended
false and does not call bcm_sysport_resume(). The wol_clk enable leaks,
and every later cycle enables it again but disables it only once.

The error path in bcm_sysport_suspend_to_wol() only calls
mpd_enable_set(priv, false). RBUF_ACPI_EN and the RXCHK filter match
bits stay set.

The device is also left with the core clock off, the netdev detached,
the rings freed and NAPI disabled, while netif_running() is still true.
Can the next suspend or interface down then hang?

    bcm_sysport_suspend() or bcm_sysport_stop()
      bcm_sysport_netif_stop()
        napi_disable(&priv->napi)

napi_disable() on a NAPI that is already disabled waits for
NAPIF_STATE_SCHED to clear, with no timeout. Nothing will ever clear it.

Right now -ETIMEDOUT is only returned when RBUF_WOL_MODE appears on the
very last poll (see below). A corrected timeout check would send every
real timeout down this path.

Should this error path undo the wol_clk enable and restore the device
state before it returns?

[Severity: Medium]
This is also a pre-existing issue, but is the timeout check in
bcm_sysport_suspend_to_wol() inverted?

    unsigned int timeout = 1000;
    ...
    do {
        reg = rbuf_readl(priv, RBUF_STATUS);
        if (reg & RBUF_WOL_MODE)
            break;

        udelay(10);
    } while (timeout-- > 0);

    /* Do not leave the UniMAC RBUF matching only MPD packets */
    if (!timeout) {

On a real timeout, the final "0 > 0" test fails and the post-decrement
wraps timeout to UINT_MAX, so !timeout is false. MPD stays enabled,
UniMAC RX is turned on, and 0 is returned even though RBUF never entered
WoL mode.

It also goes wrong the other way. If RBUF_WOL_MODE is seen on the last
iteration, the loop was entered with timeout == 0 and breaks with it
still 0. A successful WoL entry is then reported as -ETIMEDOUT, which
takes the failed-suspend path described above.

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

Re: [PATCH net v2 10/10] net: systemport: Update TDMA queue mapping dynamically on changeupper

[netdev-bot+sashiko]

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

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

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 patch programs RING_IGNORE_STATUS one way in the new
  runtime paths and leaves it unchanged in the deferred path, so the…
- [Low] In bcm_sysport_map_queues(), the new TDMA_DESC_RING_MAPPING write
  masks qp with RING_QID_MASK but ORs in `port << RING_PORT_ID_SHIFT`…
- [Low] The new MMIO in bcm_sysport_map_queues() and
  bcm_sysport_unmap_queues() (tdma_readl/tdma_writel on…

--

net: systemport: Update TDMA queue mapping dynamically on changeupper

This commit makes bcm_sysport_map_queues() and bcm_sysport_unmap_queues()
write the TDMA_DESC_RING_MAPPING registers directly when the SYSTEMPORT
interface is running. Before this, those functions only updated the
software mapping, and the hardware was programmed later from
bcm_sysport_init_tx_ring().

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index dedd49ad6c4ef..ca1b86953dba0 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)) {
> +			reg = tdma_readl(priv, TDMA_DESC_RING_MAPPING(q));

[Severity: Low]
Does netif_running() guarantee that priv->clk is enabled here? The same
question applies to the new block in bcm_sysport_unmap_queues().

bcm_sysport_suspend() calls netif_device_detach() and
clk_disable_unprepare(priv->clk) but leaves __LINK_STATE_START set.
bcm_sysport_resume() can then fail in several ways: clk_prepare_enable()
fails, bcm_sysport_init_tx_ring() or bcm_sysport_init_rx_ring() fails, or
the RDMA/TDMA enable times out. In each case it returns with the clock
disabled and does not call netif_device_attach():

bcm_sysport_resume() {
    ...
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;
}

netif_running() is still true in that state. A later NETDEV_CHANGEUPPER,
for example from unbinding bcm_sf2, would reach
bcm_sysport_netdevice_event()->bcm_sysport_unmap_queues(). That would do
this read-modify-write on a TDMA block whose clock is off. Before this
patch the notifier path only touched software state.

Would netif_running(dev) && netif_device_present(dev) be a tighter guard?

bcm_sysport_stop() already does umac/tdma MMIO in the same failed-resume
state, so these new accesses add only a little extra risk.

> +			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: Low]
qp is masked with RING_QID_MASK here, but port is not masked with
RING_PORT_ID_MASK. PORT_ID is a 3-bit field, and control bits sit
directly above it:

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

Could a dp->index of 8 or higher spill into RING_IGNORE_STATUS and
RING_FAILOVER_EN?

This doesn't look reachable with bcm_sf2. Its user ports are 0-7, and the
CPU port (8) has no DSA user netdev, so dsa_user_dev_check() filters it
out. bcm_sysport_init_tx_ring() also already uses the same unmasked
encoding.

Would it still make sense to use (port & RING_PORT_ID_MASK) here, the
same way qp is masked?

> +			tdma_writel(priv, reg, TDMA_DESC_RING_MAPPING(q));
> +		}
>  		qp++;
>  	}

[ ... ]

> @@ -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: Medium]
Can RING_IGNORE_STATUS stay set on a ring that is later inspected again?

The new running path in bcm_sysport_map_queues() clears
RING_IGNORE_STATUS. The deferred path in bcm_sysport_init_tx_ring() does
not clear it in the inspect branch:

bcm_sysport_init_tx_ring() {
    ...
	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));
    ...
}

Neither bcm_sysport_stop() nor bcm_sysport_open() resets
TDMA_DESC_RING_MAPPING before this read-modify-write. Consider this
sequence:

1. Unbind the DSA switch while SYSTEMPORT is up. This new code sets
   RING_IGNORE_STATUS.
2. Bring the SYSTEMPORT interface down.
3. Rebind the switch. bcm_sysport_map_queues() sets ring->inspect = true
   but skips the hardware write because netif_running() is false.
4. Bring the interface up. bcm_sysport_init_tx_ring() writes QID and
   PORT_ID but keeps the old RING_IGNORE_STATUS bit.

The inspected ring would then ignore the switch port/queue status. That
seems to turn off the queue inspection flow control this mapping exists
for; RING_IGNORE_STATUS was introduced in commit 3ded76a8ff53 to stop
rings from inspecting switch queues. The same state can be reached by
opening with no switch attached, closing, attaching the switch, and
opening again.

So the final register value now depends on whether the switch was
attached before or after open. Should bcm_sysport_init_tx_ring() also
clear RING_IGNORE_STATUS in its inspect branch?

> +		}
>  	}

[ ... ]

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