Re: [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver.

From: Oliver Hartkopp
Date: Sat Aug 27 2022 - 07:12:49 EST




On 8/27/22 11:39, Marc Kleine-Budde wrote:
On 25.08.2022 15:44:49, Harald Mommer wrote:
- CAN Control

- "ip link set up can0" starts the virtual CAN controller,
- "ip link set up can0" stops the virtual CAN controller

- CAN RX

Receive CAN frames. CAN frames can be standard or extended, classic or
CAN FD. Classic CAN RTR frames are supported.

- CAN TX

Send CAN frames. CAN frames can be standard or extended, classic or
CAN FD. Classic CAN RTR frames are supported.

- CAN Event indication (BusOff)

The bus off handling is considered code complete but until now bus off
handling is largely untested.

Is there an Open Source implementation of the host side of this
interface?

Please fix these checkpatch warnings:

| WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
| #65:
| new file mode 100644
|
| WARNING: Use #include <linux/atomic.h> instead of <asm/atomic.h>
| #105: FILE: drivers/net/can/virtio_can/virtio_can.c:7:
| +#include <asm/atomic.h>
|
| WARNING: __always_unused or __maybe_unused is preferred over __attribute__((__unused__))
| #186: FILE: drivers/net/can/virtio_can/virtio_can.c:88:
| +static void __attribute__((unused))
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #263: FILE: drivers/net/can/virtio_can/virtio_can.c:165:
| + BUG_ON(prio != 0); /* Currently only 1 priority */
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #264: FILE: drivers/net/can/virtio_can/virtio_can.c:166:
| + BUG_ON(atomic_read(&priv->tx_inflight[0]) >= priv->can.echo_skb_max);
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #279: FILE: drivers/net/can/virtio_can/virtio_can.c:181:
| + BUG_ON(prio >= VIRTIO_CAN_PRIO_COUNT);
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #280: FILE: drivers/net/can/virtio_can/virtio_can.c:182:
| + BUG_ON(idx >= priv->can.echo_skb_max);
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #281: FILE: drivers/net/can/virtio_can/virtio_can.c:183:
| + BUG_ON(atomic_read(&priv->tx_inflight[prio]) == 0);
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #288: FILE: drivers/net/can/virtio_can/virtio_can.c:190:
| +/*
| + * Create a scatter-gather list representing our input buffer and put
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #309: FILE: drivers/net/can/virtio_can/virtio_can.c:211:
| +/*
| + * Send a control message with message type either
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #332: FILE: drivers/net/can/virtio_can/virtio_can.c:234:
| + /*
| + * The function may be serialized by rtnl lock. Not sure.
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #382: FILE: drivers/net/can/virtio_can/virtio_can.c:284:
| +/*
| + * See also m_can.c/m_can_set_mode()
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #408: FILE: drivers/net/can/virtio_can/virtio_can.c:310:
| +/*
| + * Called by issuing "ip link set up can0"
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #443: FILE: drivers/net/can/virtio_can/virtio_can.c:345:
| + /*
| + * Keep RX napi active to allow dropping of pending RX CAN messages,
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #481: FILE: drivers/net/can/virtio_can/virtio_can.c:383:
| + /*
| + * No local check for CAN_RTR_FLAG or FD frame against negotiated
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #521: FILE: drivers/net/can/virtio_can/virtio_can.c:423:
| + /*
| + * May happen if
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #533: FILE: drivers/net/can/virtio_can/virtio_can.c:435:
| + BUG_ON(can_tx_msg->putidx < 0);
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #613: FILE: drivers/net/can/virtio_can/virtio_can.c:515:
| + /*
| + * Here also frames with result != VIRTIO_CAN_RESULT_OK are
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #646: FILE: drivers/net/can/virtio_can/virtio_can.c:548:
| +/*
| + * Poll TX used queue for sent CAN messages
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #675: FILE: drivers/net/can/virtio_can/virtio_can.c:577:
| +/*
| + * This function is the NAPI RX poll function and NAPI guarantees that this
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #698: FILE: drivers/net/can/virtio_can/virtio_can.c:600:
| + BUG_ON(len < header_size);
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #813: FILE: drivers/net/can/virtio_can/virtio_can.c:715:
| +/*
| + * See m_can_poll() / m_can_handle_state_errors() m_can_handle_state_change().
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #855: FILE: drivers/net/can/virtio_can/virtio_can.c:757:
| +/*
| + * Poll RX used queue for received CAN messages
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #897: FILE: drivers/net/can/virtio_can/virtio_can.c:799:
| + /*
| + * The interrupt function is not assumed to be interrupted by
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #904: FILE: drivers/net/can/virtio_can/virtio_can.c:806:
| + BUG_ON(len < sizeof(struct virtio_can_event_ind));
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #966: FILE: drivers/net/can/virtio_can/virtio_can.c:868:
| + /*
| + * The order of RX and TX is exactly the opposite as in console and
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #976: FILE: drivers/net/can/virtio_can/virtio_can.c:878:
| + BUG_ON(!priv);
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #977: FILE: drivers/net/can/virtio_can/virtio_can.c:879:
| + BUG_ON(!priv->vdev);
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #1004: FILE: drivers/net/can/virtio_can/virtio_can.c:906:
| + /*
| + * From here we have dead silence from the device side so no locks
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #1025: FILE: drivers/net/can/virtio_can/virtio_can.c:927:
| + /*
| + * Is keeping track of allocated elements by an own linked list
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #1060: FILE: drivers/net/can/virtio_can/virtio_can.c:962:
| + BUG_ON(!vdev);
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #1063: FILE: drivers/net/can/virtio_can/virtio_can.c:965:
| + /*
| + * CAN needs always access to the config space.
|
| WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
| #1090: FILE: drivers/net/can/virtio_can/virtio_can.c:992:
| + BUG_ON(!vdev);
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #1144: FILE: drivers/net/can/virtio_can/virtio_can.c:1046:
| + /*
| + * It is possible to consider the number of TX queue places to
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #1185: FILE: drivers/net/can/virtio_can/virtio_can.c:1087:
| +/*
| + * Compare with m_can.c/m_can_suspend(), virtio_net.c/virtnet_freeze() and
|
| WARNING: networking block comments don't use an empty /* line, use /* Comment...
| #1210: FILE: drivers/net/can/virtio_can/virtio_can.c:1112:
| +/*
| + * Compare with m_can.c/m_can_resume(), virtio_net.c/virtnet_restore() and
|
| WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
| #1273: FILE: drivers/net/can/virtio_can/virtio_can.c:1175:
| +MODULE_LICENSE("GPL v2");

Btw. @Harald:

When you want to express your code to be GPLv2 why is your code GPL-2.0+ then?

diff --git a/drivers/net/can/virtio_can/virtio_can.c b/drivers/net/can/virtio_can/virtio_can.c
new file mode 100644
index 000000000000..dafbe5a36511
--- /dev/null
+++ b/drivers/net/can/virtio_can/virtio_can.c
@@ -0,0 +1,1176 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * CAN bus driver for the Virtio CAN controller
+ * Copyright (C) 2021 OpenSynergy GmbH
+ */

Best regards,
Oliver

|
| WARNING: From:/Signed-off-by: email address mismatch: 'From: Harald Mommer <harald.mommer@xxxxxxxxxxxxxxx>' != 'Signed-off-by: Harald Mommer <hmo@xxxxxxxxxxxxxxx>'
|
| total: 0 errors, 38 warnings, 1275 lines checked

regards,
Marc