[PATCH net v9 00/10] can: fix Rockchip TX stalls and bus-off restart/close races

[PATCH net v9 00/10] can: fix Rockchip TX stalls and bus-off restart/close races

[Cunhao Lu via B4 Relay]

On a production robot using a LubanCat-5IO with an RK3588, the CAN TX
queue remained stopped while RX continued, and subsequent sends returned
ENOBUFS. The interface still reported ERROR-ACTIVE, with neither a
hardware bus-off condition nor a pending TX request.

During CAN traffic, the driver also repeatedly reported an inconsistent
TX echo state:

  rockchip_canfd fea60000.can can0: rkcanfd_tx_tail_is_eff:
  echo_skb[0]=NULL tx_head=0x00060f7d tx_tail=0x00060f7c

The Rockchip CAN-FD driver completes transmissions by matching
self-received frames with entries in the echo skb ring. A race between its
transmit and completion paths can expose a partially updated echo entry,
dereference an skb queued for NAPI, or stop the TX queue without a later
completion to wake it.

While testing the echo-loss fix with fault injection, I discovered a
separate race between bus-off restart and device close.

Rockchip currently cancels restart_work in close_candev(), after freeing
the IRQ, disabling RX offload and stopping the controller. A concurrent
restart can re-enable the controller and interrupts after IRQ teardown,
and requeue timestamp work that accesses registers after runtime suspend
has disabled the controller clocks.

The CAN core skips scheduling or starting recovery when its
netif_running() check observes the interface down. A new
close_candev_cancel_restart_sync() helper lets Rockchip wait for any
restart already in progress before tearing down resources.
The final close_candev() call retains cancellation and echo cleanup.

Rockchip restart masks the device's interrupts and drains active IRQ
handlers without disabling the shared interrupt line. It also drains
timestamp work before reinitializing the controller and timecounter.
The reset path restores and flushes the interrupt mask immediately after
reset deassertion.
The Rockchip IRQ handler stays suspended until initialization is complete.
Disabling timestamp work prevents workers from rearming after bus-off.
Timestamp locking keeps timestamp conversions consistent with timecounter
updates.

The bus-off path drains TX submission and RX-side echo users before
publishing BUS_OFF. Consumed RX FIFO entries are dropped while TX is
disabled to prevent self-transmitted frames from reaching ordinary RX.
Startup serializes carrier restoration, IRQ resumption and queue wake
with a new bus-off. The series also fixes RX offload cleanup when
request_irq() fails during open.

The following issues remain outside this series:

- Races in the Rockchip bus error counter cache.
- Incomplete runtime PM clock cleanup during removal or failed registration.
- Missing explicit interrupt-mask write completion before free_irq() in
  the Rockchip close path.

When I have time, I will send separate patches to fix them one by one.

Hardware testing used a LubanCat-5IO running Linux 6.18 PREEMPT and
PREEMPT_RT kernels with v7 and the applicable prerequisites backported.
Tests on each kernel covered 600 seconds of bidirectional classic CAN
traffic, plus CAN FD and CAN FD+BRS. Local echo and both software and
hardware TX timestamps were also verified.

All 1,000 socket-close iterations passed. Instrumentation captured
1,150 cases where the last TX skb of a closed socket was destroyed,
all after releasing tx_lock and with IRQs enabled.

All 200 injected echo clone failures dropped the frame without
submitting TX, advancing tx_head or adding BQL bytes; subsequent sends
succeeded after removing the injection. BUSY, occupied slots, missing
echo skbs and invalid indices were also tested, along with automatic and
manual recovery from hardware bus-off triggered by bitrate mismatches.
All passed.

Test-only instrumentation captured 20 restart/close and 20 close/FIFO
interleavings. No timestamp worker ran during about 300 seconds after
close, and no WARN/Oops, deadlocks or persistent TX stalls were observed
during CAN testing.

Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v9:
- Use workqueue disable/enable operations to prevent timestamp work from
  rearming after bus-off, balancing repeated stop calls.
- Move worker requeueing and initial scheduling outside timestamp_lock.
- Restore and flush device interrupt masking immediately after hardware
  reset, before the remaining controller configuration.
- Count consumed RX FIFO entries discarded while TX is disabled.
- Link to v8:
  https://patch.msgid.link/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae@qq.com

Changes in v8:
- Merge the previously submitted Rockchip TX fixes, with TX serialization
  before the retry fix.
- Move retained-skb handling and the missing-echo guard into the echo
  failure fix, using the original skb as the frame-data source.
- Establish the TX/RX critical sections and caller-held helper locking in
  the TX serialization patch, covering queue updates, TX diagnostics and
  RX pending checks, with assertions for command writes.
- Reuse those critical sections in the bus-off/restart lifecycle patch.
- Rebase onto the latest testing branch in the mkl/linux-can-next repository.
- Link to the Rockchip TX fixes:
  https://lore.kernel.org/linux-can/tencent_E25C9BE6689264E4138A464803B810FDFD09@qq.com
- Link to v7:
  https://patch.msgid.link/20261106-rockchip-canfd-restart-close-v7-0-accdb11bc845@qq.com

Changes in v7:
- Clarify that running checks do not serialize recovery with close, and
  keep the restart-ms example tied to configuration while down.
- Introduce Rockchip restart/IRQ synchronization and TX protection
  together, keeping close cancellation and open failure cleanup in
  separate patches.
- Build on the CAN skb and Rockchip TX fixes, reusing the TX lock and
  adapting failure cleanup to the updated skb ownership contract.
- Link to v6:
  https://patch.msgid.link/20261017-rockchip-canfd-restart-close-v6-0-ed605feb4b6e@qq.com

Changes in v6:
- Update the can_bus_off() comment and document the running-interface
  requirement for automatic recovery.
- Explain Rockchip's use of the cancellation helper.
- Release the original TX skb after unlocking.
- Abort TX submission if echo preparation fails.
- Link to v5:
  https://patch.msgid.link/20261015-rockchip-canfd-restart-close-v5-0-ae77327037c9@qq.com

Changes in v5:
- Drop consumed RX FIFO entries under tx_lock when TX is disabled
  instead of only skipping echo filtering.
- Link to v4:
  https://patch.msgid.link/20261015-rockchip-canfd-restart-close-v4-0-ad149ff79450@qq.com

Changes in v4:
- Serialize timecounter_cyc2time() with the worker's timecounter_read()
  and startup's timecounter_init() using timestamp_lock.
- Update rkcanfd_skb_set_timestamp() to take a non-const private pointer
  and document the expanded scope of timestamp_lock.
- Link to v3:
  https://patch.msgid.link/20261014-rockchip-canfd-restart-close-v3-0-f0eb41090242@qq.com

Changes in v3:
- Replace disable_irq()/enable_irq() in Rockchip restart with device
  interrupt masking, mask readback, irq_suspended and synchronize_irq().
- Use release/acquire ordering when resuming the IRQ handler.
- Initialize timestamp delayed work once at probe and synchronously
  cancel it before restart reinitializes the timecounter.
- Balance RX offload enablement when request_irq() fails during open.
- Prevent timestamp work from reading the counter or rearming after
  deactivation, with cancellation outside the lock.
- Drain TX submission and RX-side echo users before publishing BUS_OFF,
  protecting the core's pre-restart echo flush.
- Serialize carrier restoration, IRQ enablement and queue wake with a new
  bus-off; suspend shared IRQ handling during open until startup finishes.
- Link to v2:
  https://lore.kernel.org/linux-can/tencent_82C93561D200C84C24608C80049C23EEF80A@qq.com/

Changes in v2:
- Split the series into the common checks, the cancellation helper, and
  its use in Rockchip.
- Check netif_running() after the do_set_mode capability check and before
  the restart notification, echo cleanup, and carrier update.
- Only queue automatic recovery for a running interface; keep reporting
  bus-off and make the log message match the scheduling decision.
- Add close_candev_cancel_restart_sync() in linux/can/dev.h and use it in
  close_candev() without changing the existing cleanup order.
- Replace the Rockchip mutex and closing flag with an early call to the
  helper. Drop the extra close-path IRQ synchronization and mask readback.
- Retain IRQ exclusion around hardware reinitialization in the restart
  callback.
- Link to v1:
  https://lore.kernel.org/linux-can/tencent_5802EE88434010229EDF7A53084D994C6106@qq.com/

---
Cunhao Lu (10):
      can: rockchip_canfd: prevent TX stall on echo skb failure
      can: rockchip_canfd: serialize TX state and command writes
      can: rockchip_canfd: retry the outstanding TX buffer
      can: dev: check netif_running() before bus-off recovery
      can: dev: add close_candev_cancel_restart_sync()
      can: rockchip_canfd: cancel bus-off restart before closing the device
      can: rockchip_canfd: disable RX offload on open failure
      can: rockchip_canfd: serialize TX with bus-off and restart
      can: rockchip_canfd: prevent timestamp work from rearming after bus-off
      can: rockchip_canfd: serialize timecounter timestamp conversion

 Documentation/networking/can.rst                   | 10 +++-
 drivers/net/can/dev/dev.c                          | 17 ++++--
 drivers/net/can/rockchip/rockchip_canfd-core.c     | 69 ++++++++++++++++++++--
 drivers/net/can/rockchip/rockchip_canfd-rx.c       | 30 +++++++---
 .../net/can/rockchip/rockchip_canfd-timestamp.c    | 41 +++++++++++--
 drivers/net/can/rockchip/rockchip_canfd-tx.c       | 51 +++++++++++++---
 drivers/net/can/rockchip/rockchip_canfd.h          | 12 +++-
 include/linux/can/dev.h                            | 17 ++++++
 8 files changed, 213 insertions(+), 34 deletions(-)
---
base-commit: ae842166410d5f42ed3257c735dd2ea3de931f5c
change-id: 20261013-rockchip-canfd-restart-close-ebde5954f1bf

Best regards,
--  
Cunhao Lu <1579567540@qq.com>

[PATCH net v9 01/10] can: rockchip_canfd: prevent TX stall on echo skb failure

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

rkcanfd_start_xmit() advances tx_head and requests transmission even when
can_put_echo_skb() fails. Completion matches self-received frames against
the echo ring, so an entry without an echo skb cannot be completed and
can leave the TX queue stopped.

Install the echo skb before writing the hardware TX buffer. If echo
preparation fails or returns success without installing an echo skb,
drop the frame without writing TX data or commands, advancing tx_head
or adding BQL bytes.

Pass an extra reference to can_put_echo_skb() and retain the original skb
as the frame-data source until submission finishes. This keeps the data
alive even if echo cleanup removes the installed clone. Release the
retained reference on both success and failure.

Backports require can_put_echo_skb() to consume the passed reference on
every return path, including -EINVAL, or equivalent ownership handling.

Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v8:
- Retain the original skb as the frame-data source and release it after
  submission or failure.
- Reject successful echo preparation if no echo skb was installed.
---
 drivers/net/can/rockchip/rockchip_canfd-tx.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index 12200dcfd3389..420e59b41059c 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -117,16 +117,21 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	tx_head = rkcanfd_get_tx_head(priv);
 	reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
 
+	/* Retain the original skb as the frame-data source even if the
+	 * echo skb is removed before submission finishes.
+	 */
+	frame_len = can_skb_get_frame_len(skb);
+	err = can_put_echo_skb(skb_get(skb), ndev, tx_head, frame_len);
+	if (unlikely(err || !priv->can.echo_skb[tx_head]))
+		goto out_drop;
+
 	rkcanfd_write(priv, RKCANFD_REG_FD_TXFRAMEINFO, reg_frameinfo);
 	rkcanfd_write(priv, RKCANFD_REG_FD_TXID, reg_id);
 	for (i = 0; i < cfd->len; i += 4)
 		rkcanfd_write(priv, RKCANFD_REG_FD_TXDATA0 + i,
 			      *(u32 *)(cfd->data + i));
 
-	frame_len = can_skb_get_frame_len(skb);
-	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
-	if (!err)
-		netdev_sent_queue(priv->ndev, frame_len);
+	netdev_sent_queue(priv->ndev, frame_len);
 
 	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
 
@@ -137,6 +142,14 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 				  RKCANFD_TX_STOP_THRESHOLD,
 				  RKCANFD_TX_START_THRESHOLD);
 
+	dev_consume_skb_any(skb);
+
+	return NETDEV_TX_OK;
+
+out_drop:
+	ndev->stats.tx_dropped++;
+	dev_kfree_skb_any(skb);
+
 	return NETDEV_TX_OK;
 }
 

-- 
2.34.1

[PATCH net v9 02/10] can: rockchip_canfd: serialize TX state and command writes

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

rkcanfd_get_effective_tx_free() inspects the tail echo entry while the RX
completion path removes it and advances tx_tail. Concurrent TX submission
can observe inconsistent queue state or dereference an skb already queued
for NAPI, where another CPU can free it. An inconsistent free-space check
can also leave the TX queue stopped without a completion to wake it.

TX submission and RX-side retry also share the MODE/CMD/MODE sequence
used for erratum 12. Interleaving these sequences can restore the default
mode before the other caller writes CMD, bypassing the workaround.

Serialize TX submission with RX completion and retry using tx_lock. Hold
it across the TX space and RX pending checks, echo handling, head/tail
updates, BQL accounting, queue stop/wake and command writes. Require
caller-held locking in the shared helpers and keep TX diagnostics in
the same critical section.

Release the original skb retained by the preceding echo-failure fix after
unlocking and restoring IRQs, so its socket destructor cannot run under
tx_lock. Backports need that retained-reference handling and the CAN skb
ownership fix, or equivalent handling.

This lock covers running TX, completion and retry. Synchronization with
bus-off and controller reinitialization is provided by the later
lifecycle fix.

Fixes: ae002cc32ec4 ("can: rockchip_canfd: prepare to use full TX-FIFO depth")
Fixes: 83f9bd6bf39d ("can: rockchip_canfd: implement workaround for erratum 12")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v8:
- Extend the TX/RX critical sections to cover space and pending checks,
  queue updates and diagnostics; require caller-held helper locking and
  assert it for command writes.
- Release the retained original skb after unlocking.
---
 drivers/net/can/rockchip/rockchip_canfd-core.c |  1 +
 drivers/net/can/rockchip/rockchip_canfd-rx.c   | 23 ++++++++++++++---------
 drivers/net/can/rockchip/rockchip_canfd-tx.c   | 26 +++++++++++++++++++++++---
 drivers/net/can/rockchip/rockchip_canfd.h      |  3 +++
 4 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 37c1c22c40c9..1cae86973da5 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -921,6 +921,7 @@ static int rkcanfd_probe(struct platform_device *pdev)
 	priv->can.do_set_mode = rkcanfd_set_mode;
 	priv->can.do_get_berr_counter = rkcanfd_get_berr_counter;
 	priv->ndev = ndev;
+	spin_lock_init(&priv->tx_lock);
 
 	match = device_get_match_data(&pdev->dev);
 	if (match) {
diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
index 24e87daa1df0..23476a334d6c 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
@@ -91,6 +91,7 @@ rkcanfd_fifo_header_to_cfd_header(const struct rkcanfd_priv *priv,
 	return len + cfd->len;
 }
 
+/* Called with tx_lock held. */
 static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
 				const struct canfd_frame *cfd_rx, const u32 ts,
 				bool *tx_done)
@@ -101,6 +102,8 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
 	const struct sk_buff *skb;
 	unsigned int tx_tail;
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	tx_tail = rkcanfd_get_tx_tail(priv);
 	skb = priv->can.echo_skb[tx_tail];
 	if (!skb) {
@@ -201,9 +204,11 @@ rkcanfd_fifo_header_empty(const struct rkcanfd_fifo_header *header)
 static int rkcanfd_handle_rx_int_one(struct rkcanfd_priv *priv)
 {
 	struct net_device_stats *stats = &priv->ndev->stats;
-	struct canfd_frame cfd[1] = { }, *skb_cfd;
 	struct rkcanfd_fifo_header header[1] = { };
+	struct canfd_frame cfd[1] = { }, *skb_cfd;
+	bool tx_done = false;
 	struct sk_buff *skb;
+	unsigned long flags;
 	unsigned int len;
 	int err;
 
@@ -237,15 +242,15 @@ static int rkcanfd_handle_rx_int_one(struct rkcanfd_priv *priv)
 		return 0;
 	}
 
-	if (rkcanfd_get_tx_pending(priv)) {
-		bool tx_done = false;
-
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	err = 0;
+	if (rkcanfd_get_tx_pending(priv))
 		err = rkcanfd_rxstx_filter(priv, cfd, header->ts, &tx_done);
-		if (err)
-			return err;
-		if (tx_done && !(priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK))
-			return 0;
-	}
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+	if (err)
+		return err;
+	if (tx_done && !(priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK))
+		return 0;
 
 	/* Manual handling of CAN Bus Error counters. See
 	 * rkcanfd_get_corrected_berr_counter() for detailed
diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index 420e59b41059..caa71cf8e39a 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -14,6 +14,8 @@ static bool rkcanfd_tx_tail_is_eff(const struct rkcanfd_priv *priv)
 	const struct sk_buff *skb;
 	unsigned int tx_tail;
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	if (!rkcanfd_get_tx_pending(priv))
 		return false;
 
@@ -35,6 +37,8 @@ static bool rkcanfd_tx_tail_is_eff(const struct rkcanfd_priv *priv)
 
 unsigned int rkcanfd_get_effective_tx_free(const struct rkcanfd_priv *priv)
 {
+	lockdep_assert_held(&priv->tx_lock);
+
 	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6 &&
 	    rkcanfd_tx_tail_is_eff(priv))
 		return 0;
@@ -45,6 +49,8 @@ unsigned int rkcanfd_get_effective_tx_free(const struct rkcanfd_priv *priv)
 static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
 					 const u32 reg_cmd)
 {
+	lockdep_assert_held(&priv->tx_lock);
+
 	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_12)
 		rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default |
 			      RKCANFD_REG_MODE_SPACE_RX_MODE);
@@ -60,6 +66,8 @@ void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
 	const unsigned int tx_head = rkcanfd_get_tx_head(priv);
 	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
 }
 
@@ -69,12 +77,15 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u32 reg_frameinfo, reg_id, reg_cmd;
 	unsigned int tx_head, frame_len;
 	const struct canfd_frame *cfd;
+	unsigned long flags;
 	int err;
 	u8 i;
 
 	if (can_dev_dropped_skb(ndev, skb))
 		return NETDEV_TX_OK;
 
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
 	if (!netif_subqueue_maybe_stop(priv->ndev, 0,
 				       rkcanfd_get_effective_tx_free(priv),
 				       RKCANFD_TX_STOP_THRESHOLD,
@@ -85,7 +96,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 				    priv->tx_head, priv->tx_tail,
 				    rkcanfd_get_tx_pending(priv));
 
-		return NETDEV_TX_BUSY;
+		goto out_busy;
 	}
 
 	cfd = (struct canfd_frame *)skb->data;
@@ -117,8 +128,8 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	tx_head = rkcanfd_get_tx_head(priv);
 	reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
 
-	/* Retain the original skb as the frame-data source even if the
-	 * echo skb is removed before submission finishes.
+	/* Keep the original skb alive until IRQs are restored so echo
+	 * preparation cannot invoke its socket destructor under tx_lock.
 	 */
 	frame_len = can_skb_get_frame_len(skb);
 	err = can_put_echo_skb(skb_get(skb), ndev, tx_head, frame_len);
@@ -142,15 +153,22 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 				  RKCANFD_TX_STOP_THRESHOLD,
 				  RKCANFD_TX_START_THRESHOLD);
 
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 	dev_consume_skb_any(skb);
 
 	return NETDEV_TX_OK;
 
 out_drop:
 	ndev->stats.tx_dropped++;
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 	dev_kfree_skb_any(skb);
 
 	return NETDEV_TX_OK;
+
+out_busy:
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+	return NETDEV_TX_BUSY;
 }
 
 void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,
@@ -160,6 +178,8 @@ void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,
 	unsigned int tx_tail;
 	struct sk_buff *skb;
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	tx_tail = rkcanfd_get_tx_tail(priv);
 	skb = priv->can.echo_skb[tx_tail];
 
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 95bea9bfd8a2..077dd57d42de 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -15,6 +15,7 @@
 #include <linux/netdevice.h>
 #include <linux/reset.h>
 #include <linux/skbuff.h>
+#include <linux/spinlock.h>
 #include <linux/timecounter.h>
 #include <linux/types.h>
 #include <linux/u64_stats_sync.h>
@@ -474,6 +475,8 @@ struct rkcanfd_priv {
 	struct can_rx_offload offload;
 	struct net_device *ndev;
 
+	/* Protect running TX/RX echo state, queue updates and TX commands. */
+	spinlock_t tx_lock;
 	void __iomem *regs;
 	unsigned int tx_head;
 	unsigned int tx_tail;

-- 
2.34.1

[PATCH net v9 03/10] can: rockchip_canfd: retry the outstanding TX buffer

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

rkcanfd_xmit_retry() uses tx_head to select the buffer for the erratum 6
retransmission. With a TX FIFO depth of one, the masked head and tail
indices both select buffer 0. With two buffers and one outstanding frame,
tx_head selects the next free buffer instead of the pending one.

Use tx_tail to retry the outstanding buffer. Otherwise, the pending echo
entry can remain incomplete and leave the TX queue stopped.

Backports must include the preceding TX serialization fix or equivalent
synchronization. It prevents completion and retry from observing a newly
published head before its initial TX command and serializes the
MODE/CMD/MODE sequences used by submission and retry.

Fixes: a5605d61c7dd ("can: rockchip_canfd: enable full TX-FIFO depth of 2")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v8:
- Apply the retry-slot correction after TX serialization.
---
 drivers/net/can/rockchip/rockchip_canfd-tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index caa71cf8e39a..8998b7855d15 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -63,8 +63,8 @@ static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
 
 void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
 {
-	const unsigned int tx_head = rkcanfd_get_tx_head(priv);
-	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
+	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
+	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
 
 	lockdep_assert_held(&priv->tx_lock);
 

-- 
2.34.1

[PATCH net v9 04/10] can: dev: check netif_running() before bus-off recovery

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

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(), schedule recovery
only when the check observes the interface running, 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>

---
Changes in v7:
- Clarify the running checks' limits and configure restart-ms while down.

Changes in v6:
- Update the can_bus_off() comment and document the running-interface
  requirement for automatic recovery.

---

Q: Do the running checks serialize recovery with device close?

A: No. With automatic recovery enabled, a worker can pass the check
before close clears the running bit. Drivers such as FlexCAN still
cancel it after hardware teardown, so their restart callback can run
after shutdown. The Rockchip patches in this series wait for an active
restart before teardown and cancel again after draining the IRQ.
Other drivers need separate lifetime fixes; these checks only reject
recovery that observes down.

Q: Can HI3110 queue restart work after close and outlive the device?

A: Yes. With restart-ms nonzero, its IRQ thread can sample running
before close, then queue work after close_candev() returns. The later
free_irq() drains the producer but does not cancel the queued core
work. A quick reopen can execute stale recovery; removal can leave
work referring to a freed can_priv. The driver's private workqueue
does not own this work. A separate fix must quiesce producers before
final cancellation and protect teardown from an active restart. The
running checks narrow this window but cannot protect freed memory.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-rockchip-canfd-restart-close-v6-0-ed605feb4b6e%40qq.com
---
 Documentation/networking/can.rst | 10 ++++++++--
 drivers/net/can/dev/dev.c        | 13 ++++++++++---
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
index 51bc70748f753..9debef8702d5f 100644
--- a/Documentation/networking/can.rst
+++ b/Documentation/networking/can.rst
@@ -1302,7 +1302,8 @@ More info to the above output:
 	Automatic restart delay time. If set to a non-zero value, a
 	restart of the CAN controller will be triggered automatically
 	in case of a bus-off condition after the specified delay time
-	in milliseconds. By default it's off.
+	in milliseconds. Recovery is skipped if the interface is found
+	to be down. By default it's off.
 
 "bitrate 125000 sample-point 0.875"
 	Shows the real bit-rate in bits/sec and the sample-point in the
@@ -1376,9 +1377,14 @@ before you can start it to avoid error-prone default settings::
 A device may enter the "bus-off" state if too many errors occurred on
 the CAN bus. Then no more messages are received or sent. An automatic
 bus-off recovery can be enabled by setting the "restart-ms" to a
-non-zero value, e.g.::
+non-zero value while the interface is down, e.g.::
 
+    $ ip link set canX down
     $ ip link set canX type can restart-ms 100
+    $ ip link set canX up
+
+Before scheduling or starting recovery, the kernel checks whether the
+interface is running and skips recovery if it is down.
 
 Alternatively, the application may realize the "bus-off" condition
 by monitoring CAN error message frames and do a restart when
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 769745e22a3c7..a893a49a33813 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");
 
@@ -259,13 +262,17 @@ int can_restart_now(struct net_device *dev)
  *
  * This functions should be called when the device goes bus-off to
  * tell the netif layer that no more packets can be sent or received.
- * If enabled, a timer is started to trigger bus-off recovery.
+ * If automatic recovery is enabled, schedule delayed work unless the
+ * running check observes the interface down. Drivers must still synchronize
+ * recovery with device close.
  */
 void can_bus_off(struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
+	bool restart;
 
-	if (priv->restart_ms)
+	restart = priv->restart_ms && netif_running(dev);
+	if (restart)
 		netdev_info(dev, "bus-off, scheduling restart in %d ms\n",
 			    priv->restart_ms);
 	else
@@ -273,7 +280,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 v9 05/10] can: dev: add close_candev_cancel_restart_sync()

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

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.

The following Rockchip fix uses the helper at the start of rkcanfd_stop()
to wait for recovery before IRQ teardown, leaving echo cleanup to
close_candev().

Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v6:
- Describe the helper's use by the following Rockchip fix.
---
 drivers/net/can/dev/dev.c |  4 +---
 include/linux/can/dev.h   | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index a893a49a3381..39271a3ef0fa 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -537,9 +537,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 346faddee02e..2a286f906394 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,22 @@ struct can_priv *safe_candev_priv(struct net_device *dev);
 
 int open_candev(struct net_device *dev);
 void close_candev(struct net_device *dev);
+
+/**
+ * close_candev_cancel_restart_sync() - cancel bus-off restart synchronously
+ * @dev: CAN network device
+ *
+ * Call before tearing down resources used by the restart callback.
+ * The driver must quiesce restart work producers before calling
+ * 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 v9 06/10] can: rockchip_canfd: cancel bus-off restart before closing the device

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

The close path cancels restart_work in close_candev() only after freeing
the IRQ, disabling RX offload and stopping the controller. A concurrent
restart can re-enable the controller and interrupts after IRQ teardown,
or requeue timestamp work that accesses registers after runtime suspend
has disabled 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. Keep
close_candev() after IRQ teardown for final cancellation and echo cleanup.

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>

---
Changes in v7:
- Keep close cancellation separate from restart and open error handling.
---
 drivers/net/can/rockchip/rockchip_canfd-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 1cae86973da5..59a79fe057e6 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -758,6 +758,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

[PATCH net v9 07/10] can: rockchip_canfd: disable RX offload on open failure

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

rkcanfd_open() enables RX offload before requesting the IRQ. If
request_irq() fails, the error path leaves the offload enabled. A later
open calls napi_enable() on an already enabled NAPI instance.

Disable RX offload on that error path before stopping the controller and
releasing the runtime PM reference.

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>

---
Changes in v7:
- Split the open error cleanup into a separate patch.

Changes in v3:
- Disable RX offload when request_irq() fails during open.
---
 drivers/net/can/rockchip/rockchip_canfd-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 59a79fe057e6..d05264605efc 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -747,6 +747,7 @@ static int rkcanfd_open(struct net_device *ndev)
 	return 0;
 
 out_rkcanfd_chip_stop:
+	can_rx_offload_disable(&priv->offload);
 	rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);
 	pm_runtime_put(ndev->dev.parent);
 out_close_candev:

-- 
2.34.1

[PATCH net v9 08/10] can: rockchip_canfd: serialize TX with bus-off and restart

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

The bus-off path can reset the controller while TX submission or RX-side
echo handling is active. CAN restart flushes echo skbs before set_mode(),
so disabling TX in that callback is too late to protect the echo ring.

Use tx_lock and tx_enabled to drain TX and RX-side echo users before
publishing CAN_STATE_BUS_OFF. Return NETDEV_TX_BUSY while TX is disabled
and drop consumed RX entries so self-transmitted frames cannot escape
echo filtering as ordinary RX.

Before restart, mask device interrupts and flush the mask write, suspend
shared IRQ access, and drain active handlers and timestamp work. After
reinitialization, restore carrier, TX, IRQ handling and the queue under
tx_lock, so the old bus-off IRQ cannot leave carrier off and a new bus-off
takes precedence. Keep synchronous waits outside the lock and other
shared IRQ users enabled.

Reset clears INT_MASK. Restore and flush the full mask immediately after
deassertion so the remaining controller configuration stays masked.

Keep IRQ handling suspended until open completes and TX disabled on
close or failed open. Initialize timestamp work once at probe.

Fixes: ff60bfbaf67f ("can: rockchip_canfd: add driver for Rockchip CAN-FD controller")
Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v9:
- Restore and flush the full interrupt mask immediately after reset
  deassertion, before the remaining controller configuration.
- Count consumed RX FIFO entries discarded while TX is disabled.
- Include asm/barrier.h directly for release/acquire operations.

Changes in v8:
- Move skb ownership handling and running TX/RX critical sections to
  the preceding TX patches.

Changes in v7:
- Introduce restart/IRQ synchronization and TX protection together.
- Reuse prerequisite TX locking and adapt to the updated skb ownership.

Changes in v6:
- Defer the original skb's release until after unlocking.
- Abort TX submission if echo preparation fails.

Changes in v5:
- Drop consumed RX FIFO entries when tx_enabled is false under tx_lock,
  rather than skipping only echo filtering and delivering them as RX.

Changes in v3:
- Replace disable_irq()/enable_irq() with device interrupt masking,
  mask readback, an irq_suspended flag and synchronize_irq().
- Pair release/acquire accesses to publish reinitialized state before
  the IRQ handler resumes, without disabling other shared IRQ users.
- Initialize timestamp delayed work once at probe and synchronously
  cancel it before reinitializing the timestamp state during restart.
---
 drivers/net/can/rockchip/rockchip_canfd-core.c     | 66 ++++++++++++++++++++--
 drivers/net/can/rockchip/rockchip_canfd-rx.c       | 11 +++-
 .../net/can/rockchip/rockchip_canfd-timestamp.c    |  6 +-
 drivers/net/can/rockchip/rockchip_canfd-tx.c       |  4 ++
 drivers/net/can/rockchip/rockchip_canfd.h          |  6 +-
 5 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index d05264605efc2..a5c6331191d17 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -22,6 +22,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/string.h>
 
+#include <asm/barrier.h>
+
 #include "rockchip_canfd.h"
 
 static const struct rkcanfd_devtype_data rkcanfd_devtype_data_rk3568v2 = {
@@ -116,6 +118,10 @@ static void rkcanfd_chip_set_reset_mode(const struct rkcanfd_priv *priv)
 	udelay(2);
 	reset_control_deassert(priv->reset);
 
+	/* Reset clears INT_MASK; restore it before configuring the controller. */
+	rkcanfd_write(priv, RKCANFD_REG_INT_MASK, RKCANFD_REG_INT_ALL);
+	rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
+
 	rkcanfd_write(priv, RKCANFD_REG_MODE, 0x0);
 }
 
@@ -325,6 +331,34 @@ static void rkcanfd_chip_start(struct rkcanfd_priv *priv)
 		   rkcanfd_read(priv, RKCANFD_REG_MODE));
 }
 
+static void rkcanfd_tx_disable(struct rkcanfd_priv *priv)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	priv->tx_enabled = false;
+	netif_stop_queue(priv->ndev);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+}
+
+static void rkcanfd_chip_start_finish(struct rkcanfd_priv *priv)
+{
+	unsigned long flags;
+
+	/* Restore carrier after draining the old bus-off IRQ. Hold tx_lock
+	 * through IRQ resumption and queue wake so a new bus-off takes
+	 * precedence.
+	 */
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	netif_carrier_on(priv->ndev);
+	priv->tx_enabled = true;
+	/* Publish initialized state to the IRQ handler's acquire. */
+	smp_store_release(&priv->irq_suspended, false);
+	rkcanfd_chip_interrupts_enable(priv);
+	netif_wake_queue(priv->ndev);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+}
+
 static void __rkcanfd_chip_stop(struct rkcanfd_priv *priv, const enum can_state state)
 {
 	priv->can.state = state;
@@ -356,9 +390,17 @@ static int rkcanfd_set_mode(struct net_device *ndev,
 
 	switch (mode) {
 	case CAN_MODE_START:
+		rkcanfd_tx_disable(priv);
+		rkcanfd_chip_interrupts_disable(priv);
+		/* Flush the interrupt mask write before suspending the handler. */
+		rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
+		WRITE_ONCE(priv->irq_suspended, true);
+		/* Wait for handlers that already passed the suspended check. */
+		synchronize_irq(ndev->irq);
+		rkcanfd_timestamp_stop_sync(priv);
+
 		rkcanfd_chip_start(priv);
-		rkcanfd_chip_interrupts_enable(priv);
-		netif_wake_queue(ndev);
+		rkcanfd_chip_start_finish(priv);
 		break;
 
 	default:
@@ -592,6 +634,11 @@ static int rkcanfd_handle_state_error_int(struct rkcanfd_priv *priv)
 	 * handles cf == NULL.
 	 */
 	skb = rkcanfd_alloc_can_err_skb(priv, &cf, &timestamp);
+	/* Drain TX before publishing BUS_OFF, which permits a manual
+	 * restart to flush echo SKBs before calling our set_mode().
+	 */
+	if (new_state == CAN_STATE_BUS_OFF)
+		rkcanfd_tx_disable(priv);
 	can_change_state(ndev, cf, tx_state, rx_state);
 
 	if (new_state == CAN_STATE_BUS_OFF) {
@@ -666,6 +713,10 @@ static irqreturn_t rkcanfd_irq(int irq, void *dev_id)
 	struct rkcanfd_priv *priv = dev_id;
 	u32 reg_int_unmasked, reg_int;
 
+	/* Pairs with smp_store_release() in rkcanfd_chip_start_finish(). */
+	if (smp_load_acquire(&priv->irq_suspended))
+		return IRQ_NONE;
+
 	reg_int_unmasked = rkcanfd_read(priv, RKCANFD_REG_INT);
 	reg_int = reg_int_unmasked & ~priv->reg_int_mask_default;
 
@@ -733,6 +784,8 @@ static int rkcanfd_open(struct net_device *ndev)
 	if (err)
 		goto out_close_candev;
 
+	/* The shared IRQ handler may run before request_irq() returns. */
+	WRITE_ONCE(priv->irq_suspended, true);
 	rkcanfd_chip_start(priv);
 	can_rx_offload_enable(&priv->offload);
 
@@ -740,13 +793,12 @@ static int rkcanfd_open(struct net_device *ndev)
 	if (err)
 		goto out_rkcanfd_chip_stop;
 
-	rkcanfd_chip_interrupts_enable(priv);
-
-	netif_start_queue(ndev);
+	rkcanfd_chip_start_finish(priv);
 
 	return 0;
 
 out_rkcanfd_chip_stop:
+	rkcanfd_tx_disable(priv);
 	can_rx_offload_disable(&priv->offload);
 	rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);
 	pm_runtime_put(ndev->dev.parent);
@@ -760,7 +812,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_tx_disable(priv);
 
 	rkcanfd_chip_interrupts_disable(priv);
 	free_irq(ndev->irq, priv);
@@ -937,6 +989,8 @@ static int rkcanfd_probe(struct platform_device *pdev)
 	if (err)
 		goto out_free_candev;
 
+	rkcanfd_timestamp_work_init(priv);
+
 	err = rkcanfd_register(priv);
 	if (err)
 		goto out_can_rx_offload_del;
diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
index 23476a334d6c3..0eab00623e037 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
@@ -91,7 +91,7 @@ rkcanfd_fifo_header_to_cfd_header(const struct rkcanfd_priv *priv,
 	return len + cfd->len;
 }
 
-/* Called with tx_lock held. */
+/* Called with tx_lock held and TX enabled. */
 static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
 				const struct canfd_frame *cfd_rx, const u32 ts,
 				bool *tx_done)
@@ -242,7 +242,16 @@ static int rkcanfd_handle_rx_int_one(struct rkcanfd_priv *priv)
 		return 0;
 	}
 
+	/* Drop consumed FIFO entries while TX is disabled to avoid delivering
+	 * self-received frames as ordinary RX without echo filtering.
+	 */
 	spin_lock_irqsave(&priv->tx_lock, flags);
+	if (unlikely(!priv->tx_enabled)) {
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+		stats->rx_dropped++;
+		return 0;
+	}
+
 	err = 0;
 	if (rkcanfd_get_tx_pending(priv))
 		err = rkcanfd_rxstx_filter(priv, cfd, header->ts, &tx_done);
diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
index 72774cd2f94b7..ad582afbbd682 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
@@ -37,6 +37,11 @@ static void rkcanfd_timestamp_work(struct work_struct *work)
 	schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
 }
 
+void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv)
+{
+	INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
+}
+
 void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
 {
 	const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
@@ -73,7 +78,6 @@ void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
 	max_cycles = min(max_cycles, cc->mask);
 	work_delay_ns = clocksource_cyc2ns(max_cycles, cc->mult, cc->shift);
 	priv->work_delay_jiffies = div_u64(work_delay_ns, 3u * NSEC_PER_SEC / HZ);
-	INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
 
 	netdev_dbg(priv->ndev, "clock=%lu.%02luMHz bitrate=%lu.%02luMBit/s div=%u rate=%lu.%02luMHz mult=%u shift=%u delay=%lus\n",
 		   priv->can.clock.freq / MEGA,
diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index 8998b7855d15d..6f60d4119812f 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -85,6 +85,10 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_OK;
 
 	spin_lock_irqsave(&priv->tx_lock, flags);
+	if (unlikely(!priv->tx_enabled)) {
+		netif_stop_queue(ndev);
+		goto out_busy;
+	}
 
 	if (!netif_subqueue_maybe_stop(priv->ndev, 0,
 				       rkcanfd_get_effective_tx_free(priv),
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 077dd57d42de9..8fdb59025bfaf 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -475,7 +475,7 @@ struct rkcanfd_priv {
 	struct can_rx_offload offload;
 	struct net_device *ndev;
 
-	/* Protect running TX/RX echo state, queue updates and TX commands. */
+	/* Serialize TX submission/completion with bus-off and restart. */
 	spinlock_t tx_lock;
 	void __iomem *regs;
 	unsigned int tx_head;
@@ -485,6 +485,9 @@ struct rkcanfd_priv {
 	u32 reg_int_mask_default;
 	struct rkcanfd_devtype_data devtype_data;
 
+	bool tx_enabled;
+	bool irq_suspended;
+
 	struct cyclecounter cc;
 	struct timecounter tc;
 	struct delayed_work timestamp;
@@ -554,6 +557,7 @@ int rkcanfd_handle_rx_int(struct rkcanfd_priv *priv);
 
 void rkcanfd_skb_set_timestamp(const struct rkcanfd_priv *priv,
 			       struct sk_buff *skb, const u32 timestamp);
+void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_init(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_start(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv);

-- 
2.34.1

[PATCH net v9 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

The bus-off IRQ cancels timestamp work asynchronously. If the worker
is already running, it can requeue itself after cancellation and keep
polling during bus-off, causing unnecessary wakeups and counter reads.

Use disable_delayed_work() to block requeueing until startup re-enables
the work. Initialize it disabled and enable it after initializing the
timecounter.

Keep the active check and counter read under timestamp_lock so stop
waits for an in-flight read before the controller is reset. Use the
active flag to avoid disabling work twice when restart or close follows
bus-off.

Keep workqueue operations outside timestamp_lock; the IRQ and restart
paths already serialize start and stop. Synchronous stop waits for the
callback before reinitialization or clock shutdown.

Fixes: 4e1a18bab124 ("can: rockchip_canfd: add hardware timestamping support")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v9:
- Use disable_delayed_work() to block self-requeueing, with balanced
  enable/disable calls across repeated stops.
- Move worker requeueing and initial scheduling outside timestamp_lock.
---
 .../net/can/rockchip/rockchip_canfd-timestamp.c    | 30 ++++++++++++++++++++--
 drivers/net/can/rockchip/rockchip_canfd.h          |  3 +++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
index ad582afbbd68..7c7b95343aa8 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
@@ -30,18 +30,26 @@ static void rkcanfd_timestamp_work(struct work_struct *work)
 {
 	const struct delayed_work *delayed_work = to_delayed_work(work);
 	struct rkcanfd_priv *priv;
+	unsigned long flags;
 
 	priv = container_of(delayed_work, struct rkcanfd_priv, timestamp);
-	timecounter_read(&priv->tc);
+	spin_lock_irqsave(&priv->timestamp_lock, flags);
+	if (priv->timestamp_active)
+		timecounter_read(&priv->tc);
+	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
 
 	schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
 }
 
 void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv)
 {
+	spin_lock_init(&priv->timestamp_lock);
+	priv->timestamp_active = false;
 	INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
+	disable_delayed_work(&priv->timestamp);
 }
 
+/* The caller must quiesce IRQs and timestamp work before reconfiguration. */
 void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
 {
 	const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
@@ -93,17 +101,35 @@ void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
 
 void rkcanfd_timestamp_start(struct rkcanfd_priv *priv)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->timestamp_lock, flags);
 	timecounter_init(&priv->tc, &priv->cc, ktime_get_real_ns());
+	priv->timestamp_active = true;
+	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
 
+	enable_delayed_work(&priv->timestamp);
 	schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
 }
 
 void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv)
 {
-	cancel_delayed_work(&priv->timestamp);
+	unsigned long flags;
+
+	/* Stop counter reads before the caller resets the controller. */
+	spin_lock_irqsave(&priv->timestamp_lock, flags);
+	if (!priv->timestamp_active) {
+		spin_unlock_irqrestore(&priv->timestamp_lock, flags);
+		return;
+	}
+	priv->timestamp_active = false;
+	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
+
+	disable_delayed_work(&priv->timestamp);
 }
 
 void rkcanfd_timestamp_stop_sync(struct rkcanfd_priv *priv)
 {
+	rkcanfd_timestamp_stop(priv);
 	cancel_delayed_work_sync(&priv->timestamp);
 }
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 8fdb59025bfa..3ce996f52e45 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -477,6 +477,8 @@ struct rkcanfd_priv {
 
 	/* Serialize TX submission/completion with bus-off and restart. */
 	spinlock_t tx_lock;
+	/* Serialize timestamp work with start/stop. */
+	spinlock_t timestamp_lock;
 	void __iomem *regs;
 	unsigned int tx_head;
 	unsigned int tx_tail;
@@ -487,6 +489,7 @@ struct rkcanfd_priv {
 
 	bool tx_enabled;
 	bool irq_suspended;
+	bool timestamp_active;
 
 	struct cyclecounter cc;
 	struct timecounter tc;

-- 
2.34.1

[PATCH net v9 10/10] can: rockchip_canfd: serialize timecounter timestamp conversion

[Cunhao Lu via B4 Relay]

From: Cunhao Lu <1579567540@qq.com>

The timestamp worker advances the timecounter with timecounter_read()
while RX, TX completion and error-frame paths use timecounter_cyc2time()
to convert hardware timestamps without holding timestamp_lock.

timecounter_read() updates frac, cycle_last and nsec in separate steps.
A concurrent conversion can observe the new cycle_last with the old
nsec and produce an incorrect timestamp.

Hold timestamp_lock across timecounter_cyc2time(), using the same lock
as timecounter_read() and timecounter_init(). Keep the SKB timestamp
assignment outside the critical section.

TX completion nests timestamp_lock inside tx_lock; no timestamp-lock
holder acquires tx_lock.

Fixes: 4e1a18bab124 ("can: rockchip_canfd: add hardware timestamping support")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
 drivers/net/can/rockchip/rockchip_canfd-timestamp.c | 5 ++++-
 drivers/net/can/rockchip/rockchip_canfd.h           | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
index 7c7b95343aa8..76d522ee972e 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
@@ -15,13 +15,16 @@ static u64 rkcanfd_timestamp_read(struct cyclecounter *cc)
 	return rkcanfd_get_timestamp(priv);
 }
 
-void rkcanfd_skb_set_timestamp(const struct rkcanfd_priv *priv,
+void rkcanfd_skb_set_timestamp(struct rkcanfd_priv *priv,
 			       struct sk_buff *skb, const u32 timestamp)
 {
 	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
+	unsigned long flags;
 	u64 ns;
 
+	spin_lock_irqsave(&priv->timestamp_lock, flags);
 	ns = timecounter_cyc2time(&priv->tc, timestamp);
+	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
 
 	hwtstamps->hwtstamp = ns_to_ktime(ns);
 }
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 3ce996f52e45..da64a7c05008 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -477,7 +477,7 @@ struct rkcanfd_priv {
 
 	/* Serialize TX submission/completion with bus-off and restart. */
 	spinlock_t tx_lock;
-	/* Serialize timestamp work with start/stop. */
+	/* Protects tc and timestamp_active. */
 	spinlock_t timestamp_lock;
 	void __iomem *regs;
 	unsigned int tx_head;
@@ -558,7 +558,7 @@ void rkcanfd_ethtool_init(struct rkcanfd_priv *priv);
 
 int rkcanfd_handle_rx_int(struct rkcanfd_priv *priv);
 
-void rkcanfd_skb_set_timestamp(const struct rkcanfd_priv *priv,
+void rkcanfd_skb_set_timestamp(struct rkcanfd_priv *priv,
 			       struct sk_buff *skb, const u32 timestamp);
 void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_init(struct rkcanfd_priv *priv);

-- 
2.34.1

Re: [PATCH net v9 01/10] can: rockchip_canfd: prevent TX stall on echo skb failure

[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 understates how bad the bug is.

--

can: rockchip_canfd: prevent TX stall on echo skb failure

This commit moves can_put_echo_skb() in rkcanfd_start_xmit() so it runs
before the hardware TX buffer is written. If no echo skb was installed, the
frame is dropped and tx_head and the BQL bytes are left unchanged. It also
passes an extra skb reference to can_put_echo_skb() and releases the
original skb once submission finishes or fails.

> rkcanfd_start_xmit() advances tx_head and requests transmission even when
> can_put_echo_skb() fails. Completion matches self-received frames against
> the echo ring, so an entry without an echo skb cannot be completed and
> can leave the TX queue stopped.

[Severity: Low]
The code change looks right to me. Does this description cover the whole
impact, though? It looks like RX stops as well as TX.

can_create_echo_skb() uses skb_clone(GFP_ATOMIC), so can_put_echo_skb() can
fail with -ENOMEM under memory pressure. Before this patch, the empty slot
still became tx_tail, so rkcanfd_get_tx_pending() stayed non-zero.

After that, every received frame goes through rkcanfd_rxstx_filter():

drivers/net/can/rockchip/rockchip_canfd-rx.c:rkcanfd_rxstx_filter() {
    ...
	skb = priv->can.echo_skb[tx_tail];
	if (!skb) {
		netdev_err(priv->ndev, ...);

		return -ENOMSG;
	}
    ...
}

By this point rkcanfd_handle_rx_int_one() has already pulled the frame out
of the FIFO with rkcanfd_read_rep(). It then returns before
alloc_can_skb() and can_rx_offload_queue_timestamp() are called:

drivers/net/can/rockchip/rockchip_canfd-rx.c:rkcanfd_handle_rx_int_one() {
    ...
	if (rkcanfd_get_tx_pending(priv)) {
		bool tx_done = false;

		err = rkcanfd_rxstx_filter(priv, cfd, header->ts, &tx_done);
		if (err)
			return err;
    ...
}

rkcanfd_handle_rx_int() then leaves its FIFO loop on that error. tx_tail
never advances, so the same thing happens on every later RX interrupt, with
a netdev_err() each time. Frames from other nodes on the bus are lost as
well until the interface is restarted.

Could the commit message say that both RX and TX stop? That would help
stable backporters judge how urgent this fix is.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com

Re: [PATCH net v9 02/10] can: rockchip_canfd: serialize TX state and command writes

[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 carries Cc: stable and makes the backport
  depend on three other fixes, but only describes them loosely and never…

--

can: rockchip_canfd: serialize TX state and command writes

This patch adds a tx_lock spinlock to struct rkcanfd_priv. The lock
serializes rkcanfd_start_xmit() against RX-side echo completion and the
erratum 12 retry path. It covers the TX free-space checks, echo handling,
head/tail updates, BQL accounting and the MODE/CMD/MODE command sequence.
The retained original skb is now released after the lock is dropped.

> Release the original skb retained by the preceding echo-failure fix after
> unlocking and restoring IRQs, so its socket destructor cannot run under
> tx_lock. Backports need that retained-reference handling and the CAN skb
> ownership fix, or equivalent handling.

[Severity: Low]
This isn't a bug, but since the patch carries Cc: stable and depends on
other fixes for backports, could those fixes be named by subject (and by
hash where the hash is stable)?

The "preceding echo-failure fix" looks like the previous patch in this
series:

  can: rockchip_canfd: prevent TX stall on echo skb failure

The "CAN skb ownership fix" looks like:

  b4ebcbbf66ba ("can: dev: can_put_echo_skb(): free skb on invalid echo index")

The skb_get() passed to can_put_echo_skb() in rkcanfd_start_xmit(),
together with the dev_consume_skb_any() / dev_kfree_skb_any() calls after
the unlock, only balances if can_put_echo_skb() also frees the passed
reference on the -EINVAL path. That makes b4ebcbbf66ba a hard
prerequisite for any backport.

> This lock covers running TX, completion and retry. Synchronization with
> bus-off and controller reinitialization is provided by the later
> lifecycle fix.

Which patch is "the later lifecycle fix"? It appears to be "can:
rockchip_canfd: serialize TX with bus-off and restart" later in this
series.

At this commit, rkcanfd_chip_fifo_setup(), reached via
rkcanfd_set_mode(CAN_MODE_START) -> rkcanfd_chip_start(), still resets
the TX ring and BQL state without tx_lock:

drivers/net/can/rockchip/rockchip_canfd-core.c:rkcanfd_chip_fifo_setup() {
    ...
	WRITE_ONCE(priv->tx_head, 0);
	WRITE_ONCE(priv->tx_tail, 0);
	netdev_reset_queue(priv->ndev);
}

The chip stop / can_bus_off() handling in rkcanfd_handle_state_error_int()
also runs outside the lock.

Could the follow-up be named explicitly, so a stable backport of this
patch also picks up the change that closes that window?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com

Re: [PATCH net v9 03/10] can: rockchip_canfd: retry the outstanding TX buffer

[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 "can: rockchip_canfd: retry the
  outstanding TX buffer" leaves out a likely bus-visible effect of the…

--

can: rockchip_canfd: retry the outstanding TX buffer

This patch changes rkcanfd_xmit_retry() to pick the TX buffer for the
erratum 6 retransmission with tx_tail instead of tx_head. With a TX FIFO
depth of two and one frame outstanding, tx_head points at the free buffer
and not at the pending one.

> Use tx_tail to retry the outstanding buffer. Otherwise, the pending echo
> entry can remain incomplete and leave the TX queue stopped.

[Severity: Low]
The tx_head to tx_tail change looks correct for the one-pending case. Does
this paragraph describe the full impact of the old code, though?

The erratum 6 retry can only run when exactly one frame is pending and
that frame is EFF. In that state rkcanfd_get_effective_tx_free() returns 0:

	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6 &&
	    rkcanfd_tx_tail_is_eff(priv))
		return 0;

So before this patch, the masked tx_head in rkcanfd_xmit_retry() always
pointed at the other hardware buffer.

The fix assumes that RKCANFD_REG_CMD_TX_REQ(i) sends whatever is already
stored in buffer i. If so, did every erratum 6 retry with the old code
re-send the stale contents of the non-pending buffer instead of the
mangled EFF frame? That would usually be an earlier SFF frame that had
already completed.

If that happened, other nodes on the bus would see a duplicate frame.
Locally, the self-received copy would not match echo_skb[tx_tail] in
rkcanfd_rxstx_filter(). It would then go to local sockets as a normal
received frame through rkcanfd_handle_rx_int_one().

If instead TX_REQ(i) latches the shared FD_TXFRAMEINFO, FD_TXID and
FD_TXDATA registers written in rkcanfd_start_xmit(), the old and new code
would both send the EFF frame. In that case the stall described above
would not happen.

The driver code alone doesn't show which of these is the real hardware
behavior. Could the commit message say which TX_REQ semantics the fix
depends on? If they apply, could it also mention the duplicate
transmission and the phantom local RX frame?

Was this reproduced or tested on RK3568 hardware? If so, a note on that
or a Tested-by tag would help.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com

Re: [PATCH net v9 04/10] can: dev: check netif_running() before bus-off recovery

[netdev-bot+sashiko]

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

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

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

- [High] In can_bus_off() (drivers/net/can/dev/dev.c:274-285), the new
  running check and the schedule_delayed_work() call are two separate…
- [High] The new netif_running() check in can_restart()
  (drivers/net/can/dev/dev.c:201) runs from the restart delayed work on…
- [Low] The new text in Documentation/networking/can.rst makes an
  unconditional promise about restart-ms.

--

can: dev: check netif_running() before bus-off recovery

This patch adds netif_running() checks to can_restart() and can_bus_off().
The goal is to stop automatic bus-off recovery from being scheduled or
started once the interface has begun closing. It also updates
Documentation/networking/can.rst to describe the new behaviour and to
recommend setting restart-ms while the interface is down.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-rockchip-canfd-restart-close-v6-0-ed605feb4b6e%40qq.com

> diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
> index 51bc70748f753..9debef8702d5f 100644
> --- a/Documentation/networking/can.rst
> +++ b/Documentation/networking/can.rst
> @@ -1302,7 +1302,8 @@ More info to the above output:
>  	Automatic restart delay time. If set to a non-zero value, a
>  	restart of the CAN controller will be triggered automatically
>  	in case of a bus-off condition after the specified delay time
> -	in milliseconds. By default it's off.
> +	in milliseconds. Recovery is skipped if the interface is found
> +	to be down. By default it's off.

[ ... ]

> +Before scheduling or starting recovery, the kernel checks whether the
> +interface is running and skips recovery if it is down.

[Severity: Low]
Does this hold for every driver? Only the core software recovery path
(can_bus_off() -> restart_work -> can_restart()) does the new check.

Several in-tree drivers read a non-zero restart_ms as "let the controller
recover from bus-off by itself". In that case they never call can_bus_off()
or can_restart(). For example, kvaser_pciefd_change_state():

	if (!can->can.restart_ms) {
		kvaser_pciefd_start_controller_flush(can);
		can_bus_off(ndev);
	}

and grcan_err():

	if (priv->can.restart_ms && oldstate == CAN_STATE_BUS_OFF) {
		unsigned long flags;

		cf.can_id |= CAN_ERR_RESTARTED;
		netdev_dbg(dev, "restarted\n");
		priv->can.can_stats.restarts++;
		netif_carrier_on(dev);

kvaser_usb_leaf and kvaser_usb_hydra follow the same pattern. None of these
check netif_running(). A hardware recovery between the clearing of
__LINK_STATE_START and ndo_stop() stopping the controller is therefore not
covered by the documented check.

Given the races in the core path described below, would it be more
accurate to say the check applies only to core-driven recovery and is
best effort?

> diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
> index 769745e22a3c7..a893a49a33813 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;
> +

[Severity: High]
Can a restart worker pass this check and still restart the controller
while ndo_stop() is running?

can_restart_work() runs on system_percpu_wq in preemptible process
context. It holds neither RTNL nor the netdev instance lock, so nothing
orders this test_bit() against __dev_close_many():

worker                              close path
can_restart_work()
  can_restart()
    netif_running() == true
    <preempted>
                                    __dev_close_many()
                                      clear_bit(__LINK_STATE_START, ...)
                                      ndo_stop()
    can_flush_echo_skb()
    netif_rx(CAN_ERR_RESTARTED skb)
    netif_carrier_on()
    priv->do_set_mode(dev, CAN_MODE_START)

The only thing that waits for the worker is cancel_delayed_work_sync() in
close_candev(). Many drivers call it at the end of ndo_stop, for example
flexcan_close():

	free_irq(dev->irq, dev);
	can_rx_offload_disable(&priv->offload);
	flexcan_chip_stop_disable_on_error(dev);

	can_rx_offload_del(&priv->offload);
	flexcan_transceiver_disable(priv);
	close_candev(dev);

In that case the worker's CAN_MODE_START can restart the chip after it
was stopped. The chip can then stay active on the bus after the interface
is down.

The worker's can_flush_echo_skb() can also run at the same time as the
driver's echo/TX teardown. That contradicts the "No synchronization needed
because the device is bus-off" comment in can_restart().

The commit message says these checks do not serialize with close. It
also says the problem being fixed is "potentially re-enabling hardware
while the driver is tearing it down", and that outcome still looks
reachable here.

Would the check and the restart need to be serialized against close? For
example, can_restart_work() could take rtnl_trylock() or the instance lock
and requeue on contention, or the core could enforce cancel ordering.

The rest of the series appears to fix only rockchip_canfd, through
close_candev_cancel_restart_sync(). The core and drivers such as FlexCAN
are left unchanged.

> @@ -259,13 +262,17 @@ int can_restart_now(struct net_device *dev)

[ ... ]

>  void can_bus_off(struct net_device *dev)
>  {
>  	struct can_priv *priv = netdev_priv(dev);
> +	bool restart;
>  
> -	if (priv->restart_ms)
> +	restart = priv->restart_ms && netif_running(dev);
> +	if (restart)
>  		netdev_info(dev, "bus-off, scheduling restart in %d ms\n",
>  			    priv->restart_ms);
>  	else
> @@ -273,7 +280,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));
>  }

[Severity: High]
Can restart_work still be queued after close_candev() has already
cancelled it?

The netif_running() test and schedule_delayed_work() are two separate
steps with no lock between them. The cancel in close_candev() is not tied
to either step:

IRQ thread                          close path
can_bus_off()
  restart == true
                                    __dev_close_many()
                                      clear_bit(__LINK_STATE_START, ...)
                                      ndo_stop()
                                        close_candev()
                                          cancel_delayed_work_sync()
                                          (nothing queued yet)
  schedule_delayed_work(&priv->restart_work, ...)

HI3110 hits this case. hi3110_stop() calls close_candev() before free_irq(),
so its IRQ thread can still be inside can_bus_off() when the cancel runs:

	close_candev(net);

	priv->force_quit = 1;
	free_irq(spi->irq, priv);

On removal, free_candev() only frees the netdev. It does not cancel
restart_work, which lives inside netdev_priv(dev):

void free_candev(struct net_device *dev)
{
	free_netdev(dev);
}

Would can_restart_work() then run on a freed can_priv? The new
netif_running(priv->dev) check would itself read freed memory.

On a fast reopen within restart_ms, open_candev() does not cancel the stale
work, and netif_running() is true again. The new check in can_restart()
can't tell stale pre-close work apart from work for the new open session.

can_flush_echo_skb() would then free priv->echo_skb[] entries while the
driver's xmit and TX completion paths may be using them. After that it
calls netif_carrier_on() and do_set_mode(CAN_MODE_START) on a healthy
controller.

The underlying lifetime problem predates this patch, and the new check
narrows the window. But the commit message says recovery "can therefore be
queued or started after the interface has begun closing". The queuing side
still seems reachable through this window.

include/linux/workqueue.h already provides disable_delayed_work_sync() and
enable_delayed_work(). If close_candev() used disable_delayed_work_sync()
and open_candev() used enable_delayed_work(), any late
schedule_delayed_work() would become a no-op for every driver. Would that
work better than relying on the unlocked running check?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com

Re: [PATCH net v9 05/10] can: dev: add close_candev_cancel_restart_sync()

[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 patch adds close_candev_cancel_restart_sync() as a static
  inline in include/linux/can/dev.h and moves close_candev() onto it.

--

can: dev: add close_candev_cancel_restart_sync()

This commit adds a static inline helper, close_candev_cancel_restart_sync(),
to include/linux/can/dev.h. The helper synchronously cancels the CAN core
bus-off restart work, and close_candev() now calls it. Drivers can then
wait for recovery to finish before they tear down IRQ and hardware
resources, and echo skb cleanup stays in close_candev().

> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 346faddee02eb..2a286f906394b 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h

[ ... ]

> @@ -115,6 +116,22 @@ struct can_priv *safe_candev_priv(struct net_device *dev);
>  
>  int open_candev(struct net_device *dev);
>  void close_candev(struct net_device *dev);
> +
> +/**
> + * close_candev_cancel_restart_sync() - cancel bus-off restart synchronously
> + * @dev: CAN network device
> + *
> + * Call before tearing down resources used by the restart callback.
> + * The driver must quiesce restart work producers before calling
> + * 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);
> +}

[Severity: Low]
This isn't a runtime bug, but should this patch carry a stable tag? The
alternative would be for the following Rockchip fix to list it as a
prerequisite.

The next patch in the series, "can: rockchip_canfd: cancel bus-off restart
before closing the device", has Fixes: ff60bfbaf67f and
Cc: stable@vger.kernel.org. It calls the new helper at the start of
rkcanfd_stop() in drivers/net/can/rockchip/rockchip_canfd-core.c:

    close_candev_cancel_restart_sync(ndev);

This commit has no Cc: stable tag, and the Rockchip fix does not name it
as a dependency.

If stable picks up only the tagged Rockchip fix, would the backport fail
to build? close_candev_cancel_restart_sync() would be undefined there.

Documentation/process/stable-kernel-rules.rst gives this form for
prerequisites:

    Cc: <stable@vger.kernel.org> # x.y.z: <sha>: <subject>

Other patches in the series already state their backport dependencies.
The earlier core patch "can: dev: check netif_running() before bus-off
recovery" is also tagged for stable.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com

Re: [PATCH net v9 06/10] can: rockchip_canfd: cancel bus-off restart before closing the device

[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 patch has a missing stable dependency annotation.

--

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

The patch adds a call to close_candev_cancel_restart_sync() at the start
of rkcanfd_stop(). Today restart_work is only cancelled in close_candev(),
which runs after the IRQ has been freed and the controller stopped. The
earlier cancel is meant to stop a concurrent bus-off restart from turning
the controller back on after IRQ teardown.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 1cae86973da5a..59a79fe057e66 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -758,6 +758,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);

[Severity: Low]
This patch has Fixes: ff60bfbaf67f and Cc: stable@vger.kernel.org. Will it
build when backported on its own?

The close_candev_cancel_restart_sync() helper is only added to
include/linux/can/dev.h by the earlier commit "can: dev: add
close_candev_cancel_restart_sync()". That commit has no Fixes: tag and no
Cc: stable tag:

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

The early cancel also appears to depend on the earlier commit "can: dev:
check netif_running() before bus-off recovery". That commit adds these
checks in drivers/net/can/dev/dev.c:

can_bus_off():
	restart = priv->restart_ms && netif_running(dev);

can_restart():
	if (!netif_running(dev))
		return -ENETDOWN;

Without those checks, a bus-off IRQ can still arrive after the new early
cancel but before free_irq(). In that case can_bus_off() would requeue
restart_work.

can_restart() would then call do_set_mode(CAN_MODE_START) unconditionally:

can_restart_work()
  can_restart()
    priv->do_set_mode(dev, CAN_MODE_START)
      rkcanfd_set_mode()
        rkcanfd_chip_start()

That turns the controller and its interrupts back on and schedules
timestamp work after free_irq() and rkcanfd_chip_stop_sync(). This is the
same failure the commit message describes. The cancel_delayed_work_sync()
in close_candev() runs after rkcanfd_chip_stop_sync(), so it comes too late
to prevent it.

In mainline both prerequisites come earlier in the series, so the in-tree
fix is complete. Other patches in this series say what their backports need
(for example, the "Backports must include ..." notes in "can:
rockchip_canfd: retry the outstanding TX buffer" and "can: rockchip_canfd:
serialize TX state and command writes").

Could this patch carry a similar note, or Stable-dep-of annotations, for
both of these dependencies?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com

Re: [PATCH net v9 08/10] can: rockchip_canfd: serialize TX with bus-off and restart

[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 adds priv->irq_suspended, a plain bool used as an
  ad-hoc gate for the IRQ handler.
- [Low] The commit message for "can: rockchip_canfd: serialize TX with
  bus-off and restart" carries Cc: stable@vger.kernel.org and Fixes:…
- [Low] rkcanfd_chip_set_reset_mode() writes RKCANFD_REG_INT_MASK =
  RKCANFD_REG_INT_ALL immediately after reset_control_deassert(), with…

--

can: rockchip_canfd: serialize TX with bus-off and restart

This patch adds a tx_enabled flag, protected by tx_lock, that stops TX
submission and RX echo handling before bus-off is published and while the
controller restarts. It also adds an irq_suspended flag that keeps the
shared IRQ handler out during reinitialization, restores INT_MASK right
after reset deassertion, and moves timestamp work initialization to probe.

> Fixes: ff60bfbaf67f ("can: rockchip_canfd: add driver for Rockchip CAN-FD controller")
> Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
> Cc: stable@vger.kernel.org

[Severity: Low]
This patch is tagged for stable. Should the commit message also list the
patches a backport depends on?

It relies on several earlier changes:

- the tx_lock critical sections in rkcanfd_start_xmit() and
  rkcanfd_handle_rx_int_one(), from "can: rockchip_canfd: serialize TX
  state and command writes"
- the close_candev_cancel_restart_sync() call in rkcanfd_stop(), from
  "can: rockchip_canfd: cancel bus-off restart before closing the device"
  and "can: dev: add close_candev_cancel_restart_sync()"
- the netif_running() check in can_restart(), from "can: dev: check
  netif_running() before bus-off recovery"
- the skb_get() echo ownership handling, from "can: rockchip_canfd:
  prevent TX stall on echo skb failure"

The commit that adds close_candev_cancel_restart_sync() has neither a Cc:
stable nor a Fixes: tag.

Earlier patches in the series state their dependencies ("Backports must
include...", "Backports need..."). This patch has the most dependencies
but lists none.

A backport that only follows the Fixes: tags would fail to apply. A version
adapted by hand would lack the serialization that the tx_enabled checks
depend on.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index d05264605efc2..a5c6331191d17 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c

[ ... ]

> @@ -116,6 +118,10 @@ static void rkcanfd_chip_set_reset_mode(const struct rkcanfd_priv *priv)
>  	udelay(2);
>  	reset_control_deassert(priv->reset);
>  
> +	/* Reset clears INT_MASK; restore it before configuring the controller. */
> +	rkcanfd_write(priv, RKCANFD_REG_INT_MASK, RKCANFD_REG_INT_ALL);
> +	rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
> +
>  	rkcanfd_write(priv, RKCANFD_REG_MODE, 0x0);
>  }

[Severity: Low]
Is the CAN core known to be out of reset when this INT_MASK write
arrives?

On Rockchip, reset_control_deassert() goes to rockchip_softrst_deassert().
That function does one posted write to the CRU, with no read-back and no
delay:

drivers/clk/rockchip/softrst.c:rockchip_softrst_deassert() {
    ...
	if (softrst->flags & ROCKCHIP_SOFTRST_HIWORD_MASK) {
		writel((BIT(offset) << 16), softrst->reg_base + (bank * 4));
    ...
}

The CRU and the CAN controller are separate peripherals. The barrier in
writel() orders when writes are issued, not when they reach the device.
So the INT_MASK write could reach the CAN IP while it is still in reset.

If that write is lost, INT_MASK stays at its reset value of 0, which
leaves every interrupt unmasked. That lasts through the FIFO, timestamp
and bittiming setup, until rkcanfd_chip_interrupts_disable() runs later
in rkcanfd_chip_start(). The read-back does not detect this because its
value is thrown away.

The commit message says:

  "Restore and flush the full mask immediately after deassertion so the
  remaining controller configuration stays masked."

On the restart path, irq_suspended is true during this window. An
interrupt raised then would get IRQ_NONE without an ack, and the shared
line would stay asserted until the next mask write.

This depends on the arrival order between the CRU and the CAN block and
on how long the reset takes to release. I could not confirm either
without the SoC documentation.

Should there be a short settle delay after deassertion, or a check of the
read-back value?

[ ... ]

> @@ -356,9 +390,17 @@ static int rkcanfd_set_mode(struct net_device *ndev,
>  
>  	switch (mode) {
>  	case CAN_MODE_START:
> +		rkcanfd_tx_disable(priv);
> +		rkcanfd_chip_interrupts_disable(priv);
> +		/* Flush the interrupt mask write before suspending the handler. */
> +		rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
> +		WRITE_ONCE(priv->irq_suspended, true);
> +		/* Wait for handlers that already passed the suspended check. */
> +		synchronize_irq(ndev->irq);
> +		rkcanfd_timestamp_stop_sync(priv);
> +
>  		rkcanfd_chip_start(priv);
> -		rkcanfd_chip_interrupts_enable(priv);
> -		netif_wake_queue(ndev);
> +		rkcanfd_chip_start_finish(priv);
>  		break;

[Severity: Medium]
Is irq_suspended working as a lock here?

Two paths set it before a block of work. rkcanfd_set_mode() sets it
before the controller is reinitialized. rkcanfd_open() sets it before
rkcanfd_chip_start() and request_irq().

In both paths, rkcanfd_chip_start_finish() clears it:

	smp_store_release(&priv->irq_suspended, false);

rkcanfd_irq() checks the flag without taking a lock:

	if (smp_load_acquire(&priv->irq_suspended))
		return IRQ_NONE;

This keeps the handler away from state that rkcanfd_chip_start()
rewrites: reg_int_mask_default, reg_mode_default, bec, tx_head/tx_tail,
cc/tc and can.state.

I did not find a concrete failure in the current code. synchronize_irq()
takes desc->lock, which makes the WRITE_ONCE() visible to handlers that
start after it. The release/acquire pair makes the reinitialized state
visible.

Even so, the exclusion depends on synchronize_irq() and on the rule that
only one instance of a handler runs at a time. Lockdep cannot see either.

It also holds only as long as every path that sets the flag calls
synchronize_irq() afterwards. Other code that uses the same state, such
as the do_get_berr_counter callback rkcanfd_get_berr_counter(), is not
covered by the flag.

The networking driver guidelines treat a flag that is set on entry and
cleared on exit to block concurrent access as an ad-hoc lock. They ask
for a real lock or RCU instead. Could this use a real lock?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com

Re: [PATCH net v9 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off

[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 "can: rockchip_canfd: prevent timestamp work from rearming
  after bus-off" is tagged Cc: stable but does not say it depends on the…

--

can: rockchip_canfd: prevent timestamp work from rearming after bus-off

After bus-off, the rockchip_canfd timestamp worker could requeue itself
and keep polling. This patch switches the worker to disable_delayed_work()
and enable_delayed_work() to stop that. It also adds timestamp_lock and
timestamp_active so counter reads are serialized with start and stop, and
rkcanfd_timestamp_stop_sync() now calls the non-sync stop before it
cancels the work.

> Keep workqueue operations outside timestamp_lock; the IRQ and restart
> paths already serialize start and stop. Synchronous stop waits for the
> callback before reinitialization or clock shutdown.
>
> Fixes: 4e1a18bab124 ("can: rockchip_canfd: add hardware timestamping support")
> Cc: stable@vger.kernel.org

[Severity: Low]
This doesn't affect the current tree, but should this patch say that
it depends on the previous commit, "can: rockchip_canfd: serialize TX
with bus-off and restart"?

That commit created the one-time probe helper that this patch extends.
It moved INIT_DELAYED_WORK() out of rkcanfd_timestamp_init(), which
runs on every chip start. With this patch, rkcanfd_timestamp_work_init()
in rockchip_canfd-timestamp.c becomes:

	spin_lock_init(&priv->timestamp_lock);
	priv->timestamp_active = false;
	INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
	disable_delayed_work(&priv->timestamp);

Three calls need to stay balanced: the disable at probe,
enable_delayed_work() in rkcanfd_timestamp_start(), and
disable_delayed_work() in rkcanfd_timestamp_stop(). That only works if
the work is initialized exactly once.

The sentence "the IRQ and restart paths already serialize start and
stop" also relies on that commit. It added the restart quiescing in
rkcanfd_set_mode() for CAN_MODE_START:

	WRITE_ONCE(priv->irq_suspended, true);
	/* Wait for handlers that already passed the suspended check. */
	synchronize_irq(ndev->irq);
	rkcanfd_timestamp_stop_sync(priv);

Without the prerequisite, this patch won't apply to stable trees.
Suppose a backport fixes the conflict by hand and leaves
INIT_DELAYED_WORK() in rkcanfd_timestamp_init(). Then every chip start
resets the disable count to 0 just before enable_delayed_work() runs.
That would trigger the workqueue disable count underflow warning and
undo the requeue blocking added here.

The prerequisite is in the same series and is also Cc: stable, so
stable tooling would normally apply both in order. Could the dependency
still be made explicit? A prerequisite note on the Cc: stable line, as
stable-kernel-rules.rst describes, or a "Backports must include ..."
line would do it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com