Re: [RFC PATCH v2 1/2] can: virtio: Initial virtio CAN driver.
From: Harald Mommer
Date: Wed May 17 2023 - 07:43:25 EST
Hello,
(still sorting my E-Mail mess)
On 07.11.22 08:35, Vincent Mailhol wrote:
On Mon. 7 Nov. 2022 at 16:15, Alexander Stein
<alexander.stein@xxxxxxxxxxxxxxx> wrote:
Am Freitag, 4. November 2022, 18:24:20 CET schrieb Harald Mommer:
From: Harald Mommer <harald.mommer@xxxxxxxxxxxxxxx>
- 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.
This is version 2 of the driver after having gotten review comments.
Signed-off-by: Harald Mommer <Harald.Mommer@xxxxxxxxxxxxxxx>
Cc: Damir Shaikhutdinov <Damir.Shaikhutdinov@xxxxxxxxxxxxxxx>
[...]
diff --git a/include/uapi/linux/virtio_can.h
b/include/uapi/linux/virtio_can.h new file mode 100644
index 000000000000..0ca75c7a98ee
--- /dev/null
+++ b/include/uapi/linux/virtio_can.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Copyright (C) 2021 OpenSynergy GmbH
+ */
+#ifndef _LINUX_VIRTIO_VIRTIO_CAN_H
+#define _LINUX_VIRTIO_VIRTIO_CAN_H
+
+#include <linux/types.h>
+#include <linux/virtio_types.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+
+/* Feature bit numbers */
+#define VIRTIO_CAN_F_CAN_CLASSIC 0u
+#define VIRTIO_CAN_F_CAN_FD 1u
+#define VIRTIO_CAN_F_LATE_TX_ACK 2u
+#define VIRTIO_CAN_F_RTR_FRAMES 3u
+
+/* CAN Result Types */
+#define VIRTIO_CAN_RESULT_OK 0u
+#define VIRTIO_CAN_RESULT_NOT_OK 1u
+
+/* CAN flags to determine type of CAN Id */
+#define VIRTIO_CAN_FLAGS_EXTENDED 0x8000u
+#define VIRTIO_CAN_FLAGS_FD 0x4000u
+#define VIRTIO_CAN_FLAGS_RTR 0x2000u
+
+/* TX queue message types */
+struct virtio_can_tx_out {
+#define VIRTIO_CAN_TX 0x0001u
+ __le16 msg_type;
+ __le16 reserved;
+ __le32 flags;
+ __le32 can_id;
+ __u8 sdu[64u];
64u is CANFD_MAX_DLEN, right? Is it sensible to use that define instead?
I guess if CAN XL support is to be added at some point, a dedicated struct is
needed, to fit for CANXL_MAX_DLEN (2048 [1]).
[1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds%2flinux.git%2f&umid=127a2be7-331e-4823-805f-4578232f5495&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-52b59e6b756f1b97e14cc9439116d9b7a4ad93da
commit/?id=1a3e3034c049503ec6992a4a7d573e7fff31fac4
1.) This is CANFD_MAX_DLEN. It is not sensible to use that define
instead as this is a Linux define and this header here is not Linux
specific.
BTW, a microcontroller implementation supporting CAN classic only may
allocate only 8u bytes (CAN_MAX_DLEN) for the payload if memory usage
was an issue. However I doubt that very small controllers are an
audience for virtio.
The data structure has been updated in the meantime in the sources, spec
still to be sent. The newer structure should be prepared for CAN XL
later however CAN XL is currently not supported.
To add to Alexander's comment, what is the reason to have the msg_type
flag? The struct can_frame, canfd_frame and canxl_frame are done such
that it is feasible to decide the type (Classic, FD, XL) from the
content of the structure. Why not just reusing the current structures?
The message type costs 2 bytes, if additional alignment is needed it
causes a cost of 4 bytes in the worst case.
* Virtio uses shared memory to transfer those messages which is very
fast. From the speed perspective it costs almost nothing to have the
message type. It's not a slow serial line where every byte sent counts.
* The target machines for Virtio contain usually many megabytes if not
gigabytes. The additional message identifier plays no role for those
machines.
So the cost for the message type is relatively small. You may think
we're always transferring CAN messages on Rxq and Txq so we can save the
message type anyway. This is true for today and may be totally wrong
already in a few months.
Those queues are communication channels which can transport any
information. After the specification has been published some day someone
may want to extend the specification having to transfer a complete
different message over Txq or Rxq.
Easy with message type:
1. Add a feature flag to indicate that the device now also understands
msg_type FOO_NEW
2. Define the structure for FOO_NEW having __le_16 msg_type as first
member. The rest can be defined freely without any restrictions.
Without message type 2. can become ugly. The implementer is forced to
get into the existing scheme, this may be difficult and the result may
not look nice.
Better we spend the few bytes now to have better options in the future.
We don't know what is in 5 years or even next year.
===
The question "why not reusing exactly the existing Linux CAN structure"
is indeed something which has to be thought about. Cons: Virtio is not
directly a Linux specification. Pro: It's tempting. After I've learned
that qemu is also doing exactly this it becomes more and more tempting.
No conclusion today.
+};
+
+struct virtio_can_tx_in {
+ __u8 result;
+};
+
+/* RX queue message types */
+struct virtio_can_rx {
+#define VIRTIO_CAN_RX 0x0101u
+ __le16 msg_type;
+ __le16 reserved;
+ __le32 flags;
+ __le32 can_id;
+ __u8 sdu[64u];
+};
I have no experience with virtio drivers, but is there a need for dedicated
structs for Tx and Rx? They are identical anyway.
True for today. Could be optimized in the specification. Resulting
object code of the implementation would be the same anyway.
Best regards,
Alexander
+
+/* Control queue message types */
+struct virtio_can_control_out {
+#define VIRTIO_CAN_SET_CTRL_MODE_START 0x0201u
+#define VIRTIO_CAN_SET_CTRL_MODE_STOP 0x0202u
+ __le16 msg_type;
+};
+
+struct virtio_can_control_in {
+ __u8 result;
+};
+
+/* Indication queue message types */
+struct virtio_can_event_ind {
+#define VIRTIO_CAN_BUSOFF_IND 0x0301u
+ __le16 msg_type;
+};
+
+#endif /* #ifndef _LINUX_VIRTIO_VIRTIO_CAN_H */