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

[PATCH v7] 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 for both CAN FD
and CAN XL. The other TDC parameters are still reported, as they are
meaningful while the interface is down. Userspace must treat these
attributes as optional.

The berr-counter issue was triggered on an i.MX8QXP board by running
"ip link show" without ever bringing the interface up. On i.MX8QXP the
CAN clocks are gated through the SCU LPCG and the subsystem shares the
IMX_SC_R_CAN_0 power domain; there is a window where
pm_runtime_resume_and_get() has returned but the clock has not reached
the module yet, and the immediate ECR read raises a synchronous
external abort. 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").

The TDC auto readback path was found by code inspection; it is gated as
a precaution because xcan_get_auto_tdcv() reads XCAN_SR without taking a
runtime PM reference.

Note: do_get_state() is left unguarded. Drivers that implement it by
reading live status registers may need a similar fix. This is tracked
separately and is not changed by this patch.

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>
---
v7:
 - Rebase onto current net-next/main, including e63281614747
   ("can: netlink: add initial CAN XL support").
 - Gate do_get_auto_tdcv() on netif_running() for both IFLA_CAN_TDC and
   IFLA_CAN_XL_TDC. The other TDC parameters are still reported for down
   interfaces.
 - Pass dev to can_tdc_get_size() and can_data_bittiming_get_size()
   instead of a bool, as suggested by Vincent Mailhol.
 - Adjust the TDC size calculation for both FD and XL paths.
 - Keep the berr-counter gating unchanged.
 - The Fixes: tag points to ec56acfef2af1 because this patch supersedes
   the down-interface readback behavior it introduced, not because that
   commit was buggy.
 - TDC auto readback gating is based on code inspection; only the
   berr-counter path was tested on i.MX8QXP hardware.
---
 drivers/net/can/dev/netlink.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 766d455950f5..ac30d2849543 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -720,7 +720,8 @@ 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,
+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;
@@ -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,8 +754,10 @@ 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(const struct net_device *dev,
+			    struct data_bittiming_params *dbt_params,
+			    u32 tdc_flags)
 {
 	size_t size = 0;
 
@@ -764,7 +768,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;
 }
@@ -812,7 +816,8 @@ 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 */
@@ -825,10 +830,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 +915,8 @@ 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))
@@ -1006,7 +1012,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 v7] 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: 0 · Medium: 1 · Low: 1 (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 the issue "a generic logic problem
  that exists on all platforms", but its own root-cause analysis points…
- [Low] The patch carries `Fixes: ec56acfef2af1` (2014) and `Cc:
  stable@vger.kernel.org`, but it is explicitly rebased onto net-next…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but can_get_size()
  reserves only `sizeof(priv->bitrate_max)` (4 bytes) for…

--

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

When the interface is not running, this patch skips the calls to
do_get_berr_counter() and do_get_auto_tdcv() in the CAN netlink fill path,
and changes can_get_size() to match. As a result, IFLA_CAN_BERR_COUNTER and
IFLA_CAN_TDC_TDCV are no longer reported for down interfaces. The
motivation is a synchronous external abort seen on i.MX8QXP when "ip link
show" is run on a flexcan interface that was never brought up.

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

[ ... ]

> On i.MX8QXP the
> CAN clocks are gated through the SCU LPCG and the subsystem shares the
> IMX_SC_R_CAN_0 power domain; there is a window where
> pm_runtime_resume_and_get() has returned but the clock has not reached
> the module yet, and the immediate ECR read raises a synchronous
> external abort.

[ ... ]

> Fixes: ec56acfef2af1 ("can: flexcan: flexcan_get_berr_counter(): switch on clocks before accessing ecr register")
> Cc: stable@vger.kernel.org

[Severity: Medium]
Is "a generic logic problem that exists on all platforms" an accurate
description of what is going on here?

The flexcan netlink path already takes a runtime PM reference before it
reads ECR:

drivers/net/can/flexcan/flexcan-core.c:flexcan_get_berr_counter() {
    ...
	err = pm_runtime_resume_and_get(priv->dev);
	if (err < 0)
		return err;

	err = __flexcan_get_berr_counter(dev, bec);
    ...
}

flexcan_runtime_resume() only calls flexcan_clks_enable(priv).

Suppose pm_runtime_resume_and_get() really can return before the LPCG clock
reaches the module. That looks like an i.MX8QXP clock or power domain
problem. The SCU LPCG driver already has an erratum workaround on that
enable path, lpcg_e10858_writel() in drivers/clk/imx/clk-lpcg-scu.c.

This patch removes one trigger, a netlink query while the interface is
down. The same enable-then-MMIO sequence seems to remain elsewhere:

ip link set canX up
  flexcan_open()
    pm_runtime_resume_and_get()
    ...
    flexcan_chip_start()

register_flexcandev()
  flexcan_clks_enable()
  flexcan_chip_disable()

If that window exists, would these paths hit the same abort?
flexcan_open() does other work between the resume and the first register
access, so it may not show up there. register_flexcandev(), however,
touches registers right after it enables the clocks.

The v7 notes also say the Fixes: commit ec56acfef2af1 was not buggy. With
Cc: stable, could this lead backporters to think the i.MX8QXP synchronous
external abort has been root-caused and fixed?

Not waking the controller while the interface is down looks reasonable,
and it matches the m_can fix. The concern is only with the root cause and
the Fixes/stable framing.

>  - Rebase onto current net-next/main, including e63281614747
>    ("can: netlink: add initial CAN XL support").

[ ... ]

>  - TDC auto readback gating is based on code inspection; only the
>    berr-counter path was tested on i.MX8QXP hardware.

[Severity: Low]
The Fixes: and Cc: stable tags mark this for backporting to stable.
However, the patch is based on net-next and depends on the CAN XL netlink
support from e63281614747.

The hunks that use priv->xl, CAN_CTRLMODE_XL_TDC_MASK, the dbt_params-based
can_tdc_get_size() and can_data_bittiming_get_size(), and
can_tdc_fill_info(..., IFLA_CAN_XL_TDC) don't look like they will apply to
older stable branches.

The same stable-tagged patch also combines the berr-counter change, which
was tested on hardware, with the do_get_auto_tdcv gating, which came from
code inspection only. Would it help stable reviewers if the TDCV gating
were a separate patch?

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

[ ... ]

> @@ -825,10 +830,10 @@ static size_t can_get_size(const struct net_device *dev)
>  	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */

[Severity: Low]
This isn't a bug introduced by this patch, but should this be
nla_total_size(sizeof(priv->bitrate_max))?

can_fill_info() emits the attribute with nla_put():

	    (nla_put(skb, IFLA_CAN_BITRATE_MAX,
		     sizeof(priv->bitrate_max),
		     &priv->bitrate_max)) ||

That needs nla_total_size(4) = 8 bytes of tailroom. can_get_size() reserves
only 4, so it comes up 4 bytes short for each CAN interface. Every other
attribute in can_get_size() is counted with nla_total_size().

In practice the slack in if_nlmsg_size() and slab rounding probably hide
the shortfall.

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

[ ... ]

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

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

[淳于智强]

Hi,

> [Severity: Medium]
> Is "a generic logic problem that exists on all platforms" an accurate
> description of what is going on here?

No, that phrase is inaccurate and I'll drop it in v8. The flexcan
berr-counter path already takes a runtime PM reference; the SEA on
i.MX8QXP is caused by the LPCG clock window after
pm_runtime_resume_and_get() returns, not by a missing wakeup. This patch
removes one trigger (netlink query while down), it is not a root-cause
fix for the LPCG window.

v8 will describe it as a defensive fix that avoids calling driver
callbacks while the interface is down, and point to the LPCG clock
window as the platform-specific root cause. The Fixes: tag stays
because this supersedes the down-interface readback behavior introduced
by ec56acfef2af1, but the changelog will make clear that the i.MX8QXP
SEA is not fully root-caused by this patch.

> [Severity: Low]
> The Fixes: and Cc: stable tags mark this for backporting to stable.
> However, the patch is based on net-next and depends on the CAN XL
> netlink support from e63281614747.
> ...
> Would it help stable reviewers if the TDCV gating were a separate
> patch?

Agreed. v8 splits this into two patches:

1. berr-counter gating, hardware-tested on i.MX8QXP, Cc: stable,
   no dependency on CAN XL;
2. TDCV gating, net-next only, depends on the CAN XL netlink support,
   based on code inspection.

The stable-tagged patch will only contain the berr-counter change.

> [Severity: Low]
> This isn't a bug introduced by this patch, but should this be
> nla_total_size(sizeof(priv->bitrate_max))?

This is pre-existing and not changed by this series. I'll leave it out
of this series to keep the scope focused.

pw-bot: cr

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

[Vincent Mailhol]

Hi Chun,

Because it seems you are preparing a v8, here are a few final nitpicks
on the line wrap.

On 21/10/2026 at 16:51, 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 for both CAN FD
> and CAN XL. The other TDC parameters are still reported, as they are
> meaningful while the interface is down. Userspace must treat these
> attributes as optional.
> 
> The berr-counter issue was triggered on an i.MX8QXP board by running
> "ip link show" without ever bringing the interface up. On i.MX8QXP the
> CAN clocks are gated through the SCU LPCG and the subsystem shares the
> IMX_SC_R_CAN_0 power domain; there is a window where
> pm_runtime_resume_and_get() has returned but the clock has not reached
> the module yet, and the immediate ECR read raises a synchronous
> external abort. 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").
> 
> The TDC auto readback path was found by code inspection; it is gated as
> a precaution because xcan_get_auto_tdcv() reads XCAN_SR without taking a
> runtime PM reference.
> 
> Note: do_get_state() is left unguarded. Drivers that implement it by
> reading live status registers may need a similar fix. This is tracked
> separately and is not changed by this patch.
> 
> 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>
> ---
> v7:
>  - Rebase onto current net-next/main, including e63281614747
>    ("can: netlink: add initial CAN XL support").
>  - Gate do_get_auto_tdcv() on netif_running() for both IFLA_CAN_TDC and
>    IFLA_CAN_XL_TDC. The other TDC parameters are still reported for down
>    interfaces.
>  - Pass dev to can_tdc_get_size() and can_data_bittiming_get_size()
>    instead of a bool, as suggested by Vincent Mailhol.
>  - Adjust the TDC size calculation for both FD and XL paths.
>  - Keep the berr-counter gating unchanged.
>  - The Fixes: tag points to ec56acfef2af1 because this patch supersedes
>    the down-interface readback behavior it introduced, not because that
>    commit was buggy.
>  - TDC auto readback gating is based on code inspection; only the
>    berr-counter path was tested on i.MX8QXP hardware.
> ---
>  drivers/net/can/dev/netlink.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 766d455950f5..ac30d2849543 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -720,7 +720,8 @@ 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,
> +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;
> @@ -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,8 +754,10 @@ 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(const struct net_device *dev,
> +			    struct data_bittiming_params *dbt_params,
> +			    u32 tdc_flags)
>  {
>  	size_t size = 0;
>  
> @@ -764,7 +768,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;
>  }
> @@ -812,7 +816,8 @@ 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 */

Put this on a single line:

	if (priv->do_get_berr_counter && netif_running(dev))	/* IFLA_CAN_BERR_COUNTER */

(it slightly exceeds the 80 column limit, but so does the surrounding
code).

>  		size += nla_total_size(sizeof(struct can_berr_counter));
>  	if (priv->termination_const) {
>  		size += nla_total_size(sizeof(priv->termination));		/* IFLA_CAN_TERMINATION */
> @@ -825,10 +830,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 +915,8 @@ 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)) {

Same, this fit in one line:

		} 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))
> @@ -1006,7 +1012,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)) ||
>  


Yours sincerely,
Vincent Mailhol

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

[淳于智强]

Hi Vincent,

> Put this on a single line:
>
>  if (priv->do_get_berr_counter && netif_running(dev)) /* IFLA_CAN_BERR_COUNTER */
>
> (it slightly exceeds the 80 column limit, but so does the surrounding
> code).
>
> Same, this fit in one line:
>
>   } else if (dbt_params->do_get_auto_tdcv && netif_running(dev)) {

Agreed, both will be on a single line in v8 as you suggested.

Thanks,
ChunYuZhiQiang

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

[淳于智强]

Hi Vincent,

> Put this on a single line:
>
>  if (priv->do_get_berr_counter && netif_running(dev)) /* IFLA_CAN_BERR_COUNTER */
>
> Same, this fit in one line:
>
>   } else if (dbt_params->do_get_auto_tdcv && netif_running(dev)) {

Thanks for the nitpicks. Unfortunately v8 went out before I saw this
mail:

https://lore.kernel.org/linux-can/20261027...

The two conditions are still split across two lines in v8. Would you
prefer that I send a v9 with these folded in, or is this something you
can adjust when applying?

I'm happy to respin if there are other changes needed anyway.

Thanks,
ChunYuZhiQiang

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

[淳于智强]

Hi Vincent,

Sorry, my previous reply was wrong on two counts.

1. v8 already has both conditions on a single line:

	if (priv->do_get_berr_counter && netif_running(dev))	/* IFLA_CAN_BERR_COUNTER */

   } else if (dbt_params->do_get_auto_tdcv && netif_running(dev)) {

   So no v9 is needed for these nitpicks.

2. The link in my previous mail was a placeholder. The actual v8
   series is here:

https://lore.kernel.org/linux-can/20261027065012.191348-1-chunyuzhiqiang@yinhe.ht/T/#t

Please ignore the "still split across two lines" part.

Thanks,
ChunYuZhiQiang