Skip to contentSkip to conversation

can: virtio: Add virtio CAN driver

PATCH Superseded Matias Ezequiel Vara Larsen 3 participants 6 messages Mar 20 – Apr 7

Subsystems (3)
virtio core CAN network drivers virtio CAN driver

v13 of 15 Superseded Latest is v15 →

lore
Share & tools
lore

Matias Ezequiel Vara Larsen

author maintainer
#0 lore raw

PATCH v13 1/1 can: virtio: Add virtio CAN driver

Add virtio CAN driver based on Virtio 1.4 specification (see
https://github.com/oasis-tcs/virtio-spec/tree/virtio-1.4). The driver implements a complete CAN bus interface over Virtio transport, supporting both CAN Classic and CAN-FD Ids. In term of frames, it supports classic and CAN FD. RTR frames are only supported with classic CAN.

Usage:
- "ip link set up can0" - start controller
- "ip link set down can0" - stop controller
- "candump can0" - receive frames
- "cansend can0 123#DEADBEEF" - send frames

Signed-off-by Harald Mommer <[email protected]>
Co-developed-by Harald Mommer <[email protected]>
Signed-off-by Mikhail Golubev-Ciuchea <[email protected]>
Co-developed-by Marc Kleine-Budde <[email protected]>
Signed-off-by Marc Kleine-Budde <[email protected]>
Cc Damir Shaikhutdinov <[email protected]>
Reviewed-by Francesco Valla <[email protected]>
Tested-by Francesco Valla <[email protected]>
Signed-off-by Matias Ezequiel Vara Larsen <[email protected]>

---
V13:
- kick device only when some buffers have been added
- add virtio_can_del_vq() when virtio_can_start() fails
- move napi_disable() at the end of virtio_can_freeze()
- move napi_enable() at beginning of virtio_can_restore()
- let virtio_can_start() sets CAN_STATE_ERROR_ACTIVE
- in freeze(), if can_stop() fails, restore the state of the device
- in restore(), if !netif_running, set CAN_STATE_STOPPED

V12:
- check return value in virtio_can_start()
- use devm_krealloc() in virtio_can_restore() to re-allocate rpkt
- remove access to cf->len and replace it with len
- use __counted_by_le()
- free tx buffers during virtio_can_del_vq()
- add reinit_completion() for ctrl msgs

V11:
* Set CAN_VIRTIO_CAN config before CAN_XILINXCAN
* Use GFP_ATOMIC in virtio_can_alloc_tx_idx()
* Use open_candev() and set bittiming.bitrate to CAN_BITRATE_UNKNOWN and data_bittiming.bitrate to CAN_BITRATE_UNKNOWN
* Fix leak of skb by adding kfree_skb(skb)
* Fix out-of-bounds in virtio_can_populate_rx_vq()
* Initialize ret
* Use virtio_reset_device()
* Add napi_disable() in virtio_can_remove()
* Propagate error in virtio_can_start() and virtio_can_stop()

V10:
* Follow Reverse Christmas Tree convention

V9:
* Remove unnecessary comments
* Update maintainer list

V8:
* Address nits

V7:
* Address nits
* Remove unnecessary comments
* Remove io_callbacks[]
* Use guard() syntax
* Remove kicking for each inbuf
* replace sdu_len with rpkt_len
* Use devm_kzalloc()
* Use scoped_guard() to protect virtqueue_add_sgs() and virtqueue_kicks() for tx queue
* Tested with vhost-device-can
(see https://github.com/rust-vmm/vhost-device/tree/main/vhost-device-can) and Qemu (942b0d3) with [1]. A reviewer observed that the device stops to work after flooding from host. This issue is still present.

[1]
https://lore.kernel.org/qemu-devel/[email protected]/

V6:
* Address nits
* Check for error during register_virtio_can()
* Remove virtio_device_ready()
* Allocate virtio_can_rx rpkt[] at probe
* Define virtio_can_control struct
* Return VIRTIO_CAN_RESULT_NOT_OK after unlocking
* Define sdu[] as a flex array for both tx and rx. For rx, use VIRTIO_CAN_F_CAN_FD to figure out the max len for sdu
* Fix statistics in virtio_can_read_tx_queue() and
how we indicate error to the user when getting VIRTIO_CAN_RESULT_NOT_OK
* Fix syntax of virtio_find_vqs()
* Drop tx_list
* Fix values of VIRTIO_CAN_F_LATE_TX_ACK and VIRTIO_CAN_F_RTR_FRAMES
* Tested with vhost-device-can
(see
https://github.com/rust-vmm/vhost-device/tree/main/vhost-device-can)
and qemu (see
https://github.com/virtualopensystems/qemu/tree/vhu-can-rfc)

V5:
* Re-base on top of linux-next (next-20240103)
* Tested with https://github.com/OpenSynergy/qemu/tree/virtio-can-spec-rfc-v3

RFC V4:
* Apply reverse Christmas tree style
* Add member *classic_dlc to RX and TX CAN frames
* Fix race causing a NETDEV_TX_BUSY return

---
 MAINTAINERS                     |   9 +
 drivers/net/can/Kconfig         |  12 +
 drivers/net/can/Makefile        |   1 +
 drivers/net/can/virtio_can.c    | 991 ++++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_can.h |  78 +++
 5 files changed, 1091 insertions(+)
 create mode 100644 drivers/net/can/virtio_can.c
 create mode 100644 include/uapi/linux/virtio_can.h
MAINTAINERS +9 -0
index 80cd3498c293..f295a904c93e 100644
@@ -27068,6 +27068,15 @@ F:	drivers/scsi/virtio_scsi.c
 F:	include/uapi/linux/virtio_blk.h
 F:	include/uapi/linux/virtio_scsi.h
 
+VIRTIO CAN DRIVER
+M:	"Harald Mommer" <[email protected]>
+M:	"Matias Ezequiel Vara Larsen" <[email protected]>
+L:	[email protected]
+L:	[email protected]
+S:	Maintained
+F:	drivers/net/can/virtio_can.c
+F:	include/uapi/linux/virtio_can.h
+
 VIRTIO CONSOLE DRIVER
 M:	Amit Shah <[email protected]>
 L:	[email protected]
drivers/net/can/Kconfig +12 -0
index d43d56694667..f33ae46d9766 100644
@@ -209,6 +209,18 @@ config CAN_TI_HECC
 	  Driver for TI HECC (High End CAN Controller) module found on many
 	  TI devices. The device specifications are available from www.ti.com
 
+config CAN_VIRTIO_CAN
+	depends on VIRTIO
+	tristate "Virtio CAN device support"
+	default n
+	help
+	  Say Y here if you want to support for Virtio CAN.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called virtio-can.
+
+	  If unsure, say N.
+
 config CAN_XILINXCAN
 	tristate "Xilinx CAN"
 	depends on ARCH_ZYNQ || ARM64 || MICROBLAZE || COMPILE_TEST
drivers/net/can/Makefile +1 -0
index 56138d8ddfd2..2ddea733ed5d 100644
@@ -32,6 +32,7 @@ obj-$(CONFIG_CAN_PEAK_PCIEFD)	+= peak_canfd/
 obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
 obj-$(CONFIG_CAN_SUN4I)		+= sun4i_can.o
 obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
+obj-$(CONFIG_CAN_VIRTIO_CAN)	+= virtio_can.o
 obj-$(CONFIG_CAN_XILINXCAN)	+= xilinx_can.o
 
 subdir-ccflags-$(CONFIG_CAN_DEBUG_DEVICES) += -DDEBUG
drivers/net/can/virtio_can.c new file +991 -0

991 lines not on this page · raw message

include/uapi/linux/virtio_can.h new file +78 -0

78 lines not on this page · raw message

Signature · 1 line
2.42.0
maintainer
#1 lore raw

Hello,

The nipa/ai-review bot found these problems:

| https://netdev-ai.bots.linux.dev/ai-review.html?id=b046d75a-2de1-4063-bb96-8f64abc65e7d

commit 0f3cb3b3bc94eaff9b9709b528e498fe6150f9c1
Author: Matias Ezequiel Vara Larsen <[email protected]>

can: virtio: Add virtio CAN driver

This commit adds a virtio CAN driver implementing a complete CAN bus interface over Virtio transport, supporting both CAN Classic and CAN-FD frames.

Quoted 5 lines
drivers/net/can/virtio_can.c new file +0 -0
new file mode 100644
index 000000000000..57641c3ed6d0

[ ... ]

drivers/net/can/virtio_can.c line 933

Quoted 13 lines
drivers/net/can/virtio_can.c new file +13 -0
+static int __maybe_unused virtio_can_restore(struct virtio_device *vdev)
+{
+	struct virtio_can_priv *priv = vdev->priv;
+	struct net_device *ndev = priv->dev;
+	size_t size;
+	int err;
+
+	napi_enable(&priv->napi);
+	napi_enable(&priv->napi_tx);
+
+	err = virtio_can_find_vqs(priv);
+	if (err != 0)
+		return err;

What happens to the enabled NAPI instances when this error path is taken? virtio_can_restore() enables both NAPI instances at the start, but if virtio_can_find_vqs() fails here, the function returns without calling napi_disable().

The corresponding virtio_can_freeze() function disables both NAPIs before returning, so the restore function leaves the NAPI subsystem in an inconsistent state on failure.

If the PM framework retries restore after this failure, the second napi_enable() call on an already-enabled NAPI will hit the BUG_ON in napi_enable_locked():

net/core/dev.c:napi_enable_locked() {
	BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
	...
}

In contrast, virtio_can_probe() enables NAPI only after all setup succeeds, avoiding this issue.

drivers/net/can/virtio_can.c line 940

Quoted 7 lines
drivers/net/can/virtio_can.c new file +7 -0
+
+	size = priv->rpkt_len * priv->vqs[VIRTIO_CAN_QUEUE_RX]->num_free;
+	priv->rpkt = devm_krealloc(&vdev->dev, priv->rpkt, size, GFP_KERNEL | __GFP_ZERO);
+	if (!priv->rpkt) {
+		virtio_can_del_vq(vdev);
+		return -ENOMEM;
+	}

The same NAPI state issue occurs on this error path.

drivers/net/can/virtio_can.c line 948

Quoted 8 lines
drivers/net/can/virtio_can.c new file +8 -0
+	virtio_can_populate_rx_vq(vdev);
+
+	if (netif_running(ndev)) {
+		err = virtio_can_start(ndev);
+		if (err) {
+			virtio_can_del_vq(vdev);
+			return err;
+		}

And again on this error path.

Should all three error paths call napi_disable(&priv->napi_tx) and napi_disable(&priv->napi) before returning the error code?

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   |
author maintainer
#2 lore raw
Quoted 2 lines · Marc Kleine-Budde

Hello,

Hello and thanks, I am struggling to come up with a solution for this comment.

Quoted 31 lines

The nipa/ai-review bot found these problems:

| https://netdev-ai.bots.linux.dev/ai-review.html?id=b046d75a-2de1-4063-bb96-8f64abc65e7d

commit 0f3cb3b3bc94eaff9b9709b528e498fe6150f9c1
Author: Matias Ezequiel Vara Larsen <[email protected]>

can: virtio: Add virtio CAN driver

This commit adds a virtio CAN driver implementing a complete CAN bus interface over Virtio transport, supporting both CAN Classic and CAN-FD frames.

Quoted 5 lines
drivers/net/can/virtio_can.c new file +0 -0
new file mode 100644
index 000000000000..57641c3ed6d0

[ ... ]

Quoted 10 lines
drivers/net/can/virtio_can.c new file +10 -0
+static int __maybe_unused virtio_can_restore(struct virtio_device *vdev)
+{
+	struct virtio_can_priv *priv = vdev->priv;
+	struct net_device *ndev = priv->dev;
+	size_t size;
+	int err;
+
+	napi_enable(&priv->napi);
+	napi_enable(&priv->napi_tx);
+
Quoted 9 lines

drivers/net/can/virtio_can.c line 890

Quoted 3 lines
drivers/net/can/virtio_can.c new file +3 -0
+	err = virtio_can_find_vqs(priv);
+	if (err != 0)
+		return err;

What happens to the enabled NAPI instances when this error path is taken? virtio_can_restore() enables both NAPI instances at the start, but if virtio_can_find_vqs() fails here, the function returns without calling napi_disable().

I was expecting that, if virtio_can_restore() fails, then a successive virtio_can_remove() will not hang in napi_disable() since the napis were already enabled.

Quoted 8 lines

The corresponding virtio_can_freeze() function disables both NAPIs before returning, so the restore function leaves the NAPI subsystem in an inconsistent state on failure.

If the PM framework retries restore after this failure, the second napi_enable() call on an already-enabled NAPI will hit the BUG_ON in napi_enable_locked():

Does the PM framework really retries if virtio_can_restore() fails? If not, that BUG_ON is never reached.

Quoted 37 lines
net/core/dev.c:napi_enable_locked() {
	BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
	...
}

In contrast, virtio_can_probe() enables NAPI only after all setup succeeds, avoiding this issue.

drivers/net/can/virtio_can.c line 940

Quoted 7 lines
drivers/net/can/virtio_can.c new file +7 -0
+
+	size = priv->rpkt_len * priv->vqs[VIRTIO_CAN_QUEUE_RX]->num_free;
+	priv->rpkt = devm_krealloc(&vdev->dev, priv->rpkt, size, GFP_KERNEL | __GFP_ZERO);
+	if (!priv->rpkt) {
+		virtio_can_del_vq(vdev);
+		return -ENOMEM;
+	}

The same NAPI state issue occurs on this error path.

drivers/net/can/virtio_can.c line 948

Quoted 8 lines
drivers/net/can/virtio_can.c new file +8 -0
+	virtio_can_populate_rx_vq(vdev);
+
+	if (netif_running(ndev)) {
+		err = virtio_can_start(ndev);
+		if (err) {
+			virtio_can_del_vq(vdev);
+			return err;
+		}

And again on this error path.

Should all three error paths call napi_disable(&priv->napi_tx) and napi_disable(&priv->napi) before returning the error code?

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   |
#3 lore raw

Hi Matias,

I've been testing PATCH v13 of the virtio CAN driver and encountered a FORTIFY_SOURCE panic when transmitting frames:

	sh-5.3# cansend can0 123#DEADBEEF                                                                                     
	[   51.700501] Kernel BUG at __fortify_panic+0x9/0xb [verbose debug info unavailable]                                 
	[   51.700798] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI                                                      
	[   51.700881] CPU: 2 UID: 0 PID: 374 Comm: cansend Tainted: G        W          6.12.76 #1                           
	[   51.701070] Tainted: [W]=WARN                                                                                      
	[   51.701143] RIP: 0010:__fortify_panic+0x9/0xb                                                                      
	[   51.701212] Code: 01 00 00 e9 58 7e c2 ff cc cc cc cc cc cc cc cc cc 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90  
	90 40 0f b6 ff e8 57 a9 c2 ff <0f> 0b 48 8b 54 24 08 48 8b 74 24 10 4c 8d 44 24 1d 4c 89 e1 48 c7                     
	[   51.701406] RSP: 0018:ffffc900001ffb10 EFLAGS: 00010246                                                            
	[   51.701454] RAX: 0000000000000000 RBX: ffff888100ea8780 RCX: 0000000000000003                                      
	[   51.701530] RDX: 0000000000000000 RSI: ffffc900001ff9b8 RDI: 0000000000000001                                      
	[   51.701625] RBP: 0000000000000000 R08: 0000000000000000 R09: 00000000fffffbff                                      
	[   51.701700] R10: ffffffff82239ee0 R11: ffffc900001ff9b0 R12: ffff888100ea8000                                      
	[   51.701789] R13: ffff888100817200 R14: ffff88810037cda0 R15: ffffc900001ffb48                                      
	[   51.701866] FS:  00007f7c4cda3740(0000) GS:ffff88812bd00000(0000) knlGS:0000000000000000                           
	[   51.701948] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033                                                      
	[   51.702007] CR2: 00007f7c4ceffdc0 CR3: 0000000100d12000 CR4: 0000000000350eb0                                      
	[   51.702072] Call Trace:                                                                                            
	[   51.702105]  <TASK>                                                                                                
	[   51.702126]  ? virtio_can_start_xmit.cold+0x2b/0x4d                                                                
	[   51.702171]  ? srso_alias_return_thunk+0x5/0xfbef5

The issue is in virtio_can_start_xmit() where can_tx_msg->tx_out.length is set AFTER memcpy(can_tx_msg->tx_out.sdu, ...). Since sdu[] uses __counted_by_le(length), FORTIFY_SOURCE sees length=0 during the copy and panics.

The fix is to set length before the memcpy:

drivers/net/can/virtio_can.c +1 -1
index xxx..yyy 100644
@@ -308,6 +308,7 @@ static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
 
 	can_tx_msg->tx_out.msg_type = cpu_to_le16(VIRTIO_CAN_TX);
+	can_tx_msg->tx_out.length = cpu_to_le16(cf->len);
 	can_flags = 0;
 
 	if (cf->can_id & CAN_EFF_FLAG) {

@@ -322,7 +323,6 @@ static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
 		can_flags |= VIRTIO_CAN_FLAGS_FD;
 
 	can_tx_msg->tx_out.flags = cpu_to_le32(can_flags);
-	can_tx_msg->tx_out.length = cpu_to_le16(cf->len);
 
 	sg_init_one(&sg_out, &can_tx_msg->tx_out, hdr_size + cf->len);

Tested with vhost-device-can backend, and it works correctly after this fix.

Thanks,
Dorinda Bassey

author maintainer
#4 lore raw
Quoted 27 lines · Dorinda Bassey

Hi Matias,

I've been testing PATCH v13 of the virtio CAN driver and encountered a FORTIFY_SOURCE panic when transmitting frames:

	sh-5.3# cansend can0 123#DEADBEEF                                                                                     
	[   51.700501] Kernel BUG at __fortify_panic+0x9/0xb [verbose debug info unavailable]                                 
	[   51.700798] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI                                                      
	[   51.700881] CPU: 2 UID: 0 PID: 374 Comm: cansend Tainted: G        W          6.12.76 #1                           
	[   51.701070] Tainted: [W]=WARN                                                                                      
	[   51.701143] RIP: 0010:__fortify_panic+0x9/0xb                                                                      
	[   51.701212] Code: 01 00 00 e9 58 7e c2 ff cc cc cc cc cc cc cc cc cc 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90  
	90 40 0f b6 ff e8 57 a9 c2 ff <0f> 0b 48 8b 54 24 08 48 8b 74 24 10 4c 8d 44 24 1d 4c 89 e1 48 c7                     
	[   51.701406] RSP: 0018:ffffc900001ffb10 EFLAGS: 00010246                                                            
	[   51.701454] RAX: 0000000000000000 RBX: ffff888100ea8780 RCX: 0000000000000003                                      
	[   51.701530] RDX: 0000000000000000 RSI: ffffc900001ff9b8 RDI: 0000000000000001                                      
	[   51.701625] RBP: 0000000000000000 R08: 0000000000000000 R09: 00000000fffffbff                                      
	[   51.701700] R10: ffffffff82239ee0 R11: ffffc900001ff9b0 R12: ffff888100ea8000                                      
	[   51.701789] R13: ffff888100817200 R14: ffff88810037cda0 R15: ffffc900001ffb48                                      
	[   51.701866] FS:  00007f7c4cda3740(0000) GS:ffff88812bd00000(0000) knlGS:0000000000000000                           
	[   51.701948] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033                                                      
	[   51.702007] CR2: 00007f7c4ceffdc0 CR3: 0000000100d12000 CR4: 0000000000350eb0                                      
	[   51.702072] Call Trace:                                                                                            
	[   51.702105]  <TASK>                                                                                                
	[   51.702126]  ? virtio_can_start_xmit.cold+0x2b/0x4d                                                                
	[   51.702171]  ? srso_alias_return_thunk+0x5/0xfbef5
Quoted 7 lines

The issue is in virtio_can_start_xmit() where can_tx_msg->tx_out.length is set AFTER memcpy(can_tx_msg->tx_out.sdu, ...). Since sdu[] uses __counted_by_le(length), FORTIFY_SOURCE sees length=0 during the copy and panics.

The fix is to set length before the memcpy:

Thanks I just pick it for v14! I did not observe this behavior before. I guess they added a new validation that I was not aware of.

Matias

Quoted 24 lines
drivers/net/can/virtio_can.c +1 -1
index xxx..yyy 100644
@@ -308,6 +308,7 @@ static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
 
 	can_tx_msg->tx_out.msg_type = cpu_to_le16(VIRTIO_CAN_TX);
+	can_tx_msg->tx_out.length = cpu_to_le16(cf->len);
 	can_flags = 0;
 
 	if (cf->can_id & CAN_EFF_FLAG) {

@@ -322,7 +323,6 @@ static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
 		can_flags |= VIRTIO_CAN_FLAGS_FD;
 
 	can_tx_msg->tx_out.flags = cpu_to_le32(can_flags);
-	can_tx_msg->tx_out.length = cpu_to_le16(cf->len);
 
 	sg_init_one(&sg_out, &can_tx_msg->tx_out, hdr_size + cf->len);

Tested with vhost-device-can backend, and it works correctly after this fix.

Thanks,
Dorinda Bassey

author maintainer
#5 lore raw
In reply to Matias Ezequiel Vara Larsen
Quoted 47 lines · Matias Ezequiel Vara Larsen
Quoted 2 lines · Marc Kleine-Budde

Hello,

Hello and thanks, I am struggling to come up with a solution for this comment.

Quoted 31 lines

The nipa/ai-review bot found these problems:

| https://netdev-ai.bots.linux.dev/ai-review.html?id=b046d75a-2de1-4063-bb96-8f64abc65e7d

commit 0f3cb3b3bc94eaff9b9709b528e498fe6150f9c1
Author: Matias Ezequiel Vara Larsen <[email protected]>

can: virtio: Add virtio CAN driver

This commit adds a virtio CAN driver implementing a complete CAN bus interface over Virtio transport, supporting both CAN Classic and CAN-FD frames.

Quoted 5 lines
drivers/net/can/virtio_can.c new file +0 -0
new file mode 100644
index 000000000000..57641c3ed6d0

[ ... ]

Quoted 10 lines
drivers/net/can/virtio_can.c new file +10 -0
+static int __maybe_unused virtio_can_restore(struct virtio_device *vdev)
+{
+	struct virtio_can_priv *priv = vdev->priv;
+	struct net_device *ndev = priv->dev;
+	size_t size;
+	int err;
+
+	napi_enable(&priv->napi);
+	napi_enable(&priv->napi_tx);
+
Quoted 9 lines

drivers/net/can/virtio_can.c line 890

Quoted 3 lines
drivers/net/can/virtio_can.c new file +3 -0
+	err = virtio_can_find_vqs(priv);
+	if (err != 0)
+		return err;

What happens to the enabled NAPI instances when this error path is taken? virtio_can_restore() enables both NAPI instances at the start, but if virtio_can_find_vqs() fails here, the function returns without calling napi_disable().

Quoted 4 lines

I was expecting that, if virtio_can_restore() fails, then a successive virtio_can_remove() will not hang in napi_disable() since the napis were already enabled.

Ping @Marc Kleine-Budde, do you have any comment about the comments above?

Thanks, Matias

Quoted 51 lines
Quoted 8 lines

The corresponding virtio_can_freeze() function disables both NAPIs before returning, so the restore function leaves the NAPI subsystem in an inconsistent state on failure.

If the PM framework retries restore after this failure, the second napi_enable() call on an already-enabled NAPI will hit the BUG_ON in napi_enable_locked():

Does the PM framework really retries if virtio_can_restore() fails? If not, that BUG_ON is never reached.

Quoted 37 lines
net/core/dev.c:napi_enable_locked() {
	BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
	...
}

In contrast, virtio_can_probe() enables NAPI only after all setup succeeds, avoiding this issue.

drivers/net/can/virtio_can.c line 940

Quoted 7 lines
drivers/net/can/virtio_can.c new file +7 -0
+
+	size = priv->rpkt_len * priv->vqs[VIRTIO_CAN_QUEUE_RX]->num_free;
+	priv->rpkt = devm_krealloc(&vdev->dev, priv->rpkt, size, GFP_KERNEL | __GFP_ZERO);
+	if (!priv->rpkt) {
+		virtio_can_del_vq(vdev);
+		return -ENOMEM;
+	}

The same NAPI state issue occurs on this error path.

drivers/net/can/virtio_can.c line 948

Quoted 8 lines
drivers/net/can/virtio_can.c new file +8 -0
+	virtio_can_populate_rx_vq(vdev);
+
+	if (netif_running(ndev)) {
+		err = virtio_can_start(ndev);
+		if (err) {
+			virtio_can_del_vq(vdev);
+			return err;
+		}

And again on this error path.

Should all three error paths call napi_disable(&priv->napi_tx) and napi_disable(&priv->napi) before returning the error code?

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   |