[PATCH net v2 1/3] can: dev: prevent bus-off recovery when the interface is not running

[PATCH net v2 1/3] can: dev: prevent bus-off recovery when the interface is not running

[Cunhao Lu]

The networking core clears __LINK_STATE_START before calling ndo_stop(),
but CAN bus-off recovery neither checks this state before queuing work
nor before calling the driver's restart callback. Recovery can therefore
be queued or started after the interface has begun closing, potentially
re-enabling hardware while the driver is tearing it down.

Check netif_running() in can_restart() before flushing echo skbs, sending
the restart notification or enabling the carrier. Keep the existing
unsupported-operation check first. In can_bus_off(), only schedule
recovery for a running interface and make the log message reflect that
decision. Keep reporting bus-off and disabling the carrier.

These checks do not wait for recovery that has already passed the check,
nor serialize the check with work submission. Drivers still need to
cancel restart work before tearing down resources and quiesce its
producers before their final cleanup.

Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
 drivers/net/can/dev/dev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 769745e22a3c7..92492c503446e 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -198,6 +198,9 @@ static int can_restart(struct net_device *dev)
 	if (!priv->do_set_mode)
 		return -EOPNOTSUPP;
 
+	if (!netif_running(dev))
+		return -ENETDOWN;
+
 	if (netif_carrier_ok(dev))
 		netdev_err(dev, "Attempt to restart for bus-off recovery, but carrier is OK?\n");
 
@@ -264,8 +267,9 @@ int can_restart_now(struct net_device *dev)
 void can_bus_off(struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
+	const bool restart = priv->restart_ms && netif_running(dev);
 
-	if (priv->restart_ms)
+	if (restart)
 		netdev_info(dev, "bus-off, scheduling restart in %d ms\n",
 			    priv->restart_ms);
 	else
@@ -273,7 +277,7 @@ void can_bus_off(struct net_device *dev)
 
 	netif_carrier_off(dev);
 
-	if (priv->restart_ms)
+	if (restart)
 		schedule_delayed_work(&priv->restart_work,
 				      msecs_to_jiffies(priv->restart_ms));
 }

-- 
2.34.1

[PATCH net v2 2/3] can: dev: add close_candev_cancel_restart_sync()

[Cunhao Lu]

Drivers may need to wait for bus-off recovery before tearing down the
IRQ and hardware resources used by their restart callback. Calling
close_candev() at that point also flushes echo skbs, which belongs after
the driver has quiesced the users of those buffers.

Add close_candev_cancel_restart_sync() in linux/can/dev.h so drivers can
cancel the core restart work separately, without accessing can_priv's
work item directly. The helper only cancels restart work; callers must
still stop its producers and call close_candev() for the final cleanup.

Use the helper in close_candev(), preserving its existing cancellation
and echo cleanup order.

Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
 drivers/net/can/dev/dev.c |  4 +---
 include/linux/can/dev.h   | 12 ++++++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 92492c503446e..52e8c4854ae0d 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -534,9 +534,7 @@ EXPORT_SYMBOL_GPL(of_can_transceiver);
  */
 void close_candev(struct net_device *dev)
 {
-	struct can_priv *priv = netdev_priv(dev);
-
-	cancel_delayed_work_sync(&priv->restart_work);
+	close_candev_cancel_restart_sync(dev);
 	can_flush_echo_skb(dev);
 }
 EXPORT_SYMBOL_GPL(close_candev);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 6d0710d6f571d..a277d12332f15 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -22,6 +22,7 @@
 #include <linux/can/skb.h>
 #include <linux/ethtool.h>
 #include <linux/netdevice.h>
+#include <linux/workqueue.h>
 
 /*
  * CAN mode
@@ -115,6 +116,17 @@ struct can_priv *safe_candev_priv(struct net_device *dev);
 
 int open_candev(struct net_device *dev);
 void close_candev(struct net_device *dev);
+
+/* Cancel bus-off restart before tearing down resources used by the restart
+ * callback. The driver must still call close_candev() for the final cleanup.
+ */
+static inline void close_candev_cancel_restart_sync(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	cancel_delayed_work_sync(&priv->restart_work);
+}
+
 void can_set_default_mtu(struct net_device *dev);
 void can_set_cap_info(struct net_device *dev);
 int __must_check can_set_static_ctrlmode(struct net_device *dev,

-- 
2.34.1

[PATCH net v2 3/3] can: rockchip_canfd: cancel bus-off restart before closing the device

[Cunhao Lu]

The close path cancels restart_work through close_candev() only after
freeing the IRQ, disabling RX offload and stopping the controller. A
running restart can re-enable the controller and its interrupts after
IRQ teardown, or restart timestamp work after it has been stopped and
before runtime suspend removes the controller clocks.

Call close_candev_cancel_restart_sync() at the start of ndo_stop(), while
the resources needed by a running restart are still available. The core
netif_running() check prevents a late restart from entering the driver,
while this cancellation waits for one that has already passed the check.
Keep close_candev() after IRQ teardown for the final cancellation and
echo cleanup, including work queued by an IRQ racing with the first
cancellation.

Exclude the IRQ handler while reinitializing hardware in the restart
callback: the handler continues processing after scheduling recovery,
and a shared interrupt can also invoke it while this controller's
interrupts are masked. Keep the shared IRQ enabled while the close path
waits for restart work to finish.

Fixes: ff60bfbaf67f ("can: rockchip_canfd: add driver for Rockchip CAN-FD controller")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
 drivers/net/can/rockchip/rockchip_canfd-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 37c1c22c40c9..fb8f6b067502 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -356,8 +356,10 @@ static int rkcanfd_set_mode(struct net_device *ndev,
 
 	switch (mode) {
 	case CAN_MODE_START:
+		disable_irq(ndev->irq);
 		rkcanfd_chip_start(priv);
 		rkcanfd_chip_interrupts_enable(priv);
+		enable_irq(ndev->irq);
 		netif_wake_queue(ndev);
 		break;
 
@@ -758,6 +760,7 @@ static int rkcanfd_stop(struct net_device *ndev)
 {
 	struct rkcanfd_priv *priv = netdev_priv(ndev);
 
+	close_candev_cancel_restart_sync(ndev);
 	netif_stop_queue(ndev);
 
 	rkcanfd_chip_interrupts_disable(priv);

-- 
2.34.1

Re: [PATCH net v2 3/3] can: rockchip_canfd: cancel bus-off restart before closing the device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: rockchip_canfd: disable_irq() called on shared IRQ
--

commit 580402c6870b4c8a3e1db89526b629150785fd7a
Author: Cunhao Lu <1579567540@qq.com>

can: rockchip_canfd: cancel bus-off restart before closing the device

This commit cancels restart_work during the close path before closing the device
to prevent a running restart from re-enabling the controller. It also uses
disable_irq() during the restart callback to prevent the IRQ handler from
running while the hardware is being reinitialized.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -356,8 +356,10 @@ static int rkcanfd_set_mode(struct net_device *ndev,
>  
>  	switch (mode) {
>  	case CAN_MODE_START:
> +		disable_irq(ndev->irq);

[Severity: Medium]
Is it safe to call disable_irq() here on a shared interrupt line?

In rkcanfd_open(), the IRQ is explicitly registered as shared:

    err = request_irq(ndev->irq, rkcanfd_irq, IRQF_SHARED, ndev->name, priv);

Masking a shared IRQ line globally delays or drops interrupts for all other
devices sharing that line.

Could this driver instead use a software flag paired with synchronize_irq()
or spinlocks to exclude the handler, rather than disabling the physical line?

>  		rkcanfd_chip_start(priv);
>  		rkcanfd_chip_interrupts_enable(priv);
> +		enable_irq(ndev->irq);
>  		netif_wake_queue(ndev);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_196A2433801631CD2FA42998465E26316105@qq.com?part=3

Re: [PATCH net v2 3/3] can: rockchip_canfd: cancel bus-off restart before closing the device

[Cunhao Lu]

Hi,

> [Severity: Medium]
> Is it safe to call disable_irq() here on a shared interrupt line?
> 
> In rkcanfd_open(), the IRQ is explicitly registered as shared:
> 
>     err = request_irq(ndev->irq, rkcanfd_irq, IRQF_SHARED, ndev->name, priv);
> 
> Masking a shared IRQ line globally delays or drops interrupts for all other
> devices sharing that line.
> 
> Could this driver instead use a software flag paired with synchronize_irq()
> or spinlocks to exclude the handler, rather than disabling the physical line?
> 
> >   rkcanfd_chip_start(priv);
> >   rkcanfd_chip_interrupts_enable(priv);
> > + enable_irq(ndev->irq);
> >   netif_wake_queue(ndev);
> >   break;

I agree that disable_irq() affects all users of the shared IRQ and
should be avoided here.

The intent was to exclude the IRQ handler while reinitializing the
controller. Masking the controller's own interrupt sources and calling
synchronize_irq() alone would not be sufficient: another device on the
shared line could still invoke rkcanfd_irq() during reinitialization.

I will fix this issue in v3.

Regards,
Cunhao Lu

Re: [PATCH net v2 1/3] can: dev: prevent bus-off recovery when the interface is not running

[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] Split-frame drop accounting in stmmac_rx_zc() is inconsistent
  per frame and per core.

Pre-existing issues:
- [High] stmmac_dispatch_skb_zc() (stmmac_main.c:5533) sets
  `skb->ip_summed = CHECKSUM_UNNECESSARY` for every IP frame when RX COE…
- [Medium] priv->xstats.rx_dropped and priv->xstats.rx_errors are device-
  wide unsigned longs.

--

net: stmmac: drop split frames on AF_XDP zero-copy RX

This patch fixes stmmac_rx_zc() so that split frames are drained as
errored, and the last fragment is no longer handed to XDP as a fresh
frame. It also changes the legacy normal and enhanced descriptor
callbacks to report rx_not_ls together with discard_frame for non-last
descriptors.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> index 0512536012251..297e2428a8f9b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> @@ -184,7 +184,7 @@ static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
>  
>  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
>  		x->rx_length++;
> -		return discard_frame;
> +		return discard_frame | rx_not_ls;
>  	}
>  
>  	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> index c4b613564f87c..d19bf7ca4db38 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> @@ -70,7 +70,7 @@ static int ndesc_get_rx_status(struct stmmac_extra_stats *x,
>  
>  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
>  		x->rx_length++;
> -		return discard_frame;
> +		return discard_frame | rx_not_ls;
>  	}
>  
>  	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f0835be2f65a7..10ea72cbf889a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5695,18 +5695,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats,
>  						  rx_q->dma_erx + entry);
> -		if (unlikely(status == discard_frame)) {
> -			xsk_buff_free(buf->xdp);
> -			buf->xdp = NULL;
> -			dirty++;
> +
> +		if (unlikely(status & discard_frame)) {
>  			error = 1;
>  			if (!priv->hwts_rx_en)
>  				rx_errors++;
>  		}
>  
> -		if (unlikely(error && (status & rx_not_ls)))
> -			goto read_again;
>  		if (unlikely(error)) {
> +			xsk_buff_free(buf->xdp);
> +			buf->xdp = NULL;
> +			dirty++;
> +
> +			if (status & rx_not_ls)
> +				goto read_again;
> +
>  			count++;
>  			continue;
>  		}
> @@ -5715,8 +5718,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (likely(status & rx_not_ls)) {
>  			xsk_buff_free(buf->xdp);
>  			buf->xdp = NULL;
> +			rx_dropped++;
> +			error = 1;

[Severity: Medium]

Is the split-frame drop accounting here consistent across descriptor
formats?

The legacy callbacks now return discard_frame | rx_not_ls for every
non-last descriptor. So the discard branch above runs first for each of
those descriptors, and there is no !error guard:

    if (unlikely(status & discard_frame)) {
        error = 1;
        if (!priv->hwts_rx_en)
            rx_errors++;
    }

On dwmac100/dwmac1000, a frame spanning N descriptors would then add N-1
to rx_errors. It adds 0 to rx_dropped, while count / rx_pkt_n goes up
only once for the frame. The new rx_dropped++ in this 1:1 block can never
be reached on those cores.

On dwmac4 (and similarly xgmac), dwmac4_wrback_get_rx_status() returns a
bare rx_not_ls for non-last descriptors:

	if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
		return rx_not_ls;

That means this block does rx_dropped++ and sets error = 1. If the last
descriptor has RDES3_ERROR_SUMMARY set, the discard branch also does
rx_errors++. The same dropped frame then shows up in both rx_dropped and
rx_errors.

Later in the series, stmmac_rx() is changed to count only once, using
(!error && (status & discard_frame)). Should stmmac_rx_zc() get the same
treatment, so each dropped frame is counted once in either rx_errors or
rx_dropped?

>  			dirty++;
> -			count++;
>  			goto read_again;
>  		}
>  

[Severity: Medium]

This is a pre-existing issue and was not introduced by this patch. The
new rx_dropped++ feeds into an unsynchronized accumulation at the end of
stmmac_rx_zc():

	priv->xstats.rx_dropped += rx_dropped;
	priv->xstats.rx_errors += rx_errors;

These are device-wide unsigned longs. They are updated with a plain +=
from per-queue NAPI contexts that can run at the same time on different
CPUs. stmmac_rx() does the same thing, and stmmac_get_stats64() reads
them without any locking.

Can concurrent NAPI polls on different queues lose increments to these
counters?

[Severity: High]

This isn't a bug introduced by this patch, but it is in the same receive
path. On XDP_PASS, stmmac_rx_zc() calls stmmac_dispatch_skb_zc() without
passing status. That function marks every IP frame as verified when RX
COE is enabled:

stmmac_dispatch_skb_zc() {
    ...
	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
		skb_checksum_none_assert(skb);
	else
		skb->ip_summed = CHECKSUM_UNNECESSARY;
    ...
}

On dwmac4, dwmac4_wrback_get_rx_status() reports L4 payload checksum
failures only as csum_none, without discard_frame:

	if (rdes1 & RDES1_IP_PAYLOAD_ERROR) {
		x->ip_payload_err++;
		ret |= csum_none;
	}

Also, dwmac4_dma_rx_chan_op_mode() sets MTL_OP_MODE_DIS_TCP_EF, so the
hardware forwards these frames instead of dropping them.

Suppose a frame with a bad TCP/UDP checksum arrives on a ZC queue and
the XDP program passes it. Could it reach the stack as
CHECKSUM_UNNECESSARY and hand corrupted payload to applications?

The non-ZC stmmac_rx() handles this case:

	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb) ||
	    (status & csum_none))
		skb_checksum_none_assert(skb);

This still seems to be the case at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_196A2433801631CD2FA42998465E26316105%40qq.com

Re: [PATCH net v2 3/3] can: rockchip_canfd: cancel bus-off restart before closing the device

[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 · 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] can: rockchip_canfd: disable_irq() called on shared IRQ

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the restart
  callback the patch modifies re-initializes a delayed_work that may…

--

can: rockchip_canfd: cancel bus-off restart before closing the device

This commit calls close_candev_cancel_restart_sync() at the start of
rkcanfd_stop(), so a running bus-off restart finishes while the IRQ and
other resources are still available. It also brackets the hardware
reinitialization in rkcanfd_set_mode() with disable_irq()/enable_irq().
That keeps the IRQ handler out while the controller is restarted.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 37c1c22c40c9f..fb8f6b0675022 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -356,8 +356,10 @@ static int rkcanfd_set_mode(struct net_device *ndev,
>  
>  	switch (mode) {
>  	case CAN_MODE_START:
> +		disable_irq(ndev->irq);
>  		rkcanfd_chip_start(priv);

[Severity: Medium]
rkcanfd_open() requests ndev->irq with IRQF_SHARED, and the commit
message itself relies on the line being shared. disable_irq() masks
the whole line at the irqchip, not only this controller's source.
It also waits in synchronize_irq() for the handlers of every other
device on that line.

Every device sharing the line therefore loses interrupt delivery for
as long as rkcanfd_chip_start() and rkcanfd_chip_interrupts_enable()
run. That window covers:

- the reset and work mode transitions
- FIFO setup
- bittiming programming
- timestamp init and start

The window can be opened from outside the box. Another node on the
CAN bus can drive this controller into bus-off. With restart-ms
configured, restart_work then runs rkcanfd_set_mode(), and a manual
can_restart_now() takes the same path. Each restart stalls the
unrelated devices sharing the IRQ.

Drivers on a shared IRQ are generally expected not to use
disable_irq() for their own exclusion, for exactly this reason.

Would it be possible to exclude rkcanfd_irq() with a real lock
instead? For example, the handler could take a spinlock, and
spin_lock_irqsave() could be held around the chip_start and
interrupts_enable sequence here.

That only works if everything reached from rkcanfd_chip_start() is
safe in atomic context. Can you confirm that? Or is there a reason
the restart path has to be able to sleep while the handler is held
off?

I would rather not see a software flag checked in the handler for
this. Without a lock it does not actually exclude a handler that
has already passed the check.

[Severity: High]
This issue was not introduced by this patch. It sits in the restart path
the patch modifies, though. Can rkcanfd_chip_start() here re-initialize
priv->timestamp while its timer is still queued?

On bus-off, the IRQ path stops the timestamp work without waiting for it:

rkcanfd_irq()
  rkcanfd_handle_state_error_int()
    rkcanfd_chip_stop(priv, CAN_STATE_BUS_OFF)
      rkcanfd_timestamp_stop()
        cancel_delayed_work(&priv->timestamp);

The work callback always re-arms itself:

drivers/net/can/rockchip/rockchip_canfd-timestamp.c:rkcanfd_timestamp_work() {
    ...
	timecounter_read(&priv->tc);

	schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
}

Bus-off may be handled while rkcanfd_timestamp_work() is running, either on
another CPU or because the IRQ preempted it on the same CPU. In that case
cancel_delayed_work() finds nothing pending, and the callback then re-queues
itself. From then on the work keeps re-arming.

When the restart runs (restart_work or can_restart_now()), it goes through:

rkcanfd_set_mode()
  rkcanfd_chip_start()
    rkcanfd_timestamp_init()
      INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
    rkcanfd_timestamp_start()
      timecounter_init(&priv->tc, ...);
      schedule_delayed_work(&priv->timestamp, ...);

INIT_DELAYED_WORK() clears the PENDING bit and the timer's linkage while the
timer_list is still in a timer-wheel bucket. The following
schedule_delayed_work() then sees the work as idle and enqueues the same
timer_list a second time. Can that corrupt the timer wheel hlist?
timecounter_init() also writes priv->tc while the old work may still be
using it.

The new disable_irq()/enable_irq() pair keeps the IRQ handler out. It does
not stop the timestamp work. One option is to cancel priv->timestamp
synchronously in this sleepable restart path before re-initializing it.
Another is to call INIT_DELAYED_WORK() once at probe or open time rather
than on every rkcanfd_chip_start().

>  		rkcanfd_chip_interrupts_enable(priv);
> +		enable_irq(ndev->irq);
>  		netif_wake_queue(ndev);
>  		break;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_196A2433801631CD2FA42998465E26316105%40qq.com

Re: [PATCH net v2 1/3] can: dev: prevent bus-off recovery when the interface is not running

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 7978 bytes --]

Hello,

there was a hickup with the sashiko bot.

It seems the review for patch:

| https://lore.kernel.org/all/20261014-stmmac-rx-mb-v7-2-5d2d70f052cd@oss.qualcomm.com/

was send to as a rely to this patch:

| https://lore.kernel.org/all/tencent_196A2433801631CD2FA42998465E26316105@qq.com/

regards,
Marc

On 15.10.2026 16:37:52, netdev-bot+sashiko@kernel.org wrote:
> 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] Split-frame drop accounting in stmmac_rx_zc() is inconsistent
>   per frame and per core.
>
> Pre-existing issues:
> - [High] stmmac_dispatch_skb_zc() (stmmac_main.c:5533) sets
>   `skb->ip_summed = CHECKSUM_UNNECESSARY` for every IP frame when RX COE…
> - [Medium] priv->xstats.rx_dropped and priv->xstats.rx_errors are device-
>   wide unsigned longs.
>
> --
>
> net: stmmac: drop split frames on AF_XDP zero-copy RX
>
> This patch fixes stmmac_rx_zc() so that split frames are drained as
> errored, and the last fragment is no longer handed to XDP as a fresh
> frame. It also changes the legacy normal and enhanced descriptor
> callbacks to report rx_not_ls together with discard_frame for non-last
> descriptors.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > index 0512536012251..297e2428a8f9b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > @@ -184,7 +184,7 @@ static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
> >
> >  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
> >  		x->rx_length++;
> > -		return discard_frame;
> > +		return discard_frame | rx_not_ls;
> >  	}
> >
> >  	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> > index c4b613564f87c..d19bf7ca4db38 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> > @@ -70,7 +70,7 @@ static int ndesc_get_rx_status(struct stmmac_extra_stats *x,
> >
> >  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
> >  		x->rx_length++;
> > -		return discard_frame;
> > +		return discard_frame | rx_not_ls;
> >  	}
> >
> >  	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index f0835be2f65a7..10ea72cbf889a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5695,18 +5695,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
> >  		if (priv->extend_desc)
> >  			stmmac_rx_extended_status(priv, &priv->xstats,
> >  						  rx_q->dma_erx + entry);
> > -		if (unlikely(status == discard_frame)) {
> > -			xsk_buff_free(buf->xdp);
> > -			buf->xdp = NULL;
> > -			dirty++;
> > +
> > +		if (unlikely(status & discard_frame)) {
> >  			error = 1;
> >  			if (!priv->hwts_rx_en)
> >  				rx_errors++;
> >  		}
> >
> > -		if (unlikely(error && (status & rx_not_ls)))
> > -			goto read_again;
> >  		if (unlikely(error)) {
> > +			xsk_buff_free(buf->xdp);
> > +			buf->xdp = NULL;
> > +			dirty++;
> > +
> > +			if (status & rx_not_ls)
> > +				goto read_again;
> > +
> >  			count++;
> >  			continue;
> >  		}
> > @@ -5715,8 +5718,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
> >  		if (likely(status & rx_not_ls)) {
> >  			xsk_buff_free(buf->xdp);
> >  			buf->xdp = NULL;
> > +			rx_dropped++;
> > +			error = 1;
>
> [Severity: Medium]
>
> Is the split-frame drop accounting here consistent across descriptor
> formats?
>
> The legacy callbacks now return discard_frame | rx_not_ls for every
> non-last descriptor. So the discard branch above runs first for each of
> those descriptors, and there is no !error guard:
>
>     if (unlikely(status & discard_frame)) {
>         error = 1;
>         if (!priv->hwts_rx_en)
>             rx_errors++;
>     }
>
> On dwmac100/dwmac1000, a frame spanning N descriptors would then add N-1
> to rx_errors. It adds 0 to rx_dropped, while count / rx_pkt_n goes up
> only once for the frame. The new rx_dropped++ in this 1:1 block can never
> be reached on those cores.
>
> On dwmac4 (and similarly xgmac), dwmac4_wrback_get_rx_status() returns a
> bare rx_not_ls for non-last descriptors:
>
> 	if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
> 		return rx_not_ls;
>
> That means this block does rx_dropped++ and sets error = 1. If the last
> descriptor has RDES3_ERROR_SUMMARY set, the discard branch also does
> rx_errors++. The same dropped frame then shows up in both rx_dropped and
> rx_errors.
>
> Later in the series, stmmac_rx() is changed to count only once, using
> (!error && (status & discard_frame)). Should stmmac_rx_zc() get the same
> treatment, so each dropped frame is counted once in either rx_errors or
> rx_dropped?
>
> >  			dirty++;
> > -			count++;
> >  			goto read_again;
> >  		}
> >
>
> [Severity: Medium]
>
> This is a pre-existing issue and was not introduced by this patch. The
> new rx_dropped++ feeds into an unsynchronized accumulation at the end of
> stmmac_rx_zc():
>
> 	priv->xstats.rx_dropped += rx_dropped;
> 	priv->xstats.rx_errors += rx_errors;
>
> These are device-wide unsigned longs. They are updated with a plain +=
> from per-queue NAPI contexts that can run at the same time on different
> CPUs. stmmac_rx() does the same thing, and stmmac_get_stats64() reads
> them without any locking.
>
> Can concurrent NAPI polls on different queues lose increments to these
> counters?
>
> [Severity: High]
>
> This isn't a bug introduced by this patch, but it is in the same receive
> path. On XDP_PASS, stmmac_rx_zc() calls stmmac_dispatch_skb_zc() without
> passing status. That function marks every IP frame as verified when RX
> COE is enabled:
>
> stmmac_dispatch_skb_zc() {
>     ...
> 	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
> 		skb_checksum_none_assert(skb);
> 	else
> 		skb->ip_summed = CHECKSUM_UNNECESSARY;
>     ...
> }
>
> On dwmac4, dwmac4_wrback_get_rx_status() reports L4 payload checksum
> failures only as csum_none, without discard_frame:
>
> 	if (rdes1 & RDES1_IP_PAYLOAD_ERROR) {
> 		x->ip_payload_err++;
> 		ret |= csum_none;
> 	}
>
> Also, dwmac4_dma_rx_chan_op_mode() sets MTL_OP_MODE_DIS_TCP_EF, so the
> hardware forwards these frames instead of dropping them.
>
> Suppose a frame with a bad TCP/UDP checksum arrives on a ZC queue and
> the XDP program passes it. Could it reach the stack as
> CHECKSUM_UNNECESSARY and hand corrupted payload to applications?
>
> The non-ZC stmmac_rx() handles this case:
>
> 	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb) ||
> 	    (status & csum_none))
> 		skb_checksum_none_assert(skb);
>
> This still seems to be the case at the end of the series.
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_196A2433801631CD2FA42998465E26316105%40qq.com
>
>

--
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]