All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller if interface is down
@ 2026-10-27  6:50 ChunYuZhiQiang
  2026-10-27  6:50 ` [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV " ChunYuZhiQiang
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: ChunYuZhiQiang @ 2026-10-27  6:50 UTC (permalink / raw)
  To: netdev-bot+sinfo
  Cc: netdev, linux-can, mkl, mailhol, kernel, haibo.chen, shawnguo,
	imx, stable, ChunYuZhiQiang

If the interface is down, the CAN controller might be powered down and
its clocks gated. Don't call do_get_berr_counter() to read the bus error
counters if the interface is down, and adjust can_get_size() to match.

This removes one trigger for a synchronous external abort seen on
i.MX8QXP when "ip link show" is run on a flexcan interface that was
never brought up. The underlying cause is platform-specific: on i.MX8QXP
the CAN clocks are gated through the SCU LPCG and the subsystem shares
the IMX_SC_R_CAN_0 power domain, and there is a window after
pm_runtime_resume_and_get() returns where the clock has not reached the
module yet. This patch avoids the netlink query path while the interface
is down, but does not root-cause the LPCG window itself.

As a result, IFLA_CAN_BERR_COUNTER is no longer reported for down
interfaces. Userspace must treat the attribute as optional.

The issue was triggered on an i.MX8QXP board by running "ip link show"
without ever bringing the interface up:

    Internal error: synchronous external abort: 0000000096000210

    pc : flexcan_read_le+0x0/0x18
    lr : flexcan_get_berr_counter+0x4c/0x8c
    Call trace:
     flexcan_read_le+0x0/0x18
     flexcan_get_berr_counter+0x4c/0x8c
     can_fill_info+0x1f8/0x434
     rtnl_fill_ifinfo+0x8fc/0xbb4
     rtnl_dump_ifinfo+0x364/0x448

This is the same class of problem fixed for m_can in
commit 91a55c72a821 ("can: m_can: m_can_get_berr_counter(): don't
wake up controller if interface is down").

Fixes: ec56acfef2af1 ("can: flexcan: flexcan_get_berr_counter(): switch on clocks before accessing ecr register")
Cc: stable@vger.kernel.org
Signed-off-by: ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht>
---
v8:
 - Split the TDCV gating into a separate net-next-only patch.
 - Drop the "generic logic problem that exists on all platforms"
   wording. This patch removes one trigger, it is not a root-cause fix
   for the i.MX8QXP LPCG clock window.
 - The Fixes: tag points to ec56acfef2af1 because this patch supersedes
   the down-interface readback behavior it introduced, not because that
   commit was buggy.
---
 drivers/net/can/dev/netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 766d455950f5..b958c04d53db 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -812,7 +812,7 @@ static size_t can_get_size(const struct net_device *dev)
 	size += nla_total_size(sizeof(u32));			/* IFLA_CAN_STATE */
 	size += nla_total_size(sizeof(struct can_ctrlmode));	/* IFLA_CAN_CTRLMODE */
 	size += nla_total_size(sizeof(u32));			/* IFLA_CAN_RESTART_MS */
-	if (priv->do_get_berr_counter)				/* IFLA_CAN_BERR_COUNTER */
+	if (priv->do_get_berr_counter && netif_running(dev))	/* IFLA_CAN_BERR_COUNTER */
 		size += nla_total_size(sizeof(struct can_berr_counter));
 	if (priv->termination_const) {
 		size += nla_total_size(sizeof(priv->termination));		/* IFLA_CAN_TERMINATION */
@@ -1006,7 +1006,7 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	    nla_put(skb, IFLA_CAN_CTRLMODE, sizeof(cm), &cm) ||
 	    nla_put_u32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms) ||
 
-	    (priv->do_get_berr_counter &&
+	    (priv->do_get_berr_counter && netif_running(dev) &&
 	     !priv->do_get_berr_counter(dev, &bec) &&
 	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
 
-- 
2.47.3


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

* [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down
  2026-10-27  6:50 [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller if interface is down ChunYuZhiQiang
@ 2026-10-27  6:50 ` ChunYuZhiQiang
  2026-10-27  8:57   ` Vincent Mailhol
                     ` (2 more replies)
  2026-10-27  8:55 ` [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller " Vincent Mailhol
  2026-10-27 21:27 ` Jakub Kicinski
  2 siblings, 3 replies; 9+ messages in thread
From: ChunYuZhiQiang @ 2026-10-27  6:50 UTC (permalink / raw)
  To: netdev-bot+sinfo
  Cc: netdev, linux-can, mkl, mailhol, kernel, haibo.chen, shawnguo,
	imx, stable, ChunYuZhiQiang

The automatic TDCV readback calls into the driver via do_get_auto_tdcv().
Some drivers, such as xcan_get_auto_tdcv(), read controller registers
without taking a runtime PM reference, so calling this while the
interface is down can touch registers whose clocks are gated.

Gate do_get_auto_tdcv() on netif_running() in can_tdc_fill_info() and
adjust can_tdc_get_size() and can_data_bittiming_get_size() so that only
the automatic TDCV size is skipped when the interface is down. The other
TDC parameters are still reported, as they are meaningful while the
interface is down.

This is a defensive hardening based on code inspection; the automatic
TDCV path has not been triggered on hardware.

Signed-off-by: ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht>
---
v8:
 - Split out of the berr-counter patch so that the stable backport only
   carries the hardware-tested change.
---
 drivers/net/can/dev/netlink.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index b958c04d53db..613beee9e936 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -720,8 +720,9 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 	return 0;
 }
 
-static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
-			       u32 tdc_flags)
+static size_t can_tdc_get_size(const struct net_device *dev,
+				 struct data_bittiming_params *dbt_params,
+			     u32 tdc_flags)
 {
 	bool tdc_manual = tdc_flags & CAN_CTRLMODE_TDC_MANUAL_MASK;
 	size_t size;
@@ -742,7 +743,8 @@ static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
 	}
 
 	if (tdc_flags) {
-		if (tdc_manual || dbt_params->do_get_auto_tdcv)
+		if (tdc_manual ||
+		    (dbt_params->do_get_auto_tdcv && netif_running(dev)))
 			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
 		size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO */
 		if (dbt_params->tdc_const->tdcf_max)
@@ -752,7 +754,8 @@ static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
 	return size;
 }
 
-static size_t can_data_bittiming_get_size(struct data_bittiming_params *dbt_params,
+static size_t can_data_bittiming_get_size(const struct net_device *dev,
+					  struct data_bittiming_params *dbt_params,
 					  u32 tdc_flags)
 {
 	size_t size = 0;
@@ -764,7 +767,7 @@ static size_t can_data_bittiming_get_size(struct data_bittiming_params *dbt_para
 	if (dbt_params->data_bitrate_const)		/* IFLA_CAN_{,XL}_DATA_BITRATE_CONST */
 		size += nla_total_size(sizeof(*dbt_params->data_bitrate_const) *
 				       dbt_params->data_bitrate_const_cnt);
-	size += can_tdc_get_size(dbt_params, tdc_flags);/* IFLA_CAN_{,XL}_TDC */
+	size += can_tdc_get_size(dev, dbt_params, tdc_flags);/* IFLA_CAN_{,XL}_TDC */
 
 	return size;
 }
@@ -825,10 +828,10 @@ static size_t can_get_size(const struct net_device *dev)
 	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */
 	size += can_ctrlmode_ext_get_size();			/* IFLA_CAN_CTRLMODE_EXT */
 
-	size += can_data_bittiming_get_size(&priv->fd,
+	size += can_data_bittiming_get_size(dev, &priv->fd,
 					    priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK);
 
-	size += can_data_bittiming_get_size(&priv->xl,
+	size += can_data_bittiming_get_size(dev, &priv->xl,
 					    priv->ctrlmode & CAN_CTRLMODE_XL_TDC_MASK);
 	size += can_pwm_get_size(priv->xl.pwm_const,		/* IFLA_CAN_XL_PWM */
 				 priv->ctrlmode & CAN_CTRLMODE_XL_TMS);
@@ -910,7 +913,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev,
 		if (tdc_manual) {
 			tdcv = tdc->tdcv;
 			err = 0;
-		} else if (dbt_params->do_get_auto_tdcv) {
+		} else if (dbt_params->do_get_auto_tdcv && netif_running(dev)) {
 			err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
 		}
 		if (!err && nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdcv))
-- 
2.47.3


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

* Re: [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller if interface is down
  2026-10-27  6:50 [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller if interface is down ChunYuZhiQiang
  2026-10-27  6:50 ` [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV " ChunYuZhiQiang
@ 2026-10-27  8:55 ` Vincent Mailhol
  2026-10-27 21:27 ` Jakub Kicinski
  2 siblings, 0 replies; 9+ messages in thread
From: Vincent Mailhol @ 2026-10-27  8:55 UTC (permalink / raw)
  To: ChunYuZhiQiang
  Cc: netdev-bot+sinfo, netdev, linux-can, mkl, kernel, haibo.chen,
	shawnguo, imx, stable

On Tue. 27 Oct. 2026 at 07:54, ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht> wrote:
> If the interface is down, the CAN controller might be powered down and
> its clocks gated. Don't call do_get_berr_counter() to read the bus error
> counters if the interface is down, and adjust can_get_size() to match.
>
> This removes one trigger for a synchronous external abort seen on
> i.MX8QXP when "ip link show" is run on a flexcan interface that was
> never brought up. The underlying cause is platform-specific: on i.MX8QXP
> the CAN clocks are gated through the SCU LPCG and the subsystem shares
> the IMX_SC_R_CAN_0 power domain, and there is a window after
> pm_runtime_resume_and_get() returns where the clock has not reached the
> module yet. This patch avoids the netlink query path while the interface
> is down, but does not root-cause the LPCG window itself.
>
> As a result, IFLA_CAN_BERR_COUNTER is no longer reported for down
> interfaces. Userspace must treat the attribute as optional.
>
> The issue was triggered on an i.MX8QXP board by running "ip link show"
> without ever bringing the interface up:
>
>     Internal error: synchronous external abort: 0000000096000210
>
>     pc : flexcan_read_le+0x0/0x18
>     lr : flexcan_get_berr_counter+0x4c/0x8c
>     Call trace:
>      flexcan_read_le+0x0/0x18
>      flexcan_get_berr_counter+0x4c/0x8c
>      can_fill_info+0x1f8/0x434
>      rtnl_fill_ifinfo+0x8fc/0xbb4
>      rtnl_dump_ifinfo+0x364/0x448
>
> This is the same class of problem fixed for m_can in
> commit 91a55c72a821 ("can: m_can: m_can_get_berr_counter(): don't
> wake up controller if interface is down").
>
> Fixes: ec56acfef2af1 ("can: flexcan: flexcan_get_berr_counter(): switch on clocks before accessing ecr register")
> Cc: stable@vger.kernel.org
> Signed-off-by: ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht>

Reviewed-by: Vincent Mailhol <mailhol@kernel.org>

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

* Re: [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down
  2026-10-27  6:50 ` [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV " ChunYuZhiQiang
@ 2026-10-27  8:57   ` Vincent Mailhol
  2026-10-27  9:19     ` Marc Kleine-Budde
  2026-10-27  9:20   ` Marc Kleine-Budde
  2026-10-28 20:02   ` [PATCH " netdev-bot+sashiko
  2 siblings, 1 reply; 9+ messages in thread
From: Vincent Mailhol @ 2026-10-27  8:57 UTC (permalink / raw)
  To: ChunYuZhiQiang
  Cc: netdev-bot+sinfo, netdev, linux-can, mkl, kernel, haibo.chen,
	shawnguo, imx, stable

On Tue. 27 Oct. 2026 at 07:54, ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht> wrote:
> The automatic TDCV readback calls into the driver via do_get_auto_tdcv().
> Some drivers, such as xcan_get_auto_tdcv(), read controller registers
> without taking a runtime PM reference, so calling this while the
> interface is down can touch registers whose clocks are gated.
>
> Gate do_get_auto_tdcv() on netif_running() in can_tdc_fill_info() and
> adjust can_tdc_get_size() and can_data_bittiming_get_size() so that only
> the automatic TDCV size is skipped when the interface is down. The other
> TDC parameters are still reported, as they are meaningful while the
> interface is down.
>
> This is a defensive hardening based on code inspection; the automatic
> TDCV path has not been triggered on hardware.
> Signed-off-by: ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht>

Reviewed-by: Vincent Mailhol <mailhol@kernel.org>

I think it is still preferable to add a fix tag here:

Fixes: e8060f08cd69 ("can: netlink: add can_priv::do_get_auto_tdcv()
to retrieve tdcv from device")

@Marc, not sure if b4 would automatically pick the Fixes tag. If not,
can you add it while applying?


Yours sincerely,
Vincent Mailhol

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

* Re: [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down
  2026-10-27  8:57   ` Vincent Mailhol
@ 2026-10-27  9:19     ` Marc Kleine-Budde
  0 siblings, 0 replies; 9+ messages in thread
From: Marc Kleine-Budde @ 2026-10-27  9:19 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: ChunYuZhiQiang, netdev-bot+sinfo, netdev, linux-can, kernel,
	haibo.chen, shawnguo, imx, stable

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

On 27.10.2026 09:57:03, Vincent Mailhol wrote:
> On Tue. 27 Oct. 2026 at 07:54, ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht> wrote:
> > The automatic TDCV readback calls into the driver via do_get_auto_tdcv().
> > Some drivers, such as xcan_get_auto_tdcv(), read controller registers
> > without taking a runtime PM reference, so calling this while the
> > interface is down can touch registers whose clocks are gated.
> >
> > Gate do_get_auto_tdcv() on netif_running() in can_tdc_fill_info() and
> > adjust can_tdc_get_size() and can_data_bittiming_get_size() so that only
> > the automatic TDCV size is skipped when the interface is down. The other
> > TDC parameters are still reported, as they are meaningful while the
> > interface is down.
> >
> > This is a defensive hardening based on code inspection; the automatic
> > TDCV path has not been triggered on hardware.
> > Signed-off-by: ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht>
>
> Reviewed-by: Vincent Mailhol <mailhol@kernel.org>
>
> I think it is still preferable to add a fix tag here:
>
> Fixes: e8060f08cd69 ("can: netlink: add can_priv::do_get_auto_tdcv()
> to retrieve tdcv from device")
>
> @Marc, not sure if b4 would automatically pick the Fixes tag. If not,
> can you add it while applying?

b4 git, but struggled with the new line :) - fixed

regards,
Marc

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

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

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

* Re: [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down
  2026-10-27  6:50 ` [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV " ChunYuZhiQiang
  2026-10-27  8:57   ` Vincent Mailhol
@ 2026-10-27  9:20   ` Marc Kleine-Budde
  2026-10-27  9:50     ` RE:[PATCH " 淳于智强
  2026-10-28 20:02   ` [PATCH " netdev-bot+sashiko
  2 siblings, 1 reply; 9+ messages in thread
From: Marc Kleine-Budde @ 2026-10-27  9:20 UTC (permalink / raw)
  To: ChunYuZhiQiang
  Cc: netdev-bot+sinfo, netdev, linux-can, mailhol, kernel, haibo.chen,
	shawnguo, imx, stable

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

On 27.10.2026 14:50:12, ChunYuZhiQiang wrote:
> The automatic TDCV readback calls into the driver via do_get_auto_tdcv().
> Some drivers, such as xcan_get_auto_tdcv(), read controller registers
> without taking a runtime PM reference, so calling this while the
> interface is down can touch registers whose clocks are gated.
>
> Gate do_get_auto_tdcv() on netif_running() in can_tdc_fill_info() and
> adjust can_tdc_get_size() and can_data_bittiming_get_size() so that only
> the automatic TDCV size is skipped when the interface is down. The other
> TDC parameters are still reported, as they are meaningful while the
> interface is down.
>
> This is a defensive hardening based on code inspection; the automatic
> TDCV path has not been triggered on hardware.
>
> Signed-off-by: ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht>
> ---
> v8:
>  - Split out of the berr-counter patch so that the stable backport only
>    carries the hardware-tested change.
> ---
>  drivers/net/can/dev/netlink.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index b958c04d53db..613beee9e936 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -720,8 +720,9 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  	return 0;
>  }
>
> -static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
> -			       u32 tdc_flags)
> +static size_t can_tdc_get_size(const struct net_device *dev,
> +				 struct data_bittiming_params *dbt_params,
> +			     u32 tdc_flags)

fixed indention while applying

regards,
Marc

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

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

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

* RE:[PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down
  2026-10-27  9:20   ` Marc Kleine-Budde
@ 2026-10-27  9:50     ` 淳于智强
  0 siblings, 0 replies; 9+ messages in thread
From: 淳于智强 @ 2026-10-27  9:50 UTC (permalink / raw)
  To: Marc Kleine-Budde, Vincent Mailhol
  Cc: netdev-bot+sinfo, netdev, linux-can, mailhol, kernel, haibo.chen,
	shawnguo, imx, stable

Hi Marc, Vincent,

Thanks both for the review and for applying the series.

Vincent: thanks for the Reviewed-by and for catching the missing
Fixes: tag on the TDCV patch.

Marc: thanks for fixing the indentation and folding in the Fixes: tag
while applying.

I'll keep the b4/checkpatch lessons in mind for future submissions.

Thanks,
ChunYuZhiQiang

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

* Re: [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller if interface is down
  2026-10-27  6:50 [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller if interface is down ChunYuZhiQiang
  2026-10-27  6:50 ` [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV " ChunYuZhiQiang
  2026-10-27  8:55 ` [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller " Vincent Mailhol
@ 2026-10-27 21:27 ` Jakub Kicinski
  2 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2026-10-27 21:27 UTC (permalink / raw)
  To: ChunYuZhiQiang
  Cc: netdev-bot+sinfo, netdev, linux-can, mkl, mailhol, kernel,
	haibo.chen, shawnguo, imx, stable

On Tue, 27 Oct 2026 14:50:11 +0800 ChunYuZhiQiang wrote:
> If the interface is down, the CAN controller might be powered down and
> its clocks gated. Don't call do_get_berr_counter() to read the bus error
> counters if the interface is down, and adjust can_get_size() to match.

Please don't CC netdev@ on run-of-the-mill CAN patches.
Follow to MAINTAINERS / get_maintainers.pl

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

* Re: [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down
  2026-10-27  6:50 ` [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV " ChunYuZhiQiang
  2026-10-27  8:57   ` Vincent Mailhol
  2026-10-27  9:20   ` Marc Kleine-Budde
@ 2026-10-28 20:02   ` netdev-bot+sashiko
  2 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-10-28 20:02 UTC (permalink / raw)
  To: chunyuzhiqiang
  Cc: netdev-bot+sinfo, netdev, linux-can, mkl, mailhol, kernel,
	haibo.chen, shawnguo, imx, stable, kuba

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

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

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

- [Medium] The commit message calls this change "defensive hardening
  based on code inspection" and has no Fixes: tag.
- [Medium] netif_running() is used as a stand-in for "the controller is
  clocked", but in xilinx_can it can be true while the clocks are gated.
- [Low] The modified can_tdc_get_size() prototype in
  drivers/net/can/dev/netlink.c (lines 723-725) has continuation lines…

--

can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down

This commit makes can_tdc_fill_info() call do_get_auto_tdcv() only when
netif_running() is true. Drivers such as xilinx_can then no longer read
controller registers while the interface is down. can_tdc_get_size() and
can_data_bittiming_get_size() now take the net_device, so IFLA_CAN_TDCV
space is reserved only under the same condition.

> This is a defensive hardening based on code inspection; the automatic
> TDCV path has not been triggered on hardware.

[Severity: Medium]
Is "defensive hardening" an accurate description here? The path this patch
closes looks reachable by an unprivileged user on current mainline and
stable kernels.

xcan_probe() ends with pm_runtime_put(). The controller then stays
runtime-suspended, with bus_clk and can_clk disabled, until xcan_open()
takes a reference. xcan_get_auto_tdcv() reads a register without taking a
runtime PM reference:

drivers/net/can/xilinx_can.c:xcan_get_auto_tdcv() {
    ...
	*tdcv = FIELD_GET(XCAN_SR_TDCV_MASK, priv->read_reg(priv, XCAN_SR_OFFSET));
    ...
}

xcan_get_berr_counter(), by comparison, wraps its reads in
pm_runtime_get_sync()/pm_runtime_put().

TDC_AUTO also gets turned on while the interface is down.
can_dbt_changelink() accepts data bittiming only when the device is not
IFF_UP. It calls can_calc_tdco(), which sets TDC_AUTO whenever the data BRP
is 1 or 2. Take the usual setup sequence:

  ip link set canX type can bitrate ... dbitrate ... fd on
  ip -d link show canX

Before this patch, that sequence reaches the following path with the
clocks gated:

  rtnl_getlink()->can_fill_info()->can_tdc_fill_info()->xcan_get_auto_tdcv()

The callback was added in commit 1010a8fa96086d ("can: xilinx_can: add
Transmitter Delay Compensation (TDC) feature support"). Should this patch
carry a Fixes: tag for that commit, plus Cc: stable?

The companion patch "can: dev: can_fill_info(): don't wake up controller if
interface is down" fixes the same kind of access, and it has both tags.

The changelog note says:

> Split out of the berr-counter patch so that the stable backport only
> carries the hardware-tested change.

Without a Fixes: tag, the stable tooling cannot tell that kernels
containing 1010a8fa96086d also need this change.

> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index b958c04d53db6..613beee9e9362 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -720,8 +720,9 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  	return 0;
>  }
>  
> -static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
> -			       u32 tdc_flags)
> +static size_t can_tdc_get_size(const struct net_device *dev,
> +				 struct data_bittiming_params *dbt_params,
> +			     u32 tdc_flags)

[Severity: Low]
This isn't a bug, but the continuation lines no longer line up with the
open parenthesis. The dbt_params line is two columns to the right of it,
and the tdc_flags line is two columns to the left. checkpatch --strict will
report "Alignment should match open parenthesis".

can_data_bittiming_get_size(), later in the same patch, is aligned
correctly.

>  {
>  	bool tdc_manual = tdc_flags & CAN_CTRLMODE_TDC_MANUAL_MASK;
>  	size_t size;
> @@ -742,7 +743,8 @@ static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
>  	}
>  
>  	if (tdc_flags) {
> -		if (tdc_manual || dbt_params->do_get_auto_tdcv)
> +		if (tdc_manual ||
> +		    (dbt_params->do_get_auto_tdcv && netif_running(dev)))
>  			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
>  		size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO */
>  		if (dbt_params->tdc_const->tdcf_max)

[ ... ]

> @@ -910,7 +913,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev,
>  		if (tdc_manual) {
>  			tdcv = tdc->tdcv;
>  			err = 0;
> -		} else if (dbt_params->do_get_auto_tdcv) {
> +		} else if (dbt_params->do_get_auto_tdcv && netif_running(dev)) {
>  			err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
>  		}

[Severity: Medium]
Can netif_running() be true while the xilinx_can clocks are still gated?
If so, xcan_get_auto_tdcv() can still read XCAN_SR_OFFSET from unclocked
hardware.

xcan_suspend() stops and detaches a running interface, then calls
pm_runtime_force_suspend(). That runs xcan_runtime_suspend(), which
disables bus_clk and can_clk. __LINK_STATE_START stays set:

drivers/net/can/xilinx_can.c:xcan_suspend() {
    ...
	if (netif_running(ndev)) {
		netif_stop_queue(ndev);
		netif_device_detach(ndev);
		xcan_chip_stop(ndev);
	}

	return pm_runtime_force_suspend(dev);
}

If pm_runtime_force_resume() fails during resume, xcan_resume() returns
early. This can happen, for example, when clk_prepare_enable() fails in
xcan_runtime_resume():

drivers/net/can/xilinx_can.c:xcan_resume() {
    ...
	ret = pm_runtime_force_resume(dev);
	if (ret) {
		dev_err(dev, "pm_runtime_force_resume failed on resume\n");
		return ret;
	}
    ...
}

At that point the clocks are off, the netdev is still IFF_UP and detached,
and netif_running() still returns true. An unprivileged
"ip -d link show canX" then passes the new check in can_tdc_fill_info()
and reads XCAN_SR_OFFSET in xcan_get_auto_tdcv().

rcar_canfd looks similar. rcar_canfd_suspend() calls
rcar_canfd_global_deinit(), which disables clkp and asserts reset. If
rcar_canfd_global_init() then fails in rcar_canfd_resume(),
rcar_canfd_get_auto_tdcv() reads cfdsts from a block that is unclocked or
held in reset.

Would it be more robust to take a runtime PM reference in
xcan_get_auto_tdcv(), as xcan_get_berr_counter() does?

Another option is to gate on netif_running(dev) && netif_device_present(dev).
Both drivers call netif_device_attach() only after the hardware is back up.

With either approach, can_tdc_get_size() needs the same condition so the
size estimate stays consistent with can_tdc_fill_info().

>  		if (!err && nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdcv))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027065012.191348-1-chunyuzhiqiang%40yinhe.ht

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

end of thread, other threads:[~2026-10-28 20:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-27  6:50 [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller if interface is down ChunYuZhiQiang
2026-10-27  6:50 ` [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV " ChunYuZhiQiang
2026-10-27  8:57   ` Vincent Mailhol
2026-10-27  9:19     ` Marc Kleine-Budde
2026-10-27  9:20   ` Marc Kleine-Budde
2026-10-27  9:50     ` RE:[PATCH " 淳于智强
2026-10-28 20:02   ` [PATCH " netdev-bot+sashiko
2026-10-27  8:55 ` [PATCH v8 1/2] can: dev: can_fill_info(): don't wake up controller " Vincent Mailhol
2026-10-27 21:27 ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.