On 04.11.2026 20:46:50, netdev-bot+sashiko@kernel.org wrote:
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
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 describes this as removing one trigger of a
flexcan abort on i.MX8QXP.
--
can: dev: can_fill_info(): don't wake up controller if interface is down
This patch changes can_fill_info() and can_get_size() to skip
IFLA_CAN_BERR_COUNTER when the interface is not running. As a result,
do_get_berr_counter() is never called on a down interface. The goal is
to avoid a synchronous external abort in flexcan_get_berr_counter() on
i.MX8QXP when "ip link show" runs on an interface that was never brought
up.
Link: https://patch.msgid.link/20261027065012.191348-1-chunyuzhiqiang@yinhe.ht (local)
quoted
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
[ ... ]
quoted
@@ -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: Medium]
Is this meant to change the output for every CAN driver, and not only
flexcan?
This is change is intended. Reading the CAN error counters of a down
interface makes no sense, as they must be read from hardware. The CAN
error counters are set to 0 by the hardware (as required by the CAN
specification) when the interface is (re)started.
can_fill_info() is the only core caller of priv->do_get_berr_counter.
With this check, no driver that sets the callback will report
IFLA_CAN_BERR_COUNTER on a down interface. can_get_size() gets the same
change.
Several drivers turn on their hardware inside the callback so that this
query works while the interface is down:
Yes - this can be removed in later patches.
drivers/net/can/sun4i_can.c:sun4ican_get_berr_counter() {
...
err = clk_prepare_enable(priv->clk);
...
}
drivers/net/can/c_can/c_can_main.c:c_can_get_berr_counter() {
...
c_can_pm_runtime_get_sync(priv);
err = __c_can_get_berr_counter(dev, bec);
c_can_pm_runtime_put_sync(priv);
...
}
Other drivers do the same thing:
bxcan_get_berr_counter() calls clk_prepare_enable()
xcan_get_berr_counter() and ctucan_get_berr_counter() call
pm_runtime_get_sync()
rcar_can_get_berr_counter(), rkcanfd_get_berr_counter() and
flexcan_get_berr_counter() call pm_runtime_resume_and_get()
The commit in the Fixes: tag, ec56acfef2af1, states this intent
directly: "flexcan_get_berr_counter() may be called from userspace even
if the interface is down ... This patch switches on the clocks".
After this change, the wake-up code in those drivers only runs when the
device is already active, so it no longer does anything useful. Should
that code be removed?
ACK
Or would it be better to put the check in the
flexcan driver instead?
no - we want to fix this conceptually on the CAN netlink level.
The commit message cites m_can as the precedent. Commit 91a55c72a821
fixed m_can inside the driver:
drivers/net/can/m_can/m_can.c:m_can_get_berr_counter() {
...
/* Avoid waking up the controller if the interface is down */
if (!(dev->flags & IFF_UP))
return 0;
...
}
That version still sends the attribute, with zeroed counters. With this
patch, m_can sends no attribute at all for a down interface. Is that
change intended?
Because of the flexcan Fixes: tag and Cc: stable, this would be
backported to every stable tree since v3.16. After that, "ip -details
link show" would stop printing berr-counter for down interfaces on all
drivers, including ones where the query always worked.
The commit message does say that the attribute is no longer reported.
Could it also say that this applies to all CAN drivers, and that it
replaces the per-driver support for down interfaces?
If we need to re-spin this series/PR we'll update the commit message.
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 |