[PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[Mike Looijmans]

When there's an error attempting to store the BER counter, don't abort
but continue shutting down the chip as required.

After disabling communications, also stop the TX queue with a call to
netif_stop_queue. When the interface restarts, mcp251xfd_set_mode will
call netif_wake_queue and resume.

This fixes a hangup in either send() or poll() from userspace. To
reproduce: I ran "cansequence can0 -p" to flood the system with packets.
While running that, I shorted the CAN signals, causing a bus error.
Usually communications would resume after the CAN bus restarted, but
sometimes the system got stuck and refused to send any more packets.
The sending process would be in either poll() or send(), waiting for
the queue to resume. To "unstuck" the process from send() it was
sufficient to send any packet on the can interface from another
process. To get it out of the poll() hang, only bringing the can
interface down (and up) would work.

After adding the netif_stop_queue call, I was unable to reproduce the
problem.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 68df6d4641b5..854e0644764c 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -1084,10 +1084,11 @@ static int mcp251xfd_handle_cerrif(struct mcp251xfd_priv *priv)
 		 */
 		err = __mcp251xfd_get_berr_counter(priv->ndev, &priv->bec);
 		if (err)
-			return err;
+			netdev_err(priv->ndev, "Failed to store BER counter\n");
 
 		mcp251xfd_chip_stop(priv, CAN_STATE_BUS_OFF);
 		can_bus_off(priv->ndev);
+		netif_stop_queue(priv->ndev);
 	}
 
 	if (!skb)
-- 
2.17.1


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail

Re: [PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[Marc Kleine-Budde]

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

On 11.07.2023 17:26:47, Mike Looijmans wrote:
> When there's an error attempting to store the BER counter, don't abort
> but continue shutting down the chip as required.

If you refer to an error by __mcp251xfd_get_berr_counter(), it's not a
store error, but a failure of regmap_read(priv->map_reg,
MCP251XFD_REG_TREC, &trec). By default the SPI transfers are CRC enabled
and no/wrong data from the chip will be detected and return an error
here (after 3 retires). In out of memory conditions or other kernel
errors might be possible here, too.

Have you seen a problem here? But as we shut down the chip here anyways,
we can ignore the error here.

> After disabling communications, also stop the TX queue with a call to
> netif_stop_queue.

can_bus_off() will call netif_carrier_off(), isn't this sufficient? Have
you enabled automatic restart in case of bus off? I think the netdev
watchdog will kick you, if the interface has a stooped queue for too
long (IIRC 5 seconds).

> When the interface restarts, mcp251xfd_set_mode will
> call netif_wake_queue and resume.
> 
> This fixes a hangup in either send() or poll() from userspace. To
> reproduce: I ran "cansequence can0 -p" to flood the system with packets.
> While running that, I shorted the CAN signals, causing a bus error.
> Usually communications would resume after the CAN bus restarted, but
> sometimes the system got stuck and refused to send any more packets.
> The sending process would be in either poll() or send(), waiting for
> the queue to resume. To "unstuck" the process from send() it was
> sufficient to send any packet on the can interface from another
> process. To get it out of the poll() hang, only bringing the can
> interface down (and up) would work.
> 
> After adding the netif_stop_queue call, I was unable to reproduce the
> problem.

The newly added netif_stop_queue() will cause the netif_wake_queue() in
the mcp251xfd_set_mode() to actually wake the queue. If you observe a
problem, I think it's a general problem, so all drivers would be
effected.

> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

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: 488 bytes --]

Re: [PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[Mike Looijmans]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 11-07-2023 19:05, Marc Kleine-Budde wrote:
> On 11.07.2023 17:26:47, Mike Looijmans wrote:
>> When there's an error attempting to store the BER counter, don't abort
>> but continue shutting down the chip as required.
> 
> If you refer to an error by __mcp251xfd_get_berr_counter(), it's not a
> store error, but a failure of regmap_read(priv->map_reg,
> MCP251XFD_REG_TREC, &trec). By default the SPI transfers are CRC enabled
> and no/wrong data from the chip will be detected and return an error
> here (after 3 retires). In out of memory conditions or other kernel
> errors might be possible here, too.
> 
> Have you seen a problem here? But as we shut down the chip here anyways,
> we can ignore the error here.

Excactly my point, the important task here is to report the bus off state.

I haven't seen the error trigger, so it wasn't the cause of our problems.

In retrospect, maybe I should have split this in two patches?


>> After disabling communications, also stop the TX queue with a call to
>> netif_stop_queue.
> 
> can_bus_off() will call netif_carrier_off(), isn't this sufficient? Have
> you enabled automatic restart in case of bus off? I think the netdev
> watchdog will kick you, if the interface has a stooped queue for too
> long (IIRC 5 seconds).


Apparently not. The problem also occurred in the field, where the CAN 
communication got stuck for something like a full day.

Automatic restart was enabled, with a 100ms timeout. Setting a shorter timeout 
like 50 ms seemed to make the problem more likely to occur

What I also noticed, but didn't accurately measure, is that the time it took 
to recover used to be somewhat random, it could take up to about a second for 
packets to start going out again, as observed by watching LEDs. After adding 
netif_stop_queue the recovery was almost instantaneous.


> 
>> When the interface restarts, mcp251xfd_set_mode will
>> call netif_wake_queue and resume.
>>
>> This fixes a hangup in either send() or poll() from userspace. To
>> reproduce: I ran "cansequence can0 -p" to flood the system with packets.
>> While running that, I shorted the CAN signals, causing a bus error.
>> Usually communications would resume after the CAN bus restarted, but
>> sometimes the system got stuck and refused to send any more packets.
>> The sending process would be in either poll() or send(), waiting for
>> the queue to resume. To "unstuck" the process from send() it was
>> sufficient to send any packet on the can interface from another
>> process. To get it out of the poll() hang, only bringing the can
>> interface down (and up) would work.
>>
>> After adding the netif_stop_queue call, I was unable to reproduce the
>> problem.
> 
> The newly added netif_stop_queue() will cause the netif_wake_queue() in
> the mcp251xfd_set_mode() to actually wake the queue. If you observe a
> problem, I think it's a general problem, so all drivers would be
> effected.

After two days digging, my conclusion was that "something" wasn't getting a 
proper "kick" when the CAN interface came back online.


One could argue the netif_stop_queue should be put into can_bus_off. But the 
netif_wake_queue call is in the driver, so that would break symmetry.


> 
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> Marc
> 

[PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[Mike Looijmans]

When there's an error attempting to store the BER counter, don't abort
but continue shutting down the chip as required.

After disabling communications, also stop the TX queue with a call to
netif_stop_queue. When the interface restarts, mcp251xfd_set_mode will
call netif_wake_queue and resume.

This fixes a hangup in either send() or poll() from userspace. To
reproduce: I ran "cansequence can0 -p" to flood the system with packets.
While running that, I shorted the CAN signals, causing a bus error.
Usually communications would resume after the CAN bus restarted, but
sometimes the system got stuck and refused to send any more packets.
The sending process would be in either poll() or send(), waiting for
the queue to resume. To "unstuck" the process from send() it was
sufficient to send any packet on the can interface from another
process. To get it out of the poll() hang, only bringing the can
interface down (and up) would work.

After adding the netif_stop_queue call, I was unable to reproduce the
problem.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index f441f2265299e..9770416b24723 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -1131,10 +1131,11 @@ static int mcp251xfd_handle_cerrif(struct mcp251xfd_priv *priv)
 		 */
 		err = __mcp251xfd_get_berr_counter(priv->ndev, &priv->bec);
 		if (err)
-			return err;
+			netdev_err(priv->ndev, "Failed to store BER counter\n");
 
 		mcp251xfd_chip_stop(priv, CAN_STATE_BUS_OFF);
 		can_bus_off(priv->ndev);
+		netif_stop_queue(priv->ndev);
 	}
 
 	if (!skb)
---
base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
branch: linux-master-can-mcp251xfd-queue

-- 
2.43.0


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

Re: [PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] can: mcp251xfd: netif_stop_queue() race in
  mcp251xfd_handle_cerrif()
- [Low] can: mcp251xfd: missing Fixes trailer for
  mcp251xfd_handle_cerrif() fix
--

commit 04d0094bdd9ffb626b769cf55b6bbc25e504875f
Author: Mike Looijmans <mike.looijmans@topic.nl>

can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

This commit changes mcp251xfd_handle_cerrif() on bus-off to log an error
instead of aborting when reading the BER counter fails, and calls
netif_stop_queue() after stopping the chip and invoking can_bus_off().

[Severity: Low]
In the commit message, since this is a bug fix for the bus-off handling
in mcp251xfd_handle_cerrif(), please consider adding a Fixes: trailer:

  Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")

[Severity: Medium]
In drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c, inside
mcp251xfd_handle_cerrif() (around line 1138), can calling
netif_stop_queue(priv->ndev) after mcp251xfd_chip_stop() and
can_bus_off() race with concurrent transmissions and bus-off recovery?

CPU0 (threaded IRQ)               CPU1 (kworker / restart)
mcp251xfd_handle_cerrif()
  mcp251xfd_chip_stop()
  can_bus_off()
    schedule_delayed_work()
                                  can_restart_work()
                                    can_restart()
                                      mcp251xfd_set_mode()
                                        netif_wake_queue(ndev)
  netif_stop_queue(priv->ndev)
    <-- TX queue left stopped

can_bus_off() schedules priv->can.restart_work, which runs can_restart()
and mcp251xfd_set_mode(), reinitializing the TX ring to empty and calling
netif_wake_queue(ndev).  If the threaded IRQ handler is preempted after
can_bus_off(), netif_stop_queue() runs after mcp251xfd_set_mode() wakes
the queue.  With an empty TX ring, no TEF completion interrupt will fire
to wake the queue again, leaving TX permanently stopped until the
interface is cycled down and up.

In addition, mcp251xfd_chip_stop() performs sleeping SPI transfers while
disabling interrupts and entering configuration mode.  Leaving the TX
queue active until after mcp251xfd_chip_stop() returns allows
mcp251xfd_start_xmit() to submit new frames concurrently while the chip
is shutting down.

Calling netif_stop_queue(priv->ndev) at the start of the
CAN_STATE_BUS_OFF block before mcp251xfd_chip_stop() and can_bus_off()
(matching the order in mcp251xfd_stop()) avoids both races.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109070605.9761-1-mike.looijmans@topic.nl?part=1

Re: [PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[Marc Kleine-Budde]

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

On 09.11.2026 08:06:03, Mike Looijmans wrote:
> When there's an error attempting to store the BER counter, don't abort
> but continue shutting down the chip as required.
>
> After disabling communications, also stop the TX queue with a call to
> netif_stop_queue. When the interface restarts, mcp251xfd_set_mode will
> call netif_wake_queue and resume.
>
> This fixes a hangup in either send() or poll() from userspace. To
> reproduce: I ran "cansequence can0 -p" to flood the system with packets.
> While running that, I shorted the CAN signals, causing a bus error.
> Usually communications would resume after the CAN bus restarted, but
> sometimes the system got stuck and refused to send any more packets.
> The sending process would be in either poll() or send(), waiting for
> the queue to resume. To "unstuck" the process from send() it was
> sufficient to send any packet on the can interface from another
> process. To get it out of the poll() hang, only bringing the can
> interface down (and up) would work.
>
> After adding the netif_stop_queue call, I was unable to reproduce the
> problem.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>
> ---
>
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index f441f2265299e..9770416b24723 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -1131,10 +1131,11 @@ static int mcp251xfd_handle_cerrif(struct mcp251xfd_priv *priv)
>  		 */
>  		err = __mcp251xfd_get_berr_counter(priv->ndev, &priv->bec);
>  		if (err)
> -			return err;
> +			netdev_err(priv->ndev, "Failed to store BER counter\n");

Do you see this error message? The only thing that can fail is the SPI
communication (and probably some OOM) and if that fails we have a bigger
problem. Can you adjust the error message, that it prints "Failed to read
BER counter\n", which is a bit more towards the root cause of the problem.

>
>  		mcp251xfd_chip_stop(priv, CAN_STATE_BUS_OFF);
>  		can_bus_off(priv->ndev);
> +		netif_stop_queue(priv->ndev);

I think the cause is that can_bus_off() asynchronously stops the netif
queue. Can you apply this patch (instead of yours) and check if it
triggers?

--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -156,6 +156,12 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s
        u32 silent_mode = priv->ctrlmode & (CAN_CTRLMODE_LISTENONLY |
                                            CAN_CTRLMODE_RESTRICTED);

+       if (priv->state >= CAN_STATE_BUS_OFF) {
+               netdev_info_once(dev, "interface in %s state, dropping skb\n",
+                                can_get_state_str(priv->state));
+               goto invalid_skb;
+       }
+
        if (silent_mode) {
                netdev_info_once(dev, "interface in %s mode, dropping skb\n",
                                 can_get_ctrlmode_str(silent_mode));

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] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[Marc Kleine-Budde]

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

On 09.11.2026 07:24:53, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Medium] can: mcp251xfd: netif_stop_queue() race in
>   mcp251xfd_handle_cerrif()
> - [Low] can: mcp251xfd: missing Fixes trailer for
>   mcp251xfd_handle_cerrif() fix
> --
>
> commit 04d0094bdd9ffb626b769cf55b6bbc25e504875f
> Author: Mike Looijmans <mike.looijmans@topic.nl>
>
> can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue
>
> This commit changes mcp251xfd_handle_cerrif() on bus-off to log an error
> instead of aborting when reading the BER counter fails, and calls
> netif_stop_queue() after stopping the chip and invoking can_bus_off().
>
> [Severity: Low]
> In the commit message, since this is a bug fix for the bus-off handling
> in mcp251xfd_handle_cerrif(), please consider adding a Fixes: trailer:
>
>   Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")
>
> [Severity: Medium]
> In drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c, inside
> mcp251xfd_handle_cerrif() (around line 1138), can calling
> netif_stop_queue(priv->ndev) after mcp251xfd_chip_stop() and
> can_bus_off() race with concurrent transmissions and bus-off recovery?
>
> CPU0 (threaded IRQ)               CPU1 (kworker / restart)
> mcp251xfd_handle_cerrif()
>   mcp251xfd_chip_stop()
>   can_bus_off()
>     schedule_delayed_work()
>                                   can_restart_work()
>                                     can_restart()
>                                       mcp251xfd_set_mode()
>                                         netif_wake_queue(ndev)
>   netif_stop_queue(priv->ndev)
>     <-- TX queue left stopped
>
> can_bus_off() schedules priv->can.restart_work, which runs can_restart()
> and mcp251xfd_set_mode(), reinitializing the TX ring to empty and calling
> netif_wake_queue(ndev).  If the threaded IRQ handler is preempted after
> can_bus_off(), netif_stop_queue() runs after mcp251xfd_set_mode() wakes
> the queue.  With an empty TX ring, no TEF completion interrupt will fire
> to wake the queue again, leaving TX permanently stopped until the
> interface is cycled down and up.
>
> In addition, mcp251xfd_chip_stop() performs sleeping SPI transfers while
> disabling interrupts and entering configuration mode.  Leaving the TX
> queue active until after mcp251xfd_chip_stop() returns allows
> mcp251xfd_start_xmit() to submit new frames concurrently while the chip
> is shutting down.
>
> Calling netif_stop_queue(priv->ndev) at the start of the
> CAN_STATE_BUS_OFF block before mcp251xfd_chip_stop() and can_bus_off()
> (matching the order in mcp251xfd_stop()) avoids both races.

Please fix the race condition, the suggestion looks good to me.

Marc

pr-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] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[Mike Looijmans]

On 09-11-2026 12:26, Marc Kleine-Budde wrote:
> On 09.11.2026 08:06:03, Mike Looijmans wrote:
>> When there's an error attempting to store the BER counter, don't abort
>> but continue shutting down the chip as required.
>>
>> After disabling communications, also stop the TX queue with a call to
>> netif_stop_queue. When the interface restarts, mcp251xfd_set_mode will
>> call netif_wake_queue and resume.
>>
>> This fixes a hangup in either send() or poll() from userspace. To
>> reproduce: I ran "cansequence can0 -p" to flood the system with packets.
>> While running that, I shorted the CAN signals, causing a bus error.
>> Usually communications would resume after the CAN bus restarted, but
>> sometimes the system got stuck and refused to send any more packets.
>> The sending process would be in either poll() or send(), waiting for
>> the queue to resume. To "unstuck" the process from send() it was
>> sufficient to send any packet on the can interface from another
>> process. To get it out of the poll() hang, only bringing the can
>> interface down (and up) would work.
>>
>> After adding the netif_stop_queue call, I was unable to reproduce the
>> problem.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>
>> ---
>>
>>   drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
>> index f441f2265299e..9770416b24723 100644
>> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
>> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
>> @@ -1131,10 +1131,11 @@ static int mcp251xfd_handle_cerrif(struct mcp251xfd_priv *priv)
>>   		 */
>>   		err = __mcp251xfd_get_berr_counter(priv->ndev, &priv->bec);
>>   		if (err)
>> -			return err;
>> +			netdev_err(priv->ndev, "Failed to store BER counter\n");
> Do you see this error message? The only thing that can fail is the SPI
> communication (and probably some OOM) and if that fails we have a bigger
> problem. Can you adjust the error message, that it prints "Failed to read
> BER counter\n", which is a bit more towards the root cause of the problem.

I may have seen this trigger only once ever. It's extremely unlikely to 
ever happen, but if it does, I still think it's better to at least try 
and tear things down further than just giving up and leaving things in a 
possibly inconsistent state.


>>   		mcp251xfd_chip_stop(priv, CAN_STATE_BUS_OFF);
>>   		can_bus_off(priv->ndev);
>> +		netif_stop_queue(priv->ndev);
> I think the cause is that can_bus_off() asynchronously stops the netif
> queue. Can you apply this patch (instead of yours) and check if it
> triggers?
>
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -156,6 +156,12 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s
>          u32 silent_mode = priv->ctrlmode & (CAN_CTRLMODE_LISTENONLY |
>                                              CAN_CTRLMODE_RESTRICTED);
>
> +       if (priv->state >= CAN_STATE_BUS_OFF) {
> +               netdev_info_once(dev, "interface in %s state, dropping skb\n",
> +                                can_get_state_str(priv->state));
> +               goto invalid_skb;
> +       }
> +
>          if (silent_mode) {
>                  netdev_info_once(dev, "interface in %s mode, dropping skb\n",
>                                   can_get_ctrlmode_str(silent_mode));
>
I'll try, will have to re-assemble the test setup.

Meanwhile I'll post a v2 patch with the suggested changes. We've been 
running with the original patch for at least a year and it made the 
occasional CAN bus lockups we saw in the field disappear.


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


-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Re: [PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 2 (2)

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 new netif_stop_queue() in mcp251xfd_handle_cerrif() (line
  1138) runs after the state that allows a restart has been published,…
- [Medium] The new netif_stop_queue() is a lockless bit set from the
  threaded IRQ handler.
- [Low] The patch fixes a hang that users can see: after bus-off, a
  sending process can block in send()/poll() until the interface is…
- [Low] The commit message does not explain why the hang happens.

Pre-existing issues:
- [Medium] mcp251xfd_chip_stop() (lines 755-764) ignores errors from
  mcp251xfd_chip_interrupts_disable(), mcp251xfd_chip_rx_int_disable()…
- [Low] If __mcp251xfd_get_berr_counter() fails in
  mcp251xfd_handle_cerrif(), priv->bec is not updated.
- [Low] This problem wasn't introduced by this patch, but in
  mcp251xfd_handle_cerrif() the non-bus-off branch leaks the error skb.

--

can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

On bus-off, this patch keeps shutting the chip down even when the error
counter read fails, and logs an error instead of returning early. It also
stops the TX queue after can_bus_off() and relies on mcp251xfd_set_mode()
to wake it on restart. The goal is to fix a send()/poll() hang seen after
bus-off under heavy TX load.

> This fixes a hangup in either send() or poll() from userspace. To
> reproduce: I ran "cansequence can0 -p" to flood the system with packets.

[Severity: Low]
Should this have a Fixes: tag and Cc: stable? The bus-off block in
mcp251xfd_handle_cerrif() goes back to the original driver submission.
That includes the early return on a failed counter read and the missing
queue stop. The only later change was the rename in eb79a267c9b3:

Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")

> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index f441f2265299e..9770416b24723 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -1131,10 +1131,11 @@ static int mcp251xfd_handle_cerrif(struct mcp251xfd_priv *priv)
>  		 */
>  		err = __mcp251xfd_get_berr_counter(priv->ndev, &priv->bec);

[Severity: Low]
This isn't a bug introduced by this patch, but if
__mcp251xfd_get_berr_counter() fails here, priv->bec is not updated.

can_change_state() has already set priv->can.state to CAN_STATE_BUS_OFF.
That means mcp251xfd_get_berr_counter() returns the stale copy to
userspace:

	if (priv->can.state == CAN_STATE_BUS_OFF) {
		*bec = priv->bec;
		return 0;
	}

The old return err path gave the same result.

Could priv->bec be filled from the trec value already read at the top of
mcp251xfd_handle_cerrif(), instead of doing a second TREC read?

>  		if (err)
> -			return err;
> +			netdev_err(priv->ndev, "Failed to store BER counter\n");

[Severity: Low]
Would it be useful to print err in this message? Before, the error went
to the out_fail path in mcp251xfd_irq(), which printed the errno and
called mcp251xfd_dump(). Now the error code is lost.

The commit message doesn't say what this hunk fixes either. The old
return err leaked the error skb allocated by
mcp251xfd_alloc_can_err_skb(). It also left priv->can.state at
CAN_STATE_BUS_OFF (set by can_change_state()), with the carrier still on
and no restart scheduled.

For the netif_stop_queue() part, the only justification given is:

  After adding the netif_stop_queue call, I was unable to reproduce the
  problem.

Can the commit message explain the root cause? can_restart() assumes
nothing is being transmitted during bus-off:

	/* No synchronization needed because the device is bus-off and
	 * no messages can come in or go out.
	 */
	can_flush_echo_skb(dev);

This driver broke that assumption by leaving the TX queue running.

>  
>  		mcp251xfd_chip_stop(priv, CAN_STATE_BUS_OFF);

[Severity: Medium]
This is a pre-existing issue, but mcp251xfd_chip_stop() ignores errors
from mcp251xfd_chip_interrupts_disable(), mcp251xfd_chip_rx_int_disable()
and mcp251xfd_chip_set_mode(priv, MCP251XFD_REG_CON_MODE_CONFIG). With
this patch it is now also called after a failed TREC read.

Suppose the TREC read and the MODE_CONFIG write both fail briefly, and
SPI recovers before the restart. Could the restart then reprogram the
FIFOs while the chip is still in Normal mode, causing TEF consistency
errors?

The other callers of mcp251xfd_chip_stop() use the same unchecked,
best-effort pattern. This only matters when both failures happen in that
order.

>  		can_bus_off(priv->ndev);
> +		netif_stop_queue(priv->ndev);

[Severity: High]
Can this netif_stop_queue() undo the wake from a restart that has
already finished?

By this point, mcp251xfd_chip_stop() has set CAN_STATE_BUS_OFF, and
can_bus_off() has scheduled restart_work if restart_ms is set. When
restart_ms is 0, a manual restart through can_restart_now() only needs
priv->state == CAN_STATE_BUS_OFF. Neither restart path is serialized
against the IRQ thread:

restart path
can_restart()
  mcp251xfd_set_mode()
    mcp251xfd_chip_start()     /* state = CAN_STATE_ERROR_ACTIVE */
    mcp251xfd_chip_interrupts_enable()
    netif_wake_queue(ndev);

IRQ thread
mcp251xfd_handle_cerrif()
  mcp251xfd_chip_stop(priv, CAN_STATE_BUS_OFF);
  can_bus_off(priv->ndev);
  netif_stop_queue(priv->ndev);   /* runs after the wake */

If the wake happens first, the chip runs in ERROR_ACTIVE with an empty TX
ring. mcp251xfd_handle_tefif() then never wakes the queue. A manual
can_restart_now() also returns -EBUSY, because the state is no longer
BUS_OFF.

Wouldn't the queue then stay stopped until ifdown/ifup? That is the hang
this patch is meant to fix.

Would stopping the queue before mcp251xfd_chip_stop() and can_bus_off()
avoid this?

[Severity: Medium]
Is this stop enough to keep the queue stopped during bus-off? Here,
netif_stop_queue() is a lockless bit set from the threaded IRQ handler.
It is not serialized against mcp251xfd_start_xmit(), and two other places
can clear the bit without checking for bus-off.

mcp251xfd_tx_busy() can restart the queue:

	netif_stop_queue(priv->ndev);

	/* Memory barrier before checking tx_free (head and tail) */
	smp_mb();

	if (mcp251xfd_get_tx_free(tx_ring) == 0) {
		...
		return true;
	}

	netif_start_queue(priv->ndev);

The IRQ loop handles TEF before CERRIF in the same pass. An xmit that saw
the advanced tail and was then delayed can call netif_start_queue() after
the bus-off stop.

mcp251xfd_tx_failure_drop() also calls netif_wake_queue()
unconditionally. It can be reached from a failing spi_async() in the
xmit path, or from tx_work through mcp251xfd_tx_obj_write_sync(). tx_work
is not cancelled on bus-off.

Either way, the queue runs again while the chip is in CONFIG mode. That
brings back the concurrency between can_put_echo_skb() and
can_flush_echo_skb() in can_restart(). An xmit that has already passed
the stopped check can also still write TX objects to the stopped chip.

Would these windows close if netif_tx_disable() were called before
mcp251xfd_chip_stop(), and the wake in mcp251xfd_tx_failure_drop() were
made bus-off aware (or tx_work cancelled)?

>  	}
>  
>  	if (!skb)

[Severity: Low]
This isn't a bug introduced by this patch, but further down in
mcp251xfd_handle_cerrif() the non-bus-off branch has a similar early
return:

	if (new_state != CAN_STATE_BUS_OFF) {
		struct can_berr_counter bec;

		err = mcp251xfd_get_berr_counter(priv->ndev, &bec);
		if (err)
			return err;

Does this return leak the skb from mcp251xfd_alloc_can_err_skb()? On
this path the skb is neither freed nor queued to rx_offload. The IRQ
handler then goes to out_fail, so at most one skb leaks per fatal SPI
error. This code dates back to 55e5b97f003e.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109070605.9761-1-mike.looijmans%40topic.nl