[PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[ChunYuZhiQiang]

If the interface is down, the CAN controller might be powered down, the
clock disabled, and/or its external reset asserted.

Don't call do_get_berr_counter() or do_get_auto_tdcv() to read the CAN
controller if the interface is down. This fixes a generic logic problem
that exists on all platforms.

As a result, IFLA_CAN_BERR_COUNTER is no longer reported for down
interfaces, and the automatic TDCV readback is skipped. The other TDC
parameters are still reported, as they are meaningful while the
interface is down. This is intentional: the counters and the automatic
TDCV are not meaningful in that state, and calling into the driver can
wake a runtime-suspended controller or touch registers whose clocks are
gated.

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>
---
v6:
 - Only gate do_get_auto_tdcv() on netif_running(), as suggested by
   Vincent Mailhol and Marc Kleine-Budde. The other TDC parameters are
   still reported for down interfaces.
 - Adjust can_tdc_get_size() so that only the automatic TDCV size is
   skipped when the interface is down.
 - Keep the berr-counter gating unchanged.
---
 drivers/net/can/dev/netlink.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 6f83b87d54fc..ae0cef791ef3 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -517,7 +517,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 }
 
 static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
-			       u32 tdc_flags)
+			       u32 tdc_flags, bool auto_tdcv_running)
 {
 	bool tdc_manual = tdc_flags & CAN_CTRLMODE_TDC_MANUAL_MASK;
 	size_t size;
@@ -538,7 +538,7 @@ 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 && auto_tdcv_running))
 			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)
@@ -548,8 +548,9 @@ 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,
-					  u32 tdc_flags)
+static size_t
+can_data_bittiming_get_size(struct data_bittiming_params *dbt_params,
+			    u32 tdc_flags, bool auto_tdcv_running)
 {
 	size_t size = 0;
 
@@ -560,7 +561,8 @@ static size_t can_data_bittiming_get_size(struct data_bittiming_params *dbt_para
 	if (dbt_params->data_bitrate_const)		/* IFLA_CAN_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_TDC */
+
+	size += can_tdc_get_size(dbt_params, tdc_flags, auto_tdcv_running); /* IFLA_CAN_TDC */
 
 	return size;
 }
@@ -584,7 +586,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 */
@@ -598,7 +600,8 @@ static size_t can_get_size(const struct net_device *dev)
 	size += can_ctrlmode_ext_get_size();			/* IFLA_CAN_CTRLMODE_EXT */
 
 	size += can_data_bittiming_get_size(&priv->fd,
-					    priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK);
+					    priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK,
+					    netif_running(dev));
 
 	return size;
 }
@@ -675,7 +678,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))
@@ -735,7 +738,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

Re: [PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

RE：[PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[淳于智强]

Hi,

> - How the issue was discovered, e.g. hit in production, hit during
>   development, syzbot report, manual code inspection, LLM or static
>   analysis tool scan.
>
> - Whether the issue was actually triggered, or is only theoretical
>   (e.g. found by code inspection). If it was triggered please include
>   the symptoms, like the stack trace or error messages.

The berr-counter issue was actually triggered on an i.MX8QXP board
during development. It reproduces by simply running "ip link show"
without ever bringing the CAN interface up:

    Internal error: synchronous external abort: 0000000096000210 [#1] PREEMPT SMP
    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

The root 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. flexcan_runtime_resume() only calls flexcan_clks_enable();
there is a window where pm_runtime_resume_and_get() has returned but the
clock has not actually reached the module yet, and the immediate ECR
read in that window hits an un-clocked target and raises the external
abort. This is the same class of problem that was already fixed for
m_can in commit 91a55c72a821d ("can: m_can: m_can_get_berr_counter():
don't wake up controller if interface is down").

The TDC auto readback part was pointed out by the Sashiko AI review of
this series. It is based on code inspection of can_tdc_fill_info() and
xcan_get_auto_tdcv(), which reads XCAN_SR without taking a runtime PM
reference. I have not triggered that path on hardware, so no stack trace
is available for it; the patch gates it as a precaution.

Tested on i.MX8QXP: after the patch, "ip link show" no longer triggers
the abort.

Thanks,
ChunYuZhiQiang

Re: [PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[Marc Kleine-Budde]

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

On 21.10.2026 10:33:34, ChunYuZhiQiang wrote:
> If the interface is down, the CAN controller might be powered down, the
> clock disabled, and/or its external reset asserted.
>
> Don't call do_get_berr_counter() or do_get_auto_tdcv() to read the CAN
> controller if the interface is down. This fixes a generic logic problem
> that exists on all platforms.
>
> As a result, IFLA_CAN_BERR_COUNTER is no longer reported for down
> interfaces, and the automatic TDCV readback is skipped. The other TDC
> parameters are still reported, as they are meaningful while the
> interface is down. This is intentional: the counters and the automatic
> TDCV are not meaningful in that state, and calling into the driver can
> wake a runtime-suspended controller or touch registers whose clocks are
> gated.
>
> 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>

This patch doesn't apply to current net-next/main branch. Please rebase,
in particular you're missing commit e63281614747 ("can: netlink: add
initial CAN XL support").

regards,
Marc

pw-bot: cr

-- 
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 --]

RE：[PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[淳于智强]

Hi Marc,

> This patch doesn't apply to current net-next/main branch. Please rebase,
> in particular you're missing commit e63281614747 ("can: netlink: add
> initial CAN XL support").

Understood, v6 was based on an NXP lf-6.18.y tree. I'll rebase onto
current net-next/main, including e63281614747, and rework the patch so
that both IFLA_CAN_TDC and IFLA_CAN_XL_TDC only skip the automatic TDCV
readback when the interface is down. The size calculation will be
adjusted for both paths as well.

I'll send v7 as a new, independent thread.

Thanks,
ChunYuZhiQiang

Re: RE：[PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[Marc Kleine-Budde]

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

On 21.10.2026 16:20:18, 淳于智强 wrote:
> > This patch doesn't apply to current net-next/main branch. Please rebase,
> > in particular you're missing commit e63281614747 ("can: netlink: add
> > initial CAN XL support").
>
> Understood, v6 was based on an NXP lf-6.18.y tree.

:)

> I'll rebase onto
> current net-next/main, including e63281614747, and rework the patch so
> that both IFLA_CAN_TDC and IFLA_CAN_XL_TDC only skip the automatic TDCV
> readback when the interface is down. The size calculation will be
> adjusted for both paths as well.
>
> I'll send v7 as a new, independent thread.

Thanks, As you're contributing regularly, using b4 [1] can safe you and the
maintainers a lot of time.

regards,
Marc

[1] https://b4.docs.kernel.org/en/latest/index.html

-- 
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 --]

RE：[PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[淳于智强]

Hi Marc,

> Thanks, As you're contributing regularly, using b4 [1] can safe you and the
> maintainers a lot of time.
>
> [1] https://b4.docs.kernel.org/en/latest/index.html

Thanks for the pointer. I'll start using b4 for the v7 respin and future
series.

ChunYuZhiQiang

Re: [PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[Vincent Mailhol]

On 21/10/2026 at 04:33, ChunYuZhiQiang wrote:
> If the interface is down, the CAN controller might be powered down, the
> clock disabled, and/or its external reset asserted.
> 
> Don't call do_get_berr_counter() or do_get_auto_tdcv() to read the CAN
> controller if the interface is down. This fixes a generic logic problem
> that exists on all platforms.
> 
> As a result, IFLA_CAN_BERR_COUNTER is no longer reported for down
> interfaces, and the automatic TDCV readback is skipped. The other TDC
> parameters are still reported, as they are meaningful while the
> interface is down. This is intentional: the counters and the automatic
> TDCV are not meaningful in that state, and calling into the driver can
> wake a runtime-suspended controller or touch registers whose clocks are
> gated.
> 
> 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>
> ---
> v6:
>  - Only gate do_get_auto_tdcv() on netif_running(), as suggested by
>    Vincent Mailhol and Marc Kleine-Budde. The other TDC parameters are
>    still reported for down interfaces.
>  - Adjust can_tdc_get_size() so that only the automatic TDCV size is
>    skipped when the interface is down.
>  - Keep the berr-counter gating unchanged.
> ---
>  drivers/net/can/dev/netlink.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 6f83b87d54fc..ae0cef791ef3 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -517,7 +517,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  }
>  
>  static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
> -			       u32 tdc_flags)
> +			       u32 tdc_flags, bool auto_tdcv_running)

Nitpick: I think it is slightly better to pass dev rather than
auto_tdcv_running:

	static size_t can_tdc_get_size(const struct net_device *dev,
				       struct data_bittiming_params *dbt_params,
				       u32 tdc_flags)

The reason is that by passing dev, it would be easier in the future if
for whatever reason we need to retrieve other values. (And to be
honest, I was not going to comment just for this, but because a v7 is
needed anyway, I think you can add this small change while at it).

>  {
>  	bool tdc_manual = tdc_flags & CAN_CTRLMODE_TDC_MANUAL_MASK;
>  	size_t size;
> @@ -538,7 +538,7 @@ 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 && auto_tdcv_running))

And by passing dev, this becomes:

		if (tdc_manual || (dbt_params->do_get_auto_tdcv && netif_running(dev)))

making the pattern slightly more consistent throughout the file.

>  			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)
> @@ -548,8 +548,9 @@ static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
>  	return size;
>  }


Yours sincerely,
Vincent Mailhol

RE：[PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[淳于智强]

Hi Vincent,

> Nitpick: I think it is slightly better to pass dev rather than
> auto_tdcv_running:
>
>  static size_t can_tdc_get_size(const struct net_device *dev,
>            struct data_bittiming_params *dbt_params,
>            u32 tdc_flags)
>
> The reason is that by passing dev, it would be easier in the future if
> for whatever reason we need to retrieve other values.

Agreed, that's cleaner and more consistent with the rest of the file.
I'll fold this into v7: can_tdc_get_size() and
can_data_bittiming_get_size() will take dev instead of a bool, and the
TDCV size condition will use netif_running(dev) directly.

Thanks,
ChunYuZhiQiang

RE：[PATCH v6] can: dev: can_fill_info(): don't wake up controller if interface is down

[淳于智强]

Hi Vincent,

> Nitpick: I think it is slightly better to pass dev rather than
> auto_tdcv_running:
> ...

v7 is out and folds in your suggestion: can_tdc_get_size() and
can_data_bittiming_get_size() now take dev, and the TDCV size condition
uses netif_running(dev) directly.

Link: https://lore.kernel.org/linux-can/20261021145115.153107-1-chunyuzhiqiang@yinhe.ht/

Sorry for not Cc'ing you on the v7 posting.

Thanks,
ChunYuZhiQiang