* [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* 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
* [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* 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
* [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* 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
* [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, ×tamp);
+ 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* 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
* [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* 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
* [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 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