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

[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

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