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

[PATCH net v4] 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() to read the CAN bus error counters if
the interface is down. This fixes a generic logic problem that exists
on all platforms.

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>
---
v1 -> v2:
  - Reword the commit message to drop the MDIS reasoning.
  - Add Fixes: tag.

v2 -> v3:
  - Use the title and commit message suggested by Bough Chen.
  - Drop the i.MX8QXP-specific crash dump and erratum explanation.
  - Drop the m_can reference.
  - Drop the Link: tag.

v3 -> v4:
  - Move the netif_running() check from the flexcan driver to
    can_fill_info() and can_get_size() in the CAN core, as suggested by
    Marc Kleine-Budde. This covers all CAN drivers, not just flexcan.

 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 6f83b87d54fc..d17f4e9cc144 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -584,7 +584,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 */
@@ -735,7 +735,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 net v4] can: dev: can_fill_info(): don't wake up controller if interface is down

[netdev-bot+sashiko]

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

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

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

- [High] The fix is incomplete.
- [Low] The patch changes netlink uAPI output for every CAN driver that
  implements do_get_berr_counter: IFLA_CAN_BERR_COUNTER is now left out…

--

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

This adds a netif_running() check to can_get_size() and can_fill_info().
When the interface is down, do_get_berr_counter() is no longer called and
IFLA_CAN_BERR_COUNTER is no longer reported. The check used to be in the
flexcan driver and is now in the CAN core, so it applies to all drivers.

> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 766d455950f55..b958c04d53db6 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c

[ ... ]

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

[Severity: High]
Is this fix complete? Later in the same expression, can_fill_info() calls
can_tdc_fill_info() for IFLA_CAN_TDC and IFLA_CAN_XL_TDC. That path can
still read controller registers while the interface is down:

drivers/net/can/dev/netlink.c:can_tdc_fill_info() {
    ...
	if (tdc_is_enabled) {
		...
		} else if (dbt_params->do_get_auto_tdcv) {
			err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
		}
    ...
}

Nothing on this path checks netif_running().

On Xilinx CAN-FD, xcan_get_auto_tdcv() reads XCAN_SR without taking a
runtime PM reference. By contrast, xcan_get_berr_counter() calls
pm_runtime_get_sync() before it reads anything:

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

After probe, or after xcan_close() calls pm_runtime_put(), the device is
runtime suspended. At that point xcan_runtime_suspend() has disabled bus_clk
and can_clk. xcan_close() does not clear ctrlmode, so TDC_AUTO stays set
after every ifdown.

TDC_AUTO can also be turned on while the interface is down. For example,
"ip link set canX type can bitrate ... dbitrate ... fd on" goes through
can_calc_tdco(), which enables TDC_AUTO when the driver supports it. After
that, the RTM_NEWLINK notification or an unprivileged "ip -d link show"
would end up in:

can_fill_info()
  can_tdc_fill_info()
    xcan_get_auto_tdcv()
      priv->read_reg(priv, XCAN_SR_OFFSET)

and the read happens with the register clock gated. Depending on the SoC,
this either returns garbage or causes a bus stall or external abort.
rcar_canfd_get_auto_tdcv() also does a live readl() of cfdsts from this
path, whatever the interface state.

The commit message says "This fixes a generic logic problem that exists on
all platforms". Given that, should the do_get_auto_tdcv() readback in
can_tdc_fill_info() also be gated on netif_running()? Alternatively, should
xcan_get_auto_tdcv() take a runtime PM reference?

[Severity: Low]
This also changes what userspace sees for every driver that implements
do_get_berr_counter(). Together with the matching change in can_get_size(),
IFLA_CAN_BERR_COUNTER is now left out of RTM_GETLINK and RTM_NEWLINK
messages whenever the interface is down. As a result, "ip -details link
show" no longer prints berr-counter for a down interface.

Before this patch the attribute was always present:

- flexcan, xilinx_can and c_can woke the device and reported the real
  counters.
- m_can_get_berr_counter() and mcp251xfd_get_berr_counter() returned 0 with
  zeroed counters when the interface was not up:

drivers/net/can/m_can/m_can.c:m_can_get_berr_counter() {
    ...
	if (!(dev->flags & IFF_UP))
		return 0;
    ...
}

The netlink path can no longer reach these driver-level IFF_UP checks.

The commit named in Fixes:, ec56acfef2af1, was added so that userspace could
call flexcan_get_berr_counter() even while the interface was down. This
patch reverses that behaviour rather than fixing a defect in it, and it is
tagged for stable.

Could the commit message say that IFLA_CAN_BERR_COUNTER is no longer
reported for down interfaces, and explain why that change is acceptable for
stable?

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

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

[Marc Kleine-Budde]

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

On 19.10.2026 10:24:35, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The fix is incomplete.
> - [Low] The patch changes netlink uAPI output for every CAN driver that
>   implements do_get_berr_counter: IFLA_CAN_BERR_COUNTER is now left out…
>
> --
>
> can: dev: can_fill_info(): don't wake up controller if interface is down
>
> This adds a netif_running() check to can_get_size() and can_fill_info().
> When the interface is down, do_get_berr_counter() is no longer called and
> IFLA_CAN_BERR_COUNTER is no longer reported. The check used to be in the
> flexcan driver and is now in the CAN core, so it applies to all drivers.
>
> > diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > index 766d455950f55..b958c04d53db6 100644
> > --- a/drivers/net/can/dev/netlink.c
> > +++ b/drivers/net/can/dev/netlink.c
>
> [ ... ]
>
> > @@ -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)) ||
>
> [Severity: High]
> Is this fix complete? Later in the same expression, can_fill_info() calls
> can_tdc_fill_info() for IFLA_CAN_TDC and IFLA_CAN_XL_TDC. That path can
> still read controller registers while the interface is down:
>
> drivers/net/can/dev/netlink.c:can_tdc_fill_info() {
>     ...
> 	if (tdc_is_enabled) {
> 		...
> 		} else if (dbt_params->do_get_auto_tdcv) {
> 			err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
> 		}
>     ...
> }

Can you add the same for both IFLA_CAN_TDC and IFLA_CAN_XL_TDC.

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

回复：[PATCH net v4] can: dev: can_fill_info(): don't wake up controller if interface is down

[淳于智强]

Hi Marc,

> Can you add the same for both IFLA_CAN_TDC and IFLA_CAN_XL_TDC.
>
> Marc

Yes, will do. v2 gates can_tdc_fill_info() on netif_running() for both
IFLA_CAN_TDC and IFLA_CAN_XL_TDC, and keeps can_get_size() consistent.

> [Severity: Low]
> This also changes what userspace sees for every driver that implements
> do_get_berr_counter(). Together with the matching change in can_get_size(),
> IFLA_CAN_BERR_COUNTER is now left out of RTM_GETLINK and RTM_NEWLINK
> messages whenever the interface is down. As a result, "ip -details link
> show" no longer prints berr-counter for a down interface.
>
> Before this patch the attribute was always present:
>
> - flexcan, xilinx_can and c_can woke the device and reported the real
>   counters.
> - m_can_get_berr_counter() and mcp251xfd_get_berr_counter() returned 0 with
>   zeroed counters when the interface was not up:
>
> The netlink path can no longer reach these driver-level IFF_UP checks.
>
> The commit named in Fixes:, ec56acfef2af1, was added so that userspace could
> call flexcan_get_berr_counter() even while the interface was down. This
> patch reverses that behaviour rather than fixing a defect in it, and it is
> tagged for stable.
>
> Could the commit message say that IFLA_CAN_BERR_COUNTER is no longer
> reported for down interfaces, and explain why that change is acceptable for
> stable?

This is a real and intentional userspace-visible change, and I will make it
explicit in the commit message.

The reason for omitting IFLA_CAN_BERR_COUNTER while the interface is down is
that the counters are not meaningful in that state, and calling into the
driver can wake a runtime-suspended controller or touch registers whose
clocks are gated. The same concern applies to the TDC auto readback path,
which is why v2 also gates IFLA_CAN_TDC and IFLA_CAN_XL_TDC on
netif_running().

I agree that this supersedes the behaviour introduced by ec56acfef2af1
rather than fixing a defect in it. v2 will describe it that way: down
interfaces no longer report berr-counter, userspace must treat the attribute
as optional, and the change is acceptable for stable because it avoids
hardware access in the down state. I will also reword the Fixes: tag so it
does not imply that ec56acfef2af1 itself was buggy.

pw-bot: cr

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

[淳于智强]

Hi Marc,

> Can you add the same for both IFLA_CAN_TDC and IFLA_CAN_XL_TDC.
>
> Marc

Yes, will do. v2 gates can_tdc_fill_info() on netif_running() for both
IFLA_CAN_TDC and IFLA_CAN_XL_TDC, and keeps can_get_size() consistent.

> [Severity: Low]
> This also changes what userspace sees for every driver that implements
> do_get_berr_counter(). Together with the matching change in can_get_size(),
> IFLA_CAN_BERR_COUNTER is now left out of RTM_GETLINK and RTM_NEWLINK
> messages whenever the interface is down. As a result, "ip -details link
> show" no longer prints berr-counter for a down interface.
>
> Before this patch the attribute was always present:
>
> - flexcan, xilinx_can and c_can woke the device and reported the real
>   counters.
> - m_can_get_berr_counter() and mcp251xfd_get_berr_counter() returned 0 with
>   zeroed counters when the interface was not up:
>
> The netlink path can no longer reach these driver-level IFF_UP checks.
>
> The commit named in Fixes:, ec56acfef2af1, was added so that userspace could
> call flexcan_get_berr_counter() even while the interface was down. This
> patch reverses that behaviour rather than fixing a defect in it, and it is
> tagged for stable.
>
> Could the commit message say that IFLA_CAN_BERR_COUNTER is no longer
> reported for down interfaces, and explain why that change is acceptable for
> stable?

This is a real and intentional userspace-visible change, and I will make it
explicit in the commit message.

The reason for omitting IFLA_CAN_BERR_COUNTER while the interface is down is
that the counters are not meaningful in that state, and calling into the
driver can wake a runtime-suspended controller or touch registers whose
clocks are gated. The same concern applies to the TDC auto readback path,
which is why v2 also gates IFLA_CAN_TDC and IFLA_CAN_XL_TDC on
netif_running().

I agree that this supersedes the behaviour introduced by ec56acfef2af1
rather than fixing a defect in it. v2 will describe it that way: down
interfaces no longer report berr-counter, userspace must treat the attribute
as optional, and the change is acceptable for stable because it avoids
hardware access in the down state. I will also reword the Fixes: tag so it
does not imply that ec56acfef2af1 itself was buggy.

pw-bot: cr



------------------------------------------------------------------
发件人：Marc Kleine-Budde <mkl@pengutronix.de>
发送时间：2026年10月19日(周一) 19:14
收件人："netdev-bot+sashiko"<netdev-bot+sashiko@kernel.org>
抄　送："淳于智强"<chunyuzhiqiang@yinhe.ht>; netdev<netdev@vger.kernel.org>; "linux-can"<linux-can@vger.kernel.org>; kernel<kernel@pengutronix.de>; "haibo.chen"<haibo.chen@oss.nxp.com>; shawnguo<shawnguo@kernel.org>; imx<imx@lists.linux.dev>; kuba<kuba@kernel.org>
主　题：Re: [PATCH net v4] can: dev: can_fill_info(): don't wake up controller if interface is down


On 19.10.2026 10:24:35, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The fix is incomplete.
> - [Low] The patch changes netlink uAPI output for every CAN driver that
>   implements do_get_berr_counter: IFLA_CAN_BERR_COUNTER is now left out…
>
> --
>
> can: dev: can_fill_info(): don't wake up controller if interface is down
>
> This adds a netif_running() check to can_get_size() and can_fill_info().
> When the interface is down, do_get_berr_counter() is no longer called and
> IFLA_CAN_BERR_COUNTER is no longer reported. The check used to be in the
> flexcan driver and is now in the CAN core, so it applies to all drivers.
>
> > diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > index 766d455950f55..b958c04d53db6 100644
> > --- a/drivers/net/can/dev/netlink.c
> > +++ b/drivers/net/can/dev/netlink.c
>
> [ ... ]
>
> > @@ -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)) ||
>
> [Severity: High]
> Is this fix complete? Later in the same expression, can_fill_info() calls
> can_tdc_fill_info() for IFLA_CAN_TDC and IFLA_CAN_XL_TDC. That path can
> still read controller registers while the interface is down:
>
> drivers/net/can/dev/netlink.c:can_tdc_fill_info() {
>     ...
>  if (tdc_is_enabled) {
>   ...
>   } else if (dbt_params->do_get_auto_tdcv) {
>    err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
>   }
>     ...
> }

Can you add the same for both IFLA_CAN_TDC and IFLA_CAN_XL_TDC.

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   |

[PATCH v5] 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 and IFLA_CAN_TDC are no longer
reported for down interfaces. This is intentional: the counters 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>
---
v5:
 - Gate can_tdc_fill_info() on netif_running() for IFLA_CAN_TDC, as
   requested by Marc Kleine-Budde. This branch has no IFLA_CAN_XL_TDC
   definition or call path at all.
 - Keep the TDC part of can_get_size() consistent with can_fill_info(),
   without hiding the non-TDC data bittiming attributes.
 - Document that IFLA_CAN_BERR_COUNTER is no longer reported for down
   interfaces, and explain why this is acceptable for stable.
 - Clarify the Fixes: rationale.
---
 drivers/net/can/dev/netlink.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 6f83b87d54fc..66b37257b76b 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -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 tdc_if_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 */
+	if (tdc_if_running)
+		size += can_tdc_get_size(dbt_params, tdc_flags);/* 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;
 }
@@ -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)) ||
 
@@ -764,7 +767,7 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		     sizeof(priv->bitrate_max),
 		     &priv->bitrate_max)) ||
 
-	    can_tdc_fill_info(skb, dev, IFLA_CAN_TDC) ||
+	    (netif_running(dev) && can_tdc_fill_info(skb, dev, IFLA_CAN_TDC)) ||
 
 	    can_ctrlmode_ext_fill_info(skb, priv)
 	    )
-- 
2.47.3

Re: [PATCH v5] 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 v5] can: dev: can_fill_info(): don't wake up controller if interface is down

[Marc Kleine-Budde]

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

On 20.10.2026 15:53:40, 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 and IFLA_CAN_TDC are no longer
> reported for down interfaces. This is intentional: the counters 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>
> ---
> v5:
>  - Gate can_tdc_fill_info() on netif_running() for IFLA_CAN_TDC, as
>    requested by Marc Kleine-Budde. This branch has no IFLA_CAN_XL_TDC
>    definition or call path at all.
>  - Keep the TDC part of can_get_size() consistent with can_fill_info(),
>    without hiding the non-TDC data bittiming attributes.
>  - Document that IFLA_CAN_BERR_COUNTER is no longer reported for down
>    interfaces, and explain why this is acceptable for stable.
>  - Clarify the Fixes: rationale.

Thanks for the patch. In the future, please don't send vN+1 as a reply
to vN.

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

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

[淳于智强]

Hi Marc,> Thanks for the patch. In the future, please don't send vN+1 as a reply
> to vN.
Understood, thanks for the reminder. I'll post future versions as a new,
independent thread.
Thanks,
ChunYuZhiQiang

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

[Vincent Mailhol]

On Tue. 20 Oct. 2026 at 10:02, ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht> 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 and IFLA_CAN_TDC are no longer
> reported for down interfaces. This is intentional: the counters are not
> meaningful in that state, and calling into the driver can wake a
> runtime-suspended controller or touch registers whose clocks are gated.

The CAN TDC parameters are still meaningful when the interface is down.

What about this:

---8<---
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 766d455950f55..1bc160b78b0e1 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -910,7 +910,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))
---8<---

?

All the other TDC parameters should still be available when the
interface is down, so just gating the auto tdcv is enough I think.

> 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>
> ---
> v5:
>  - Gate can_tdc_fill_info() on netif_running() for IFLA_CAN_TDC, as
>    requested by Marc Kleine-Budde. This branch has no IFLA_CAN_XL_TDC
>    definition or call path at all.
>  - Keep the TDC part of can_get_size() consistent with can_fill_info(),
>    without hiding the non-TDC data bittiming attributes.
>  - Document that IFLA_CAN_BERR_COUNTER is no longer reported for down
>    interfaces, and explain why this is acceptable for stable.
>  - Clarify the Fixes: rationale.
> ---
>  drivers/net/can/dev/netlink.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 6f83b87d54fc..66b37257b76b 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -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 tdc_if_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 */
> +       if (tdc_if_running)
> +               size += can_tdc_get_size(dbt_params, tdc_flags);/* 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;
>  }
> @@ -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)) ||
>
> @@ -764,7 +767,7 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
>                      sizeof(priv->bitrate_max),
>                      &priv->bitrate_max)) ||
>
> -           can_tdc_fill_info(skb, dev, IFLA_CAN_TDC) ||
> +           (netif_running(dev) && can_tdc_fill_info(skb, dev, IFLA_CAN_TDC)) ||
>
>             can_ctrlmode_ext_fill_info(skb, priv)
>             )

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

[Marc Kleine-Budde]

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

Hey Vincent,

Looks like you've set up your email/attention filter pretty well :D
Thanks for reviewing this TDC patch.

On 20.10.2026 11:41:49, Vincent Mailhol wrote:
> On Tue. 20 Oct. 2026 at 10:02, ChunYuZhiQiang <chunyuzhiqiang@yinhe.ht> 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 and IFLA_CAN_TDC are no longer
> > reported for down interfaces. This is intentional: the counters are not
> > meaningful in that state, and calling into the driver can wake a
> > runtime-suspended controller or touch registers whose clocks are gated.
>
> The CAN TDC parameters are still meaningful when the interface is down.

ACK

> What about this:
>
> ---8<---
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 766d455950f55..1bc160b78b0e1 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -910,7 +910,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))
> ---8<---
>
> ?
>
> All the other TDC parameters should still be available when the
> interface is down, so just gating the auto tdcv is enough I think.

Yes, the call to dbt_params->do_get_auto_tdcv() should be gated. Also
check if the size calculation has to be adjusted.

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