can: mcp251xfd: fix infinite loop when xmit fails

PATCH Superseded Vitor Soares 2 participants 5 messages Mar 7, 2024

Subsystems (3)

Stable branch CAN network drivers MCP251XFD SPI-CAN network driver

lore

Share & tools

lore

Vitor Soares

author

2024-03-07 12:05 UTC #0 lore raw

PATCH 1/1 can: mcp251xfd: fix infinite loop when xmit fails

From: Vitor Soares <[email protected]>

When the mcp251xfd_start_xmit() function fails, the driver stops processing messages, and the interrupt routine does not return, running indefinitely even after killing the running application.

Error messages:

[  441.298819] mcp251xfd spi2.0 can0: ERROR in mcp251xfd_start_xmit: -16
[  441.306498] mcp251xfd spi2.0 can0: Transmit Event FIFO buffer not empty. (seq=0x000017c7, tef_tail=0x000017cf, tef_head=0x000017d0, tx_head=0x000017d3).

... and repeat forever.

The issue can be triggered when multiple devices share the same SPI interface. And there is concurrent access to the bus.

The problem occurs because tx_ring->head increments even if mcp251xfd_start_xmit() fails. Consequently, the driver skips one TX package while still expecting a response in mcp251xfd_handle_tefif_one().

This patch resolves the issue by decreasing tx_ring->head if mcp251xfd_start_xmit() fails. With the fix, if we attempt to trigger the issue again, the driver prints an error and discard the message.

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

Cc [email protected]

Signed-off-by Vitor Soares <[email protected]>

---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c | 27 ++++++++++----------
 1 file changed, 14 insertions(+), 13 deletions(-)

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +14 ~~-13~~

index 160528d3cc26..a8eb941c1b95 100644
@@ -181,25 +181,26 @@ netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb,
 	tx_obj = mcp251xfd_get_tx_obj_next(tx_ring);
 	mcp251xfd_tx_obj_from_skb(priv, tx_obj, skb, tx_ring->head);
 
-	/* Stop queue if we occupy the complete TX FIFO */
 	tx_head = mcp251xfd_get_tx_head(tx_ring);
-	tx_ring->head++;
-	if (mcp251xfd_get_tx_free(tx_ring) == 0)
-		netif_stop_queue(ndev);
-
 	frame_len = can_skb_get_frame_len(skb);
-	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
-	if (!err)
-		netdev_sent_queue(priv->ndev, frame_len);
+	can_put_echo_skb(skb, ndev, tx_head, frame_len);
+
+	tx_ring->head++;
 
 	err = mcp251xfd_tx_obj_write(priv, tx_obj);
-	if (err)
-		goto out_err;
+	if (err) {
+		can_free_echo_skb(ndev, tx_head, NULL);
 
-	return NETDEV_TX_OK;
+		tx_ring->head--;
+
+		netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err);
+	} else {
+		/* Stop queue if we occupy the complete TX FIFO */
+		if (mcp251xfd_get_tx_free(tx_ring) == 0)
+			netif_stop_queue(ndev);
 
- out_err:
-	netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err);
+		netdev_sent_queue(priv->ndev, frame_len);
+	}
 
 	return NETDEV_TX_OK;
 }

Signature · 1 line

2.34.1

Marc Kleine-Budde

maintainer

2024-03-07 12:21 UTC #1 lore raw

Quoted 14 lines · Vitor Soares

From: Vitor Soares <[email protected]>
When the mcp251xfd_start_xmit() function fails, the driver stops processing messages, and the interrupt routine does not return, running indefinitely even after killing the running application.
Error messages:
[  441.298819] mcp251xfd spi2.0 can0: ERROR in mcp251xfd_start_xmit: -16
[  441.306498] mcp251xfd spi2.0 can0: Transmit Event FIFO buffer not empty. (seq=0x000017c7, tef_tail=0x000017cf, tef_head=0x000017d0, tx_head=0x000017d3).
... and repeat forever.
The issue can be triggered when multiple devices share the same SPI interface. And there is concurrent access to the bus.

Quoted 8 lines

The problem occurs because tx_ring->head increments even if mcp251xfd_start_xmit() fails. Consequently, the driver skips one TX package while still expecting a response in mcp251xfd_handle_tefif_one().
This patch resolves the issue by decreasing tx_ring->head if mcp251xfd_start_xmit() fails. With the fix, if we attempt to trigger the issue again, the driver prints an error and discard the message.

What about returning NETDEV_TX_BUSY, then the networking stack will retry.

Quoted 28 lines

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

Cc [email protected]

Signed-off-by Vitor Soares <[email protected]>

---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c | 27 ++++++++++----------
 1 file changed, 14 insertions(+), 13 deletions(-)

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +3 -8

index 160528d3cc26..a8eb941c1b95 100644
@@ -181,25 +181,26 @@ netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb,
 	tx_obj = mcp251xfd_get_tx_obj_next(tx_ring);
 	mcp251xfd_tx_obj_from_skb(priv, tx_obj, skb, tx_ring->head);
 
-	/* Stop queue if we occupy the complete TX FIFO */
 	tx_head = mcp251xfd_get_tx_head(tx_ring);
-	tx_ring->head++;
-	if (mcp251xfd_get_tx_free(tx_ring) == 0)
-		netif_stop_queue(ndev);
-
 	frame_len = can_skb_get_frame_len(skb);
-	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
-	if (!err)
-		netdev_sent_queue(priv->ndev, frame_len);
+	can_put_echo_skb(skb, ndev, tx_head, frame_len);
+
+	tx_ring->head++;

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb, · line 195

Quoted 10 lines

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +4 -3

 
 	err = mcp251xfd_tx_obj_write(priv, tx_obj);
-	if (err)
-		goto out_err;
+	if (err) {
+		can_free_echo_skb(ndev, tx_head, NULL);
 
-	return NETDEV_TX_OK;
+		tx_ring->head--;
+

I'm not sure, if we want an error message for -EBUSY. We could add proper ethtool statistics.

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb, · line 206

Quoted 13 lines

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +7 -2

+		netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err);
+	} else {
+		/* Stop queue if we occupy the complete TX FIFO */
+		if (mcp251xfd_get_tx_free(tx_ring) == 0)
+			netif_stop_queue(ndev);
 
- out_err:
-	netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err);
+		netdev_sent_queue(priv->ndev, frame_len);
+	}
 
 	return NETDEV_TX_OK;
 }

regards,
Marc

Signature · 4 lines

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   |

Marc Kleine-Budde

maintainer

2024-03-07 12:23 UTC #2 lore raw

In reply to Vitor Soares

Sorry, resend. I fsck up the linux-can ML's address.

Quoted 14 lines · Vitor Soares

From: Vitor Soares <[email protected]>
When the mcp251xfd_start_xmit() function fails, the driver stops processing messages, and the interrupt routine does not return, running indefinitely even after killing the running application.
Error messages:
[  441.298819] mcp251xfd spi2.0 can0: ERROR in mcp251xfd_start_xmit: -16
[  441.306498] mcp251xfd spi2.0 can0: Transmit Event FIFO buffer not empty. (seq=0x000017c7, tef_tail=0x000017cf, tef_head=0x000017d0, tx_head=0x000017d3).
... and repeat forever.
The issue can be triggered when multiple devices share the same SPI interface. And there is concurrent access to the bus.

Quoted 8 lines

The problem occurs because tx_ring->head increments even if mcp251xfd_start_xmit() fails. Consequently, the driver skips one TX package while still expecting a response in mcp251xfd_handle_tefif_one().
This patch resolves the issue by decreasing tx_ring->head if mcp251xfd_start_xmit() fails. With the fix, if we attempt to trigger the issue again, the driver prints an error and discard the message.

What about returning NETDEV_TX_BUSY, then the networking stack will retry.

Quoted 28 lines

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

Cc [email protected]

Signed-off-by Vitor Soares <[email protected]>

---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c | 27 ++++++++++----------
 1 file changed, 14 insertions(+), 13 deletions(-)

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +3 -8

index 160528d3cc26..a8eb941c1b95 100644
@@ -181,25 +181,26 @@ netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb,
 	tx_obj = mcp251xfd_get_tx_obj_next(tx_ring);
 	mcp251xfd_tx_obj_from_skb(priv, tx_obj, skb, tx_ring->head);
 
-	/* Stop queue if we occupy the complete TX FIFO */
 	tx_head = mcp251xfd_get_tx_head(tx_ring);
-	tx_ring->head++;
-	if (mcp251xfd_get_tx_free(tx_ring) == 0)
-		netif_stop_queue(ndev);
-
 	frame_len = can_skb_get_frame_len(skb);
-	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
-	if (!err)
-		netdev_sent_queue(priv->ndev, frame_len);
+	can_put_echo_skb(skb, ndev, tx_head, frame_len);
+
+	tx_ring->head++;

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb, · line 195

Quoted 10 lines

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +4 -3

 
 	err = mcp251xfd_tx_obj_write(priv, tx_obj);
-	if (err)
-		goto out_err;
+	if (err) {
+		can_free_echo_skb(ndev, tx_head, NULL);
 
-	return NETDEV_TX_OK;
+		tx_ring->head--;
+

I'm not sure, if we want an error message for -EBUSY. We could add proper ethtool statistics.

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c netdev_tx_t mcp251xfd_start_xmit(struct sk_buff *skb, · line 206

Quoted 13 lines

drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +7 -2

+		netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err);
+	} else {
+		/* Stop queue if we occupy the complete TX FIFO */
+		if (mcp251xfd_get_tx_free(tx_ring) == 0)
+			netif_stop_queue(ndev);
 
- out_err:
-	netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err);
+		netdev_sent_queue(priv->ndev, frame_len);
+	}
 
 	return NETDEV_TX_OK;
 }

regards,
Marc

Signature · 4 lines

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   |

vitor

author

2024-03-07 16:54 UTC #3 lore raw

Hi Marc,

Quoted 1 line · Marc Kleine-Budde

Sorry, resend. I fsck up the linux-can ML's address.

Thanks for that.

Quoted 24 lines · Vitor Soares

Quoted 22 lines
From: Vitor Soares <[email protected]>
When the mcp251xfd_start_xmit() function fails, the driver stops processing messages, and the interrupt routine does not return, running indefinitely even after killing the running application.
Error messages:
[ 441.298819] mcp251xfd spi2.0 can0: ERROR in
mcp251xfd_start_xmit: -16
[ 441.306498] mcp251xfd spi2.0 can0: Transmit Event FIFO buffer not empty. (seq=0x000017c7, tef_tail=0x000017cf, tef_head=0x000017d0, tx_head=0x000017d3).
... and repeat forever.
The issue can be triggered when multiple devices share the same SPI interface. And there is concurrent access to the bus.
The problem occurs because tx_ring->head increments even if mcp251xfd_start_xmit() fails. Consequently, the driver skips one TX package while still expecting a response in mcp251xfd_handle_tefif_one().

Quoted 8 lines

Quoted 5 lines
This patch resolves the issue by decreasing tx_ring->head if mcp251xfd_start_xmit() fails. With the fix, if we attempt to trigger
the issue again, the driver prints an error and discard the message.
What about returning NETDEV_TX_BUSY, then the networking stack will retry.

Do you mean when err == -EBUSY?

Quoted 36 lines

Quoted 36 lines
Fixes 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip
MCP25xxFD SPI CAN")
Cc [email protected]
Signed-off-by Vitor Soares <[email protected]>
---
drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c | 27 ++++++++++------
----
 1 file changed, 14 insertions(+), 13 deletions(-)
drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +0 -0
index 160528d3cc26..a8eb941c1b95 100644
@@ -181,25 +181,26 @@ netdev_tx_t mcp251xfd_start_xmit(struct
 sk_buff *skb,
         tx_obj = mcp251xfd_get_tx_obj_next(tx_ring);
        mcp251xfd_tx_obj_from_skb(priv, tx_obj, skb, tx_ring-
Quoted 1 line
head);

-       /* Stop queue if we occupy the complete TX FIFO */ tx_head = mcp251xfd_get_tx_head(tx_ring);
-       tx_ring->head++;
-       if (mcp251xfd_get_tx_free(tx_ring) == 0)
-               netif_stop_queue(ndev);
-
        frame_len = can_skb_get_frame_len(skb);
-       err = can_put_echo_skb(skb, ndev, tx_head, frame_len); -       if (!err)
-               netdev_sent_queue(priv->ndev, frame_len); +       can_put_echo_skb(skb, ndev, tx_head, frame_len); +
+       tx_ring->head++;

        err = mcp251xfd_tx_obj_write(priv, tx_obj);

Quoted 8 lines

Quoted 8 lines
-       if (err)
-               goto out_err;
+       if (err) {
+               can_free_echo_skb(ndev, tx_head, NULL);
-       return NETDEV_TX_OK;
+               tx_ring->head--;
+

I'm testing returning here when err = -EBUSY, but can_put_echo_skb() should be invoked after mcp251xfd_tx_obj_write(). Otherwise, I get a Kernel NULL pointer dereference error.

Quoted 19 lines

I'm not sure, if we want an error message for -EBUSY. We could add proper ethtool statistics.
Quoted 6 lines
+               netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err);
+       } else {
+               /* Stop queue if we occupy the complete TX FIFO */ +               if (mcp251xfd_get_tx_free(tx_ring) == 0) +                       netif_stop_queue(ndev);
Quoted 8 lines

- out_err:
-       netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err); +               netdev_sent_queue(priv->ndev, frame_len); +       }

        return NETDEV_TX_OK;
}

Quoted 3 lines

regards,
Marc

Best regards,
Vitor Soares

Marc Kleine-Budde

maintainer

2024-03-07 20:40 UTC #4 lore raw

Quoted 29 lines · vitor

Quoted 29 lines · Vitor Soares
Quoted 27 lines
From: Vitor Soares <[email protected]>
When the mcp251xfd_start_xmit() function fails, the driver stops processing messages, and the interrupt routine does not return, running indefinitely even after killing the running application.
Error messages:
[ 441.298819] mcp251xfd spi2.0 can0: ERROR in
mcp251xfd_start_xmit: -16
[ 441.306498] mcp251xfd spi2.0 can0: Transmit Event FIFO buffer not empty. (seq=0x000017c7, tef_tail=0x000017cf, tef_head=0x000017d0, tx_head=0x000017d3).
... and repeat forever.
The issue can be triggered when multiple devices share the same SPI interface. And there is concurrent access to the bus.
The problem occurs because tx_ring->head increments even if mcp251xfd_start_xmit() fails. Consequently, the driver skips one TX package while still expecting a response in mcp251xfd_handle_tefif_one().
This patch resolves the issue by decreasing tx_ring->head if mcp251xfd_start_xmit() fails. With the fix, if we attempt to trigger
the issue again, the driver prints an error and discard the message.

Quoted 4 lines

Quoted 2 lines
What about returning NETDEV_TX_BUSY, then the networking stack will retry.
Do you mean when err == -EBUSY?

ACK

Quoted 40 lines

Quoted 40 lines
Quoted 40 lines
Fixes 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip
MCP25xxFD SPI CAN")
Cc [email protected]
Signed-off-by Vitor Soares <[email protected]>
---
drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c | 27 ++++++++++------
----
 1 file changed, 14 insertions(+), 13 deletions(-)
drivers/net/can/spi/mcp251xfd/mcp251xfd-tx.c +0 -0
index 160528d3cc26..a8eb941c1b95 100644
@@ -181,25 +181,26 @@ netdev_tx_t mcp251xfd_start_xmit(struct
 sk_buff *skb,
         tx_obj = mcp251xfd_get_tx_obj_next(tx_ring);
        mcp251xfd_tx_obj_from_skb(priv, tx_obj, skb, tx_ring-
Quoted 1 line
head);

-       /* Stop queue if we occupy the complete TX FIFO */ tx_head = mcp251xfd_get_tx_head(tx_ring);
-       tx_ring->head++;
-       if (mcp251xfd_get_tx_free(tx_ring) == 0)
-               netif_stop_queue(ndev);
-
        frame_len = can_skb_get_frame_len(skb);
-       err = can_put_echo_skb(skb, ndev, tx_head, frame_len); -       if (!err)
-               netdev_sent_queue(priv->ndev, frame_len); +       can_put_echo_skb(skb, ndev, tx_head, frame_len); +
+       tx_ring->head++;

        err = mcp251xfd_tx_obj_write(priv, tx_obj); -       if (err)
-               goto out_err;
+       if (err) {
+               can_free_echo_skb(ndev, tx_head, NULL);

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines

- return NETDEV_TX_OK;
+ tx_ring->head--;
+
I'm testing returning here when err = -EBUSY, but can_put_echo_skb() should be invoked after mcp251xfd_tx_obj_write(). Otherwise, I get a Kernel NULL pointer dereference error.

I mean if mcp251xfd_tx_obj_write() returns -EBUSY don't print an error message, on any other error it's ok to print the error.

Quoted 4 lines

Quoted 4 lines
I'm not sure, if we want an error message for -EBUSY. We could add proper ethtool statistics.

Quoted 14 lines

Quoted 6 lines
Quoted 6 lines
+               netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err);
+       } else {
+               /* Stop queue if we occupy the complete TX FIFO */ +               if (mcp251xfd_get_tx_free(tx_ring) == 0) +                       netif_stop_queue(ndev);
Quoted 8 lines
Quoted 8 lines

- out_err:
-       netdev_err(priv->ndev, "ERROR in %s: %d\n", __func__, err); +               netdev_sent_queue(priv->ndev, frame_len); +       }

        return NETDEV_TX_OK;
}

Marc

Signature · 4 lines

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   |