mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races
@ 2026-11-08 11:33 Cunhao Lu via B4 Relay
  2026-11-08 11:33 ` [PATCH net v8 01/10] can: rockchip_canfd: prevent TX stall on echo skb failure Cunhao Lu via B4 Relay
                   ` (10 more replies)
  0 siblings, 11 replies; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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 Rockchip IRQ handler stays suspended until initialization is complete.
Timestamp locking prevents workers from rearming after deactivation and
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 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/

To: Marc Kleine-Budde <mkl@pengutronix.de>
To: kernel@pengutronix.de
To: Vincent Mailhol <mailhol@kernel.org>
To: Heiko Stuebner <heiko@sntech.de>
To: Oliver Hartkopp <socketcan@hartkopp.net>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Simon Horman <horms@kernel.org>
To: Jonathan Corbet <corbet@lwn.net>
To: Shuah Khan <skhan@linuxfoundation.org>
To: Randy Dunlap <rdunlap@infradead.org>
To: Wolfgang Grandegger <wg@grandegger.com>
Cc: linux-can@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-rockchip@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-doc@vger.kernel.org

---
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     | 66 ++++++++++++++++++++--
 drivers/net/can/rockchip/rockchip_canfd-rx.c       | 29 +++++++---
 .../net/can/rockchip/rockchip_canfd-timestamp.c    | 43 ++++++++++++--
 drivers/net/can/rockchip/rockchip_canfd-tx.c       | 51 ++++++++++++++---
 drivers/net/can/rockchip/rockchip_canfd.h          | 11 +++-
 include/linux/can/dev.h                            | 17 ++++++
 8 files changed, 209 insertions(+), 35 deletions(-)
---
base-commit: 26e74534310157d284fe9f2518c1503e055b6210
change-id: 20261013-rockchip-canfd-restart-close-ebde5954f1bf

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 01/10] can: rockchip_canfd: prevent TX stall on echo skb failure
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-08 11:33 ` [PATCH net v8 02/10] can: rockchip_canfd: serialize TX state and command writes Cunhao Lu via B4 Relay
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 02/10] can: rockchip_canfd: serialize TX state and command writes
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
  2026-11-08 11:33 ` [PATCH net v8 01/10] can: rockchip_canfd: prevent TX stall on echo skb failure Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-08 11:33 ` [PATCH net v8 03/10] can: rockchip_canfd: retry the outstanding TX buffer Cunhao Lu via B4 Relay
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 03/10] can: rockchip_canfd: retry the outstanding TX buffer
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
  2026-11-08 11:33 ` [PATCH net v8 01/10] can: rockchip_canfd: prevent TX stall on echo skb failure Cunhao Lu via B4 Relay
  2026-11-08 11:33 ` [PATCH net v8 02/10] can: rockchip_canfd: serialize TX state and command writes Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-09 11:37   ` netdev-bot+sashiko
  2026-11-08 11:33 ` [PATCH net v8 04/10] can: dev: check netif_running() before bus-off recovery Cunhao Lu via B4 Relay
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 04/10] can: dev: check netif_running() before bus-off recovery
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
                   ` (2 preceding siblings ...)
  2026-11-08 11:33 ` [PATCH net v8 03/10] can: rockchip_canfd: retry the outstanding TX buffer Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-09 11:37   ` netdev-bot+sashiko
  2026-11-08 11:33 ` [PATCH net v8 05/10] can: dev: add close_candev_cancel_restart_sync() Cunhao Lu via B4 Relay
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 05/10] can: dev: add close_candev_cancel_restart_sync()
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
                   ` (3 preceding siblings ...)
  2026-11-08 11:33 ` [PATCH net v8 04/10] can: dev: check netif_running() before bus-off recovery Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-08 11:33 ` [PATCH net v8 06/10] can: rockchip_canfd: cancel bus-off restart before closing the device Cunhao Lu via B4 Relay
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 06/10] can: rockchip_canfd: cancel bus-off restart before closing the device
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
                   ` (4 preceding siblings ...)
  2026-11-08 11:33 ` [PATCH net v8 05/10] can: dev: add close_candev_cancel_restart_sync() Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-09 11:37   ` netdev-bot+sashiko
  2026-11-08 11:33 ` [PATCH net v8 07/10] can: rockchip_canfd: disable RX offload on open failure Cunhao Lu via B4 Relay
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 07/10] can: rockchip_canfd: disable RX offload on open failure
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
                   ` (5 preceding siblings ...)
  2026-11-08 11:33 ` [PATCH net v8 06/10] can: rockchip_canfd: cancel bus-off restart before closing the device Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-08 11:33 ` [PATCH net v8 08/10] can: rockchip_canfd: serialize TX with bus-off and restart Cunhao Lu via B4 Relay
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 08/10] can: rockchip_canfd: serialize TX with bus-off and restart
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
                   ` (6 preceding siblings ...)
  2026-11-08 11:33 ` [PATCH net v8 07/10] can: rockchip_canfd: disable RX offload on open failure Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-09 11:37   ` netdev-bot+sashiko
  2026-11-08 11:33 ` [PATCH net v8 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off Cunhao Lu via B4 Relay
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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.

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 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     | 63 +++++++++++++++++++---
 drivers/net/can/rockchip/rockchip_canfd-rx.c       | 10 +++-
 .../net/can/rockchip/rockchip_canfd-timestamp.c    |  6 ++-
 drivers/net/can/rockchip/rockchip_canfd-tx.c       |  4 ++
 drivers/net/can/rockchip/rockchip_canfd.h          |  5 +-
 5 files changed, 79 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..536d9bc812afe 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -20,6 +20,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/smp.h>
 #include <linux/string.h>
 
 #include "rockchip_canfd.h"
@@ -325,6 +326,35 @@ 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;
+
+	/* Serialize carrier, IRQ enable and queue wake with a new bus-off. */
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	/* A manual restart may have raced with the old bus-off IRQ's
+	 * netif_carrier_off(). That IRQ is now drained; restore carrier
+	 * before resuming IRQ handling so a new bus-off takes precedence.
+	 */
+	netif_carrier_on(priv->ndev);
+	priv->tx_enabled = true;
+	/* Publish the initialized state before resuming IRQ handling. */
+	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 +386,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 +630,12 @@ static int rkcanfd_handle_state_error_int(struct rkcanfd_priv *priv)
 	 * handles cf == NULL.
 	 */
 	skb = rkcanfd_alloc_can_err_skb(priv, &cf, &timestamp);
+	if (new_state == CAN_STATE_BUS_OFF) {
+		/* Drain TX before publishing BUS_OFF, which permits a manual
+		 * restart to flush echo SKBs before calling our set_mode().
+		 */
+		rkcanfd_tx_disable(priv);
+	}
 	can_change_state(ndev, cf, tx_state, rx_state);
 
 	if (new_state == CAN_STATE_BUS_OFF) {
@@ -666,6 +710,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 +781,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 +790,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 +809,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 +986,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..fb98dd0da4913 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,15 @@ 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);
+		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..3664124a8da25 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;
@@ -483,6 +483,8 @@ struct rkcanfd_priv {
 
 	u32 reg_mode_default;
 	u32 reg_int_mask_default;
+	bool tx_enabled;
+	bool irq_suspended;
 	struct rkcanfd_devtype_data devtype_data;
 
 	struct cyclecounter cc;
@@ -554,6 +556,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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
                   ` (7 preceding siblings ...)
  2026-11-08 11:33 ` [PATCH net v8 08/10] can: rockchip_canfd: serialize TX with bus-off and restart Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-09 11:37   ` netdev-bot+sashiko
  2026-11-08 11:33 ` [PATCH net v8 10/10] can: rockchip_canfd: serialize timecounter timestamp conversion Cunhao Lu via B4 Relay
  2026-11-09 19:24 ` [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Jakub Kicinski
  10 siblings, 1 reply; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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 while the controller remains bus-off.

Add a dedicated lock and an active flag, initialized once at probe.
Serialize the worker's active check, counter read and self-requeue with
start and stop. Clear the flag under the lock before cancelling work
so no worker can read the counter or rearm after deactivation. Enable
the flag and schedule the initial work under the same lock at start.

Keep both cancellation calls outside the lock so synchronous stop can
wait for the worker to finish.

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

diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
index ad582afbbd68..1d2a122770dc 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
@@ -30,18 +30,27 @@ 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);
-
-	schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
+	spin_lock_irqsave(&priv->timestamp_lock, flags);
+	if (priv->timestamp_active) {
+		timecounter_read(&priv->tc);
+
+		/* Serialize requeueing with timestamp deactivation. */
+		schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
+	}
+	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
 }
 
 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);
 }
 
+/* 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 +102,34 @@ void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
 
 void rkcanfd_timestamp_start(struct rkcanfd_priv *priv)
 {
-	timecounter_init(&priv->tc, &priv->cc, ktime_get_real_ns());
+	unsigned long flags;
 
+	spin_lock_irqsave(&priv->timestamp_lock, flags);
+	timecounter_init(&priv->tc, &priv->cc, ktime_get_real_ns());
+	priv->timestamp_active = true;
 	schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
+	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
 }
 
 void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->timestamp_lock, flags);
+	priv->timestamp_active = false;
+	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
+
 	cancel_delayed_work(&priv->timestamp);
 }
 
 void rkcanfd_timestamp_stop_sync(struct rkcanfd_priv *priv)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->timestamp_lock, flags);
+	priv->timestamp_active = false;
+	spin_unlock_irqrestore(&priv->timestamp_lock, flags);
+
+	/* The worker needs timestamp_lock to finish. */
 	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 3664124a8da2..078876769a05 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;
+	/* Protects timestamp_active and worker reads/requeueing. */
+	spinlock_t timestamp_lock;
 	void __iomem *regs;
 	unsigned int tx_head;
 	unsigned int tx_tail;
@@ -485,6 +487,7 @@ struct rkcanfd_priv {
 	u32 reg_int_mask_default;
 	bool tx_enabled;
 	bool irq_suspended;
+	bool timestamp_active;
 	struct rkcanfd_devtype_data devtype_data;
 
 	struct cyclecounter cc;

-- 
2.34.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net v8 10/10] can: rockchip_canfd: serialize timecounter timestamp conversion
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
                   ` (8 preceding siblings ...)
  2026-11-08 11:33 ` [PATCH net v8 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off Cunhao Lu via B4 Relay
@ 2026-11-08 11:33 ` Cunhao Lu via B4 Relay
  2026-11-09 11:37   ` netdev-bot+sashiko
  2026-11-09 19:24 ` [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Jakub Kicinski
  10 siblings, 1 reply; 18+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-11-08 11:33 UTC (permalink / raw)
  To: Marc Kleine-Budde, kernel, Vincent Mailhol, Heiko Stuebner,
	Oliver Hartkopp, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger
  Cc: Cunhao Lu, linux-can, linux-arm-kernel, linux-rockchip,
	linux-kernel, netdev, linux-doc, stable

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 1d2a122770dc..e255bb33a20d 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 078876769a05..ecb9e2d6aa19 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;
-	/* Protects timestamp_active and worker reads/requeueing. */
+	/* Protects tc, timestamp_active and worker requeueing. */
 	spinlock_t timestamp_lock;
 	void __iomem *regs;
 	unsigned int tx_head;
@@ -557,7 +557,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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net v8 03/10] can: rockchip_canfd: retry the outstanding TX buffer
  2026-11-08 11:33 ` [PATCH net v8 03/10] can: rockchip_canfd: retry the outstanding TX buffer Cunhao Lu via B4 Relay
@ 2026-11-09 11:37   ` netdev-bot+sashiko
  0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 11:37 UTC (permalink / raw)
  To: 1579567540
  Cc: mkl, kernel, mailhol, heiko, socketcan, davem, edumazet, kuba,
	pabeni, horms, corbet, skhan, rdunlap, wg, linux-can,
	linux-arm-kernel, linux-rockchip, linux-kernel, netdev,
	linux-doc

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" states the user-visible failure ("the pending…

--

can: rockchip_canfd: retry the outstanding TX buffer

This patch changes rkcanfd_xmit_retry() so the erratum 6 retransmission
uses tx_tail instead of tx_head to pick the TX buffer. The goal is to retry
the frame that is still pending rather than the next free buffer. It is
tagged for stable with a Fixes: tag pointing at a5605d61c7dd.

> 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_tail indexing looks correct. The stable justification is harder to
check, though, because the stall described here depends on how the
hardware handles TX_REQ(n), and the commit message doesn't say.

The controller has one TX frame register window (RKCANFD_REG_FD_TXFRAMEINFO,
RKCANFD_REG_FD_TXID, RKCANFD_REG_FD_TXDATA0..) and two request bits. Every
frame is written into that shared window in rkcanfd_start_xmit() before
TX_REQ(tx_head) is issued.

rkcanfd_xmit_retry() has one caller, rkcanfd_rxstx_filter(). That caller
only gets that far when the nominal frame echo_skb[tx_tail] is EFF:

drivers/net/can/rockchip/rockchip_canfd-rx.c:rkcanfd_rxstx_filter() {
    ...
	if (!(cfd_nominal->can_id & CAN_EFF_FLAG) ||
	    (cfd_rx->can_id & CAN_EFF_FLAG))
		return 0;
    ...
	rkcanfd_xmit_retry(priv);
    ...
}

While the tail frame is EFF, no further frame can be queued:

drivers/net/can/rockchip/rockchip_canfd-tx.c:rkcanfd_get_effective_tx_free() {
    ...
	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6 &&
	    rkcanfd_tx_tail_is_eff(priv))
		return 0;
    ...
}

So whenever the retry runs, exactly one frame should be pending, and it
should be the last frame written into the shared window.

If writing TX_REQ(n) latches the current window into buffer n, wouldn't the
old TX_REQ(tx_head) have resent the correct frame? The echo would then
match echo_skb[tx_tail] and completion would go ahead normally.

As far as I can tell, the stall only happens if buffer n keeps its old
contents and TX_REQ(n) resends them without reloading from the window.

Was the stall actually seen on hardware, or is this mainly making the slot
indexing consistent with the echo_skb[tx_tail] model? The Erratum 6 comment
in rockchip_canfd.h has a canfdtest reproducer, and a5605d61c7dd carries a
Tested-by for the depth-2 change.

Could the commit message say which TX_REQ(n) behaviour the fix relies on,
or add a Tested-by or reproducer, so stable backporters can judge it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae%40qq.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net v8 04/10] can: dev: check netif_running() before bus-off recovery
  2026-11-08 11:33 ` [PATCH net v8 04/10] can: dev: check netif_running() before bus-off recovery Cunhao Lu via B4 Relay
@ 2026-11-09 11:37   ` netdev-bot+sashiko
  0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 11:37 UTC (permalink / raw)
  To: 1579567540
  Cc: mkl, kernel, mailhol, heiko, socketcan, davem, edumazet, kuba,
	pabeni, horms, corbet, skhan, rdunlap, wg, linux-can,
	linux-arm-kernel, linux-rockchip, linux-kernel, netdev,
	linux-doc

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

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

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

- [High] The new netif_running() checks in can_bus_off() and
  can_restart() (drivers/net/can/dev/dev.c) are lockless test_bit()…

--

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

This adds netif_running() checks to can_restart() and can_bus_off(), so
bus-off recovery is not queued or started once the interface has begun
closing. The documentation now says recovery is skipped when 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/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");
>  

[ ... ]

>  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]
Do these lockless netif_running() reads actually prevent what the commit
message describes?

  "Recovery can therefore be queued or started after the interface has
  begun closing, potentially re-enabling hardware while the driver is
  tearing it down."

Both checks are plain test_bit() reads of __LINK_STATE_START.
__dev_close_many() clears that bit under RTNL. Neither the restart
worker nor the IRQ producers take RTNL, so nothing orders the reads
against the clear.

For the worker, can_restart_work() can pass the new check just before
the bit is cleared:

CPU1 (restart worker)
can_restart_work()
  can_restart()
    netif_running() returns true

CPU2 (ip link set down)
__dev_close_many()
  clear_bit(__LINK_STATE_START, &dev->state)
  flexcan_close()
    flexcan_chip_interrupts_disable()
    free_irq()
    flexcan_chip_stop_disable_on_error()
    flexcan_transceiver_disable()

CPU1
    can_flush_echo_skb()
    netif_carrier_on()
    priv->do_set_mode(dev, CAN_MODE_START)

CPU2
    close_candev()
      cancel_delayed_work_sync()

flexcan_close() only waits for the worker in close_candev(), and by then
the hardware is already down. Can the restart callback still re-enable a
controller while it is being shut down?

For producers, can_bus_off() reads the running state and calls
schedule_delayed_work() later. Nothing makes those two steps atomic.
hi3110_stop() calls close_candev() before it quiesces the IRQ thread:

drivers/net/can/spi/hi311x.c:hi3110_stop() {
    ...
	close_candev(net);

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

hi3110_can_ist()->can_bus_off() can see running == true and then be
preempted. Meanwhile close clears the bit, and cancel_delayed_work_sync()
finds nothing pending. The IRQ thread then queues restart_work.
free_irq() waits for the IRQ thread, but it does not cancel the queued
work.

What happens to that stale work afterwards?

On a quick reopen, open_candev() only turns the carrier back on. It does
not cancel restart_work, so the stale work passes netif_running() again.
can_restart() has no priv->state == CAN_STATE_BUS_OFF check, so it calls
can_flush_echo_skb() while the TX path is live:

	/* No synchronization needed because the device is bus-off and
	 * no messages can come in or go out.
	 */
	can_flush_echo_skb(dev);

Could this free echo_skb[] entries while can_put_echo_skb() or
can_get_echo_skb() is using them? It would also call
do_set_mode(CAN_MODE_START) on a controller that is not bus-off.

On removal, free_candev() is just free_netdev(), and nothing cancels
restart_work after the last close. Would can_restart_work() then
dereference priv->dev from a freed can_priv, before the new check runs?

These stale-work consequences existed before this patch. The commit
message's HI3110 notes acknowledge the window. The later "can: dev: add
close_candev_cancel_restart_sync()" in the series only wraps the same
cancel_delayed_work_sync(). The netif_running() gate makes the windows
smaller but does not close them, which is the flag-as-gate-guard
pattern.

Would it be more robust to call disable_delayed_work_sync() in
close_candev() and enable_delayed_work() in open_candev()? Failing that,
the work could be cancelled in open_candev() and free_candev(), with a
priv->state check in can_restart(). Either option would replace the
netif_running() checks.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae%40qq.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net v8 06/10] can: rockchip_canfd: cancel bus-off restart before closing the device
  2026-11-08 11:33 ` [PATCH net v8 06/10] can: rockchip_canfd: cancel bus-off restart before closing the device Cunhao Lu via B4 Relay
@ 2026-11-09 11:37   ` netdev-bot+sashiko
  0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 11:37 UTC (permalink / raw)
  To: 1579567540
  Cc: mkl, kernel, mailhol, heiko, socketcan, davem, edumazet, kuba,
	pabeni, horms, corbet, skhan, rdunlap, wg, linux-can,
	linux-arm-kernel, linux-rockchip, linux-kernel, netdev,
	linux-doc

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] rkcanfd_stop() (drivers/net/can/rockchip/rockchip_canfd-
  core.c:761) now calls close_candev_cancel_restart_sync().

Pre-existing issues:
- [High] At this commit, the bus-off restart path re-initializes a
  delayed_work that may still be pending or running.
- [High] At this commit, rkcanfd_open() enables RX offload with
  can_rx_offload_enable(&priv->offload) (rockchip_canfd-core.c:737).

--

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

This patch calls close_candev_cancel_restart_sync() at the start of
rkcanfd_stop(). A running bus-off restart is then cancelled while the IRQ,
RX offload and controller clocks are still available. close_candev() stays
after IRQ teardown for the final cancellation and the echo cleanup.

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

[Severity: Low]
Will this build on a stable tree when it is picked up on its own?

close_candev_cancel_restart_sync() is a static inline helper. It is added
to include/linux/can/dev.h by the preceding commit "can: dev: add
close_candev_cancel_restart_sync()":

include/linux/can/dev.h:
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);
}

That helper commit has no Fixes: tag and no Cc: stable. This patch has
both, but the commit message does not say it depends on the helper. If
stable or AUTOSEL tooling cherry-picks this patch alone, the build fails
with an implicit declaration of close_candev_cancel_restart_sync().

Other stable-tagged patches in this series do name their prerequisites,
for example "Backports must include the preceding TX serialization
fix..." and "Backports require can_put_echo_skb() to consume...".

Could this patch get a similar note, or a "Cc: stable@vger.kernel.org #
<ver>: <prereq>" annotation? Another option is to call
cancel_delayed_work_sync(&priv->can.restart_work) directly here.

>  	netif_stop_queue(ndev);
>  
>  	rkcanfd_chip_interrupts_disable(priv);

[Severity: High]
This isn't a bug introduced by this patch, but the bus-off restart path
nearby looks like it can re-initialize the timestamp delayed_work while
that work is still pending or running.

rkcanfd_handle_state_error_int() runs in hardirq context. It calls
rkcanfd_chip_stop()->rkcanfd_timestamp_stop(), which uses only the non-sync
cancel:

drivers/net/can/rockchip/rockchip_canfd-timestamp.c:
void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv)
{
	cancel_delayed_work(&priv->timestamp);
}

If rkcanfd_timestamp_work() is already running on another CPU, it can
re-arm itself after that cancel returns:

rkcanfd_timestamp_work() {
	...
	timecounter_read(&priv->tc);

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

Later the restart runs this path:

can_restart_work()
  rkcanfd_set_mode(CAN_MODE_START)
    rkcanfd_chip_start()
      rkcanfd_timestamp_init()
        INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);

Can that INIT_DELAYED_WORK() run on a timer that is still enqueued, and
corrupt the timer base list or drop the pending bit?

There is also a lock-free timecounter_init(&priv->tc, ...) in
rkcanfd_timestamp_start(). It can race with timecounter_read() in the
worker and with timecounter_cyc2time() in the IRQ path.

Later patches in this series appear to fix this:

- INIT_DELAYED_WORK() moves to rkcanfd_timestamp_work_init(), called once
  at probe ("can: rockchip_canfd: serialize TX with bus-off and restart").
- rkcanfd_set_mode() calls rkcanfd_timestamp_stop_sync() before
  rkcanfd_chip_start().
- A timestamp_active flag under timestamp_lock stops the worker from
  re-arming ("can: rockchip_canfd: prevent timestamp work from rearming
  after bus-off").
- The timecounter accesses are serialized by timestamp_lock ("can:
  rockchip_canfd: serialize timecounter timestamp conversion").

[Severity: High]
This is a pre-existing issue, but rkcanfd_open() at this commit does not
undo can_rx_offload_enable() when request_irq() fails:

rkcanfd_open() {
	...
	rkcanfd_chip_start(priv);
	can_rx_offload_enable(&priv->offload);

	err = request_irq(ndev->irq, rkcanfd_irq, IRQF_SHARED, ndev->name, priv);
	if (err)
		goto out_rkcanfd_chip_stop;
	...
out_rkcanfd_chip_stop:
	rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);
	pm_runtime_put(ndev->dev.parent);
out_close_candev:
	close_candev(ndev);
	return err;
}

Because ndo_open() failed, ndo_stop() is never called, so NAPI stays
enabled. Will the next "ip link set canX up" call napi_enable() on a NAPI
that is already enabled, and hit this check in napi_enable_locked()?

	BUG_ON(!test_bit(NAPI_STATE_SCHED, &val));

The next patch in the series, "can: rockchip_canfd: disable RX offload on
open failure", seems to fix this. It adds can_rx_offload_disable() to the
out_rkcanfd_chip_stop path.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae%40qq.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net v8 08/10] can: rockchip_canfd: serialize TX with bus-off and restart
  2026-11-08 11:33 ` [PATCH net v8 08/10] can: rockchip_canfd: serialize TX with bus-off and restart Cunhao Lu via B4 Relay
@ 2026-11-09 11:37   ` netdev-bot+sashiko
  0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 11:37 UTC (permalink / raw)
  To: 1579567540
  Cc: mkl, kernel, mailhol, heiko, socketcan, davem, edumazet, kuba,
	pabeni, horms, corbet, skhan, rdunlap, wg, linux-can,
	linux-arm-kernel, linux-rockchip, linux-kernel, netdev,
	linux-doc

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

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

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` as a hand-rolled gate
  that keeps rkcanfd_irq() out of a code region: -…
- [Medium] rkcanfd_set_mode(CAN_MODE_START) masks device interrupts
  (INT_MASK = INT_ALL), reads the mask back, sets irq_suspended and…
- [Low] This commit carries `Cc: stable@vger.kernel.org` and Fixes: tags
  for the original v6.12-era driver commits (ff60bfbaf67f,…
- [Low] The new `!priv->tx_enabled` early return in
  rkcanfd_handle_rx_int_one() (drivers/net/can/rockchip/rockchip_canfd-…
- [Low] The patch adds `#include <linux/smp.h>` to
  drivers/net/can/rockchip/rockchip_canfd-core.c.

--

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-side echo handling before CAN_STATE_BUS_OFF is
published. It also adds an irq_suspended flag that keeps rkcanfd_irq()
out while the controller is reinitialized on restart and open, and moves
carrier, TX, IRQ and queue restoration into rkcanfd_chip_start_finish().
Timestamp delayed work is now initialized 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

[Severity: Low]
Could the commit message list the backport prerequisites?

This patch is tagged for stable, but it only applies on top of earlier
patches in this series:

  "can: rockchip_canfd: serialize TX state and command writes" adds the
  tx_lock critical sections in rkcanfd_start_xmit() and
  rkcanfd_handle_rx_int_one() that the new tx_enabled checks sit in.

  "can: rockchip_canfd: prevent TX stall on echo skb failure" adds the
  skb_get() and echo failure handling.

  "can: rockchip_canfd: cancel bus-off restart before closing the device"
  adds the close_candev_cancel_restart_sync() context in rkcanfd_stop().

  "can: rockchip_canfd: disable RX offload on open failure" adds the
  can_rx_offload_disable() context in the open error path.

The v8 changelog below the --- line says the TX/RX critical sections were
moved into those earlier patches, but that text is dropped when the patch
is applied.

The chain also depends on "can: dev: add
close_candev_cancel_restart_sync()", which carries neither Cc: stable nor
Fixes:. Other patches in the series, such as "serialize TX state and
command writes" and "retry the outstanding TX buffer", do name their
prerequisites in the commit message.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index d05264605efc2..536d9bc812afe 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -20,6 +20,7 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/smp.h>
>  #include <linux/string.h>
>  
>  #include "rockchip_canfd.h"

[Severity: Low]
This isn't a bug, but is linux/smp.h the intended header here?

The only new symbols that need a header are smp_store_release() in
rkcanfd_chip_start_finish() and smp_load_acquire() in rkcanfd_irq(). Both
come from asm/barrier.h, not include/linux/smp.h. Nothing else in the file
uses linux/smp.h.

[ ... ]

> @@ -356,9 +386,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);

[Severity: Medium]
Is the INT_MASK value written above still in place once
rkcanfd_chip_start() runs?

rkcanfd_chip_start() starts with rkcanfd_chip_set_reset_mode(), which
pulses the controller reset:

rkcanfd_chip_set_reset_mode() {
	reset_control_assert(priv->reset);
	udelay(2);
	reset_control_deassert(priv->reset);

	rkcanfd_write(priv, RKCANFD_REG_MODE, 0x0);
}

The reset puts INT_MASK back to its hardware default. The driver only
writes it again after the ID filter, MODE, FIFO, timestamp and bit-timing
programming:

rkcanfd_chip_start() {
    ...
	rkcanfd_set_bittiming(priv);

	rkcanfd_chip_interrupts_disable(priv);
	rkcanfd_chip_set_work_mode(priv);
    ...
}

For that whole window irq_suspended is true. So rkcanfd_irq() returns
IRQ_NONE without reading or acking RKCANFD_REG_INT.

Suppose the INT_MASK reset default leaves some sources unmasked, and the
controller latches a status bit during this window. Could the
level-triggered, possibly shared, line then stay asserted with nobody
acking it? That would produce IRQ_NONE returns until
rkcanfd_chip_interrupts_disable() runs. If the IRQ goes to the CPU running
set_mode, it could hold off that progress. note_interrupt() could also end
up disabling the line for every device that shares it.

Whether this happens depends on the INT_MASK reset value and on whether
the controller can latch status in reset or config mode. Neither could be
confirmed from the code. Later patches in the series don't appear to
change this sequence.

The commit message says the restart masks device interrupts and keeps
other shared IRQ users enabled, but the mask doesn't seem to survive the
reset. Would re-masking INT_MASK right after reset_control_deassert() help
here? Another option would be to still ack RKCANFD_REG_INT in the
suspended path.

> -		rkcanfd_chip_interrupts_enable(priv);
> -		netif_wake_queue(ndev);
> +		rkcanfd_chip_start_finish(priv);
>  		break;

[ ... ]

> @@ -666,6 +710,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;

[Severity: Medium]
Is irq_suspended acting as a hand-rolled lock around the reinitialization
region?

rkcanfd_set_mode() and rkcanfd_open() set it on entry:

	WRITE_ONCE(priv->irq_suspended, true);

rkcanfd_chip_start_finish() clears it on exit:

	smp_store_release(&priv->irq_suspended, false);

It is checked here without a lock. Its job is to keep rkcanfd_irq() away
from FIFO, timestamp, BQL and INT_MASK state while rkcanfd_chip_start()
reprograms the controller. The networking driver guidelines list a flag
that is set on entry and cleared on exit of a code region to prevent
concurrent access as a bug pattern. Lockdep cannot see it.

No race with the current callers was found, because synchronize_irq()
after the flag store drains handlers that are already running. Even so,
correctness relies on every caller repeating the same sequence: mask,
readback, flag store, then synchronize_irq().

rkcanfd_set_mode() does the full sequence. rkcanfd_open() only sets the
flag. rkcanfd_stop() does not use the flag and relies on free_irq().

While the flag is set, the handler also stops acking device status (see
the INT_MASK question above). Would a real synchronization primitive fit
better here?

[ ... ]

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> index 23476a334d6c3..fb98dd0da4913 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c

[ ... ]

> @@ -242,7 +242,15 @@ 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);
> +		return 0;
> +	}

[Severity: Low]
Should this drop path increment stats->rx_dropped?

By this point rkcanfd_read_rep() has already taken the entry out of
RKCANFD_REG_RX_FIFO_RDATA. The check does not tell self-received echoes
apart from frames sent by other nodes, so valid frames are dropped as
well. The bec.rxerr correction is also skipped.

Every other path in rkcanfd_handle_rx_int_one() that consumes an entry
without delivering it is counted:

  an empty header bumps rx_fifo_empty_errors
  an FDF frame with CAN-FD disabled bumps stats->rx_dropped
  an skb allocation failure bumps stats->rx_dropped

rkcanfd_stop() can reach this branch, because it disables TX before it
masks device interrupts and frees the IRQ:

rkcanfd_stop() {
    ...
	close_candev_cancel_restart_sync(ndev);
	rkcanfd_tx_disable(priv);

	rkcanfd_chip_interrupts_disable(priv);
	free_irq(ndev->irq, priv);
    ...
}

An RX_FINISH interrupt in that window would go through
rkcanfd_irq()->rkcanfd_handle_rx_int()->rkcanfd_handle_rx_int_one(). So
would a handler that is already draining the FIFO on another CPU. Either
way the frames are discarded and do not show up in the standard netdev
stats.

The restart and open paths don't appear to reach this branch.
can_restart_now() requires CAN_STATE_BUS_OFF, so the controller is already
in reset. tx_enabled is also set again before irq_suspended is cleared.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae%40qq.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net v8 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off
  2026-11-08 11:33 ` [PATCH net v8 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off Cunhao Lu via B4 Relay
@ 2026-11-09 11:37   ` netdev-bot+sashiko
  0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 11:37 UTC (permalink / raw)
  To: 1579567540
  Cc: mkl, kernel, mailhol, heiko, socketcan, davem, edumazet, kuba,
	pabeni, horms, corbet, skhan, rdunlap, wg, linux-can,
	linux-arm-kernel, linux-rockchip, linux-kernel, netdev,
	linux-doc

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 is tagged Cc: stable but does not say that it depends
  on the preceding commit "can: rockchip_canfd: serialize TX with bus-…
- [Low] The commit message gives only one consequence: the worker can
  "keep polling while the controller remains bus-off".

Pre-existing issues:
- [Medium] Concern: in this commit, rkcanfd_skb_set_timestamp() reads
  priv->tc through timecounter_cyc2time() from hardirq context without…

--

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

This adds timestamp_lock and a timestamp_active flag to rkcanfd_priv.
With them, the timestamp worker reads the timecounter and requeues itself
only while timestamping is active. The start and stop helpers set or clear
the flag under the lock, and the work is cancelled outside it.

> The bus-off IRQ cancels timestamp work asynchronously. If the worker
> is already running, it can requeue itself after cancellation and keep
> polling while the controller remains bus-off.

[Severity: Low]
Can the commit message name a user-visible consequence of this polling?

The only trigger is the bus-off path:

rkcanfd_handle_state_error_int()
  rkcanfd_chip_stop()
    rkcanfd_timestamp_stop()
      cancel_delayed_work()

As far as I can tell, every way out of bus-off already stops the worker
synchronously:

- Restart through rkcanfd_set_mode() calls rkcanfd_timestamp_stop_sync()
  before rkcanfd_chip_start().
- Close goes through rkcanfd_chip_stop_sync().

In both cases cancel_delayed_work_sync() handles a work item that requeues
itself.

Runtime PM is held until after rkcanfd_chip_stop_sync() in rkcanfd_stop().
That means the stray worker never reads a clock-gated controller.

Anything it adds to priv->tc is thrown away when rkcanfd_timestamp_start()
runs timecounter_init() again.

Given the Cc: stable tag, could the message either describe a concrete
consequence or say that this is a hardening change?

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> index ad582afbbd682..1d2a122770dc8 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> @@ -30,18 +30,27 @@ static void rkcanfd_timestamp_work(struct work_struct *work)

[ ... ]

>  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);
>  }
>  
> +/* The caller must quiesce IRQs and timestamp work before reconfiguration. */
>  void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)

[Severity: Low]
Should the commit message say that this depends on the preceding commit,
"can: rockchip_canfd: serialize TX with bus-off and restart"?

That commit added rkcanfd_timestamp_work_init() and its one-time call from
rkcanfd_probe(). It also moved INIT_DELAYED_WORK() out of
rkcanfd_timestamp_init().

Before it, the work is initialized on every rkcanfd_chip_start(), which
means on every open and every restart:

rkcanfd_timestamp_init() {
    ...
	priv->work_delay_jiffies = div_u64(work_delay_ns, 3u * NSEC_PER_SEC / HZ);
	INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
    ...
}

The new comment on rkcanfd_timestamp_init() also relies on code from that
commit: the synchronize_irq() and rkcanfd_timestamp_stop_sync() calls in
rkcanfd_set_mode().

On its own, this patch won't apply to stable trees. An obvious manual
backport would put spin_lock_init() next to the old INIT_DELAYED_WORK().
That would re-initialize timestamp_lock on every restart, while an old
worker may still hold it.

Other patches in this series have a "Backports must include ..."
paragraph. Would a similar note help here?

[ ... ]

> diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
> index 3664124a8da25..078876769a05d 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;
> +	/* Protects timestamp_active and worker reads/requeueing. */
> +	spinlock_t timestamp_lock;

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch.
priv->tc still has a reader that does not take the lock.
rkcanfd_skb_set_timestamp() calls timecounter_cyc2time() from hardirq
context without timestamp_lock:

rkcanfd_skb_set_timestamp() {
    ...
	ns = timecounter_cyc2time(&priv->tc, timestamp);
    ...
}

Meanwhile, timecounter_read() in rkcanfd_timestamp_work() and
timecounter_init() in rkcanfd_timestamp_start() now change tc under
timestamp_lock.

If a conversion on another CPU races with the worker, can it combine a
new cycle_last with an old nsec? That would give wrong or non-monotonic
hardware timestamps on RX, TX-done and error frames.

The last patch in the series fixes this: "can: rockchip_canfd: serialize
timecounter timestamp conversion". It takes timestamp_lock around
timecounter_cyc2time() in rkcanfd_skb_set_timestamp(). It also updates
this comment to "Protects tc, timestamp_active and worker requeueing."

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae%40qq.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net v8 10/10] can: rockchip_canfd: serialize timecounter timestamp conversion
  2026-11-08 11:33 ` [PATCH net v8 10/10] can: rockchip_canfd: serialize timecounter timestamp conversion Cunhao Lu via B4 Relay
@ 2026-11-09 11:37   ` netdev-bot+sashiko
  0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 11:37 UTC (permalink / raw)
  To: 1579567540
  Cc: mkl, kernel, mailhol, heiko, socketcan, davem, edumazet, kuba,
	pabeni, horms, corbet, skhan, rdunlap, wg, linux-can,
	linux-arm-kernel, linux-rockchip, linux-kernel, netdev,
	linux-doc

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 fix only works because the preceding series patch "can:
  rockchip_canfd: prevent timestamp work from rearming after bus-off"…

--

can: rockchip_canfd: serialize timecounter timestamp conversion

The commit takes timestamp_lock around timecounter_cyc2time() in
rkcanfd_skb_set_timestamp(), so a conversion can no longer run while the
timestamp worker is inside timecounter_read(). It also updates the
comment on timestamp_lock in struct rkcanfd_priv to say that the lock
covers tc.

> 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

[Severity: Low]
Should this commit message state its backport prerequisites?

At 4e1a18bab124, struct rkcanfd_priv has no timestamp_lock and no
timestamp_active field. Also, rkcanfd_timestamp_work() calls
timecounter_read(&priv->tc) with no lock held before it calls
schedule_delayed_work().

The statement "using the same lock as timecounter_read() and
timecounter_init()" is only true once the preceding patch in this
series is applied:

  806691a20048 ("can: rockchip_canfd: prevent timestamp work from
  rearming after bus-off")

That patch adds the lock and the active flag. It also adds the locking
in rkcanfd_timestamp_work() and rkcanfd_timestamp_start():

drivers/net/can/rockchip/rockchip_canfd-timestamp.c:rkcanfd_timestamp_work() {
    ...
	spin_lock_irqsave(&priv->timestamp_lock, flags);
	if (priv->timestamp_active) {
		timecounter_read(&priv->tc);
    ...
}

The paragraph about lock nesting also depends on tx_lock. That lock comes
from the earlier patches "can: rockchip_canfd: serialize TX state and
command writes" and "can: rockchip_canfd: serialize TX with bus-off and
restart".

A plain cherry-pick onto a tree without 806691a20048 would fail to build,
and that patch carries the same Fixes: tag and Cc: stable. But suppose
someone adapts this patch by hand and only adds a lock in
rkcanfd_skb_set_timestamp(). The timecounter_read() in the worker would
still run unlocked, so the race would stay open.

Other patches in this series include a note such as "Backports must
include the preceding TX serialization fix...". Could a similar note be
added here, naming 806691a20048 and the tx_lock patches as prerequisites?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae%40qq.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races
  2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
                   ` (9 preceding siblings ...)
  2026-11-08 11:33 ` [PATCH net v8 10/10] can: rockchip_canfd: serialize timecounter timestamp conversion Cunhao Lu via B4 Relay
@ 2026-11-09 19:24 ` Jakub Kicinski
  10 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2026-11-09 19:24 UTC (permalink / raw)
  To: Cunhao Lu via B4 Relay
  Cc: 1579567540, Marc Kleine-Budde, kernel, Vincent Mailhol,
	Heiko Stuebner, Oliver Hartkopp, David S. Miller, Eric Dumazet,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Wolfgang Grandegger, linux-can, linux-arm-kernel,
	linux-rockchip, linux-kernel, netdev, linux-doc, stable

On Sun, 08 Nov 2026 19:33:27 +0800 Cunhao Lu via B4 Relay wrote:
> Subject: [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races

please use get_maintainer.pl, do not CC netdev on CAN patches directly.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2026-11-09 19:24 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-08 11:33 [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Cunhao Lu via B4 Relay
2026-11-08 11:33 ` [PATCH net v8 01/10] can: rockchip_canfd: prevent TX stall on echo skb failure Cunhao Lu via B4 Relay
2026-11-08 11:33 ` [PATCH net v8 02/10] can: rockchip_canfd: serialize TX state and command writes Cunhao Lu via B4 Relay
2026-11-08 11:33 ` [PATCH net v8 03/10] can: rockchip_canfd: retry the outstanding TX buffer Cunhao Lu via B4 Relay
2026-11-09 11:37   ` netdev-bot+sashiko
2026-11-08 11:33 ` [PATCH net v8 04/10] can: dev: check netif_running() before bus-off recovery Cunhao Lu via B4 Relay
2026-11-09 11:37   ` netdev-bot+sashiko
2026-11-08 11:33 ` [PATCH net v8 05/10] can: dev: add close_candev_cancel_restart_sync() Cunhao Lu via B4 Relay
2026-11-08 11:33 ` [PATCH net v8 06/10] can: rockchip_canfd: cancel bus-off restart before closing the device Cunhao Lu via B4 Relay
2026-11-09 11:37   ` netdev-bot+sashiko
2026-11-08 11:33 ` [PATCH net v8 07/10] can: rockchip_canfd: disable RX offload on open failure Cunhao Lu via B4 Relay
2026-11-08 11:33 ` [PATCH net v8 08/10] can: rockchip_canfd: serialize TX with bus-off and restart Cunhao Lu via B4 Relay
2026-11-09 11:37   ` netdev-bot+sashiko
2026-11-08 11:33 ` [PATCH net v8 09/10] can: rockchip_canfd: prevent timestamp work from rearming after bus-off Cunhao Lu via B4 Relay
2026-11-09 11:37   ` netdev-bot+sashiko
2026-11-08 11:33 ` [PATCH net v8 10/10] can: rockchip_canfd: serialize timecounter timestamp conversion Cunhao Lu via B4 Relay
2026-11-09 11:37   ` netdev-bot+sashiko
2026-11-09 19:24 ` [PATCH net v8 00/10] can: fix Rockchip TX stalls and bus-off restart/close races Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®