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