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

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

[Harald Mommer]

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

Signed-off-by: Harald Mommer <hmo@opensynergy.com>
---
 drivers/net/can/Kconfig                 |    1 +
 drivers/net/can/Makefile                |    1 +
 drivers/net/can/virtio_can/Kconfig      |   12 +
 drivers/net/can/virtio_can/Makefile     |    5 +
 drivers/net/can/virtio_can/virtio_can.c | 1176 +++++++++++++++++++++++
 include/uapi/linux/virtio_can.h         |   69 ++
 6 files changed, 1264 insertions(+)
 create mode 100644 drivers/net/can/virtio_can/Kconfig
 create mode 100644 drivers/net/can/virtio_can/Makefile
 create mode 100644 drivers/net/can/virtio_can/virtio_can.c
 create mode 100644 include/uapi/linux/virtio_can.h

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 3048ad77edb3..432f5f15baaf 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -218,6 +218,7 @@ source "drivers/net/can/sja1000/Kconfig"
 source "drivers/net/can/softing/Kconfig"
 source "drivers/net/can/spi/Kconfig"
 source "drivers/net/can/usb/Kconfig"
+source "drivers/net/can/virtio_can/Kconfig"
 
 endif #CAN_NETLINK
 
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 61c75ce9d500..93cfd1b03a35 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -31,5 +31,6 @@ obj-$(CONFIG_CAN_SUN4I)		+= sun4i_can.o
 obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
 obj-$(CONFIG_CAN_XILINXCAN)	+= xilinx_can.o
 obj-$(CONFIG_PCH_CAN)		+= pch_can.o
+obj-$(CONFIG_CAN_VIRTIO_CAN)	+= virtio_can/
 
 subdir-ccflags-$(CONFIG_CAN_DEBUG_DEVICES) += -DDEBUG
diff --git a/drivers/net/can/virtio_can/Kconfig b/drivers/net/can/virtio_can/Kconfig
new file mode 100644
index 000000000000..ef8228fcc73e
--- /dev/null
+++ b/drivers/net/can/virtio_can/Kconfig
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only
+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.
diff --git a/drivers/net/can/virtio_can/Makefile b/drivers/net/can/virtio_can/Makefile
new file mode 100644
index 000000000000..c931e4b8c029
--- /dev/null
+++ b/drivers/net/can/virtio_can/Makefile
@@ -0,0 +1,5 @@
+#
+#  Makefile for the Virtio CAN controller driver.
+#
+
+obj-$(CONFIG_CAN_VIRTIO_CAN) += virtio_can.o
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
+ */
+
+#include <asm/atomic.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/netdevice.h>
+#include <linux/stddef.h>
+#include <linux/can/dev.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ring.h>
+#include <linux/virtio_can.h>
+
+/* CAN device queues */
+#define VIRTIO_CAN_QUEUE_TX 0u /* Driver side view! The device receives here */
+#define VIRTIO_CAN_QUEUE_RX 1u /* Driver side view! The device transmits here */
+#define VIRTIO_CAN_QUEUE_CONTROL 2u
+#define VIRTIO_CAN_QUEUE_INDICATION 3u
+#define VIRTIO_CAN_QUEUE_COUNT 4u
+
+#define CAN_KNOWN_FLAGS \
+	(VIRTIO_CAN_FLAGS_EXTENDED |\
+	 VIRTIO_CAN_FLAGS_FD |\
+	 VIRTIO_CAN_FLAGS_RTR)
+
+/* napi related */
+#define VIRTIO_CAN_NAPI_WEIGHT	NAPI_POLL_WEIGHT
+
+/* CAN TX message priorities (0 = normal priority) */
+#define VIRTIO_CAN_PRIO_COUNT 1
+/* Max. nummber of in flight TX messages */
+#define VIRTIO_CAN_ECHO_SKB_MAX 128u
+
+struct virtio_can_tx {
+	struct list_head list;
+	int prio; /* Currently always 0 "normal priority" */
+	int putidx;
+	struct virtio_can_tx_out tx_out;
+	struct virtio_can_tx_in tx_in;
+};
+
+/* virtio_can private data structure */
+struct virtio_can_priv {
+	struct can_priv can;	/* must be the first member */
+	/* NAPI for RX messages */
+	struct napi_struct napi;
+	/* NAPI for TX messages */
+	struct napi_struct napi_tx;
+	/* The network device we're associated with */
+	struct net_device *dev;
+	/* The virtio device we're associated with */
+	struct virtio_device *vdev;
+	/* The virtqueues */
+	struct virtqueue *vqs[VIRTIO_CAN_QUEUE_COUNT];
+	/* I/O callback function pointers for the virtqueues */
+	vq_callback_t *io_callbacks[VIRTIO_CAN_QUEUE_COUNT];
+	/* Lock for TX operations */
+	spinlock_t tx_lock;
+	/* Control queue lock. Defensive programming, may be not needed */
+	struct mutex ctrl_lock;
+	/* List of virtio CAN TX message */
+	struct list_head tx_list;
+	/* Array of receive queue messages */
+	struct virtio_can_rx rpkt[128u];
+	/* Those control queue messages cannot live on the stack! */
+	struct virtio_can_control_out cpkt_out;
+	struct virtio_can_control_in cpkt_in;
+	/* Indication queue message */
+	struct virtio_can_event_ind ipkt[1u];
+	/* Data to get and maintain the putidx for local TX echo */
+	struct list_head tx_putidx_free;
+	struct list_head *tx_putidx_list;
+	/* In flight TX messages per prio */
+	atomic_t tx_inflight[VIRTIO_CAN_PRIO_COUNT];
+	/* Max. In flight TX messages per prio */
+	u16 tx_limit[VIRTIO_CAN_PRIO_COUNT];
+	/* BusOff pending. Reset after successful indication to upper layer */
+	bool busoff_pending;
+};
+
+#ifdef DEBUG
+static void __attribute__((unused))
+virtio_can_hexdump(const void *data, size_t length, size_t base)
+{
+#define VIRTIO_CAN_MAX_BYTES_PER_LINE 16u
+	const char *cdata = data;
+	char *bp;
+	size_t offset = 0u;
+	unsigned int per_line = VIRTIO_CAN_MAX_BYTES_PER_LINE;
+	unsigned int col;
+	int c;
+	char buf[32u + 4u * VIRTIO_CAN_MAX_BYTES_PER_LINE];
+
+	do {
+		bp = &buf[0];
+		bp += sprintf(bp, "%zX:", base + offset);
+		if (length > 0u) {
+			if (per_line > length)
+				per_line = (unsigned int)length;
+			for (col = 0u; col < per_line; col++) {
+				c = cdata[offset + col] & 0xff;
+				bp += sprintf(bp, " %02X", c);
+			}
+			bp += sprintf(bp, " ");
+			for (col = 0u; col < per_line; col++) {
+				c = cdata[offset + col] & 0xff;
+				if (c < 32 || c >=  127)
+					c = '.';
+				bp += sprintf(bp, "%c", c);
+			}
+
+			offset += per_line;
+			length -= per_line;
+		}
+		pr_debug("%s\n", buf);
+	} while (length > 0u);
+}
+#else
+#define virtio_can_hexdump(data, length, base)
+#endif
+
+/* Function copied from virtio_net.c */
+static void virtqueue_napi_schedule(struct napi_struct *napi,
+				    struct virtqueue *vq)
+{
+	if (napi_schedule_prep(napi)) {
+		virtqueue_disable_cb(vq);
+		__napi_schedule(napi);
+	}
+}
+
+/* Function copied from virtio_net.c */
+static void virtqueue_napi_complete(struct napi_struct *napi,
+				    struct virtqueue *vq, int processed)
+{
+	int opaque;
+
+	opaque = virtqueue_enable_cb_prepare(vq);
+	if (napi_complete_done(napi, processed)) {
+		if (unlikely(virtqueue_poll(vq, opaque)))
+			virtqueue_napi_schedule(napi, vq);
+	} else {
+		virtqueue_disable_cb(vq);
+	}
+}
+
+static void virtio_can_free_candev(struct net_device *ndev)
+{
+	struct virtio_can_priv *priv = netdev_priv(ndev);
+
+	kfree(priv->tx_putidx_list);
+	free_candev(ndev);
+}
+
+static int virtio_can_alloc_tx_idx(struct virtio_can_priv *priv, int prio)
+{
+	struct list_head *entry;
+
+	BUG_ON(prio != 0); /* Currently only 1 priority */
+	BUG_ON(atomic_read(&priv->tx_inflight[0]) >= priv->can.echo_skb_max);
+	atomic_add(1, &priv->tx_inflight[prio]);
+
+	if (list_empty(&priv->tx_putidx_free))
+		return -1; /* Not expected to happen */
+
+	entry = priv->tx_putidx_free.next;
+	list_del(entry);
+
+	return entry - priv->tx_putidx_list;
+}
+
+static void virtio_can_free_tx_idx(struct virtio_can_priv *priv, int prio,
+				   int idx)
+{
+	BUG_ON(prio >= VIRTIO_CAN_PRIO_COUNT);
+	BUG_ON(idx >= priv->can.echo_skb_max);
+	BUG_ON(atomic_read(&priv->tx_inflight[prio]) == 0);
+
+	list_add(&priv->tx_putidx_list[idx], &priv->tx_putidx_free);
+	atomic_sub(1, &priv->tx_inflight[prio]);
+}
+
+/*
+ * Create a scatter-gather list representing our input buffer and put
+ * it in the queue.
+ *
+ * Callers should take appropriate locks.
+ */
+static int virtio_can_add_inbuf(struct virtqueue *vq, void *buf,
+				unsigned int size)
+{
+	struct scatterlist sg[1];
+	int ret;
+
+	sg_init_one(sg, buf, size);
+
+	ret = virtqueue_add_inbuf(vq, sg, 1, buf, GFP_ATOMIC);
+	virtqueue_kick(vq);
+	if (ret == 0)
+		ret = vq->num_free;
+	return ret;
+}
+
+/*
+ * Send a control message with message type either
+ *
+ * - VIRTIO_CAN_SET_CTRL_MODE_START or
+ * - VIRTIO_CAN_SET_CTRL_MODE_STOP.
+ *
+ * Unlike AUTOSAR CAN Driver Can_SetControllerMode() there is no requirement
+ * for this Linux driver to have an asynchronous implementation of the mode
+ * setting function so in order to keep things simple the function is
+ * implemented as synchronous function. Design pattern is
+ * virtio_console.c/__send_control_msg() & virtio_net.c/virtnet_send_command().
+ */
+static u8 virtio_can_send_ctrl_msg(struct net_device *ndev, u16 msg_type)
+{
+	struct virtio_can_priv *priv = netdev_priv(ndev);
+	struct device *dev = &priv->vdev->dev;
+	struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_CONTROL];
+	struct scatterlist sg_out[1u];
+	struct scatterlist sg_in[1u];
+	struct scatterlist *sgs[2u];
+	int err;
+	unsigned int len;
+
+	/*
+	 * The function may be serialized by rtnl lock. Not sure.
+	 * Better safe than sorry.
+	 */
+	mutex_lock(&priv->ctrl_lock);
+
+	priv->cpkt_out.msg_type = cpu_to_le16(msg_type);
+	sg_init_one(&sg_out[0], &priv->cpkt_out, sizeof(priv->cpkt_out));
+	sg_init_one(&sg_in[0], &priv->cpkt_in, sizeof(priv->cpkt_in));
+	sgs[0] = sg_out;
+	sgs[1] = sg_in;
+
+	err = virtqueue_add_sgs(vq, sgs, 1u, 1u, priv, GFP_ATOMIC);
+	if (err != 0) {
+		/* Not expected to happen */
+		dev_err(dev, "%s(): virtqueue_add_sgs() failed\n", __func__);
+	}
+
+	if (!virtqueue_kick(vq)) {
+		/* Not expected to happen */
+		dev_err(dev, "%s(): Kick failed\n", __func__);
+	}
+
+	while (!virtqueue_get_buf(vq, &len) && !virtqueue_is_broken(vq))
+		cpu_relax();
+
+	mutex_unlock(&priv->ctrl_lock);
+
+	return priv->cpkt_in.result;
+}
+
+static void virtio_can_start(struct net_device *ndev)
+{
+	struct virtio_can_priv *priv = netdev_priv(ndev);
+	u8 result;
+
+	result = virtio_can_send_ctrl_msg(ndev, VIRTIO_CAN_SET_CTRL_MODE_START);
+	if (result != VIRTIO_CAN_RESULT_OK) {
+		/* Not expected to happen */
+		netdev_err(ndev, "CAN controller start failed\n");
+	}
+
+	priv->busoff_pending = false;
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	/* Switch carrier on if device was not connected to the bus */
+	if (!netif_carrier_ok(ndev))
+		netif_carrier_on(ndev);
+}
+
+/*
+ * See also m_can.c/m_can_set_mode()
+ *
+ * It is interesting that not only the M-CAN implementation but also all other
+ * implementations I looked into only support CAN_MODE_START.
+ * That CAN_MODE_SLEEP is frequently not found to be supported anywhere did not
+ * come not as surprise but that CAN_MODE_STOP is also never supported was one.
+ * The function is accessible via the method pointer do_set_mode in
+ * struct can_priv. As ususal no documentation there.
+ * May not play any role as grepping through the code did not reveal any place
+ * from where the method is actually called.
+ */
+static int virtio_can_set_mode(struct net_device *dev, enum can_mode mode)
+{
+	switch (mode) {
+	case CAN_MODE_START:
+		virtio_can_start(dev);
+		netif_wake_queue(dev);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+/*
+ * Called by issuing "ip link set up can0"
+ */
+static int virtio_can_open(struct net_device *dev)
+{
+	/* start the virtio_can controller */
+	virtio_can_start(dev);
+
+	/* RX and TX napi were already enabled in virtio_can_probe() */
+	netif_start_queue(dev);
+
+	return 0;
+}
+
+static void virtio_can_stop(struct net_device *ndev)
+{
+	struct virtio_can_priv *priv = netdev_priv(ndev);
+	struct device *dev = &priv->vdev->dev;
+	u8 result;
+
+	result = virtio_can_send_ctrl_msg(ndev, VIRTIO_CAN_SET_CTRL_MODE_STOP);
+	if (result != VIRTIO_CAN_RESULT_OK)
+		dev_err(dev, "CAN controller stop failed\n");
+
+	priv->busoff_pending = false;
+	priv->can.state = CAN_STATE_STOPPED;
+
+	/* Switch carrier off if device was connected to the bus */
+	if (netif_carrier_ok(ndev))
+		netif_carrier_off(ndev);
+}
+
+static int virtio_can_close(struct net_device *dev)
+{
+	netif_stop_queue(dev);
+	/*
+	 * Keep RX napi active to allow dropping of pending RX CAN messages,
+	 * keep TX napi active to allow processing of cancelled CAN messages
+	 */
+	virtio_can_stop(dev);
+	close_candev(dev);
+
+	return 0;
+}
+
+static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
+					 struct net_device *dev)
+{
+	struct virtio_can_priv *priv = netdev_priv(dev);
+	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+	struct virtio_can_tx *can_tx_msg;
+	struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
+	struct scatterlist sg_out[1u];
+	struct scatterlist sg_in[1u];
+	struct scatterlist *sgs[2u];
+	unsigned long flags;
+	size_t len;
+	u32 can_flags;
+	int err;
+	const unsigned int hdr_size = offsetof(struct virtio_can_tx_out, sdu);
+
+	/* virtio_can_hexdump(cf, hdr_size + cf->len, 0u); */
+
+	if (can_dropped_invalid_skb(dev, skb))
+		return NETDEV_TX_OK; /* No way to return NET_XMIT_DROP here */
+
+	/* Virtio CAN does not support error message frames */
+	if ((cf->can_id & CAN_ERR_FLAG) != 0u) {
+		kfree_skb(skb);
+		dev->stats.tx_dropped++;
+		return NETDEV_TX_OK; /* No way to return NET_XMIT_DROP here */
+	}
+
+	/*
+	 * No local check for CAN_RTR_FLAG or FD frame against negotiated
+	 * features. The device will reject those anyway if not supported.
+	 */
+
+	can_tx_msg = kzalloc(sizeof(*can_tx_msg), GFP_ATOMIC);
+	if (!can_tx_msg)
+		return NETDEV_TX_OK; /* No way to return NET_XMIT_DROP here */
+
+	can_tx_msg->tx_out.msg_type = cpu_to_le16(VIRTIO_CAN_TX);
+	can_flags = 0u;
+	if ((cf->can_id & CAN_EFF_FLAG) != 0u)
+		can_flags |= VIRTIO_CAN_FLAGS_EXTENDED;
+	if ((cf->can_id & CAN_RTR_FLAG) != 0u)
+		can_flags |= VIRTIO_CAN_FLAGS_RTR;
+	if (can_is_canfd_skb(skb))
+		can_flags |= VIRTIO_CAN_FLAGS_FD;
+	can_tx_msg->tx_out.flags = cpu_to_le32(can_flags);
+	can_tx_msg->tx_out.can_id = cpu_to_le32(cf->can_id & CAN_EFF_MASK);
+	len = cf->len;
+	if (len > sizeof(cf->data))
+		len = sizeof(cf->data);
+	if (len > sizeof(can_tx_msg->tx_out.sdu))
+		len = sizeof(can_tx_msg->tx_out.sdu);
+	if ((can_flags & VIRTIO_CAN_FLAGS_RTR) == 0u) {
+		/* Copy if no RTR frame. RTR frames have a DLC but no payload */
+		memcpy(can_tx_msg->tx_out.sdu, cf->data, len);
+	}
+
+	/* Prepare sending of virtio message */
+	sg_init_one(&sg_out[0], &can_tx_msg->tx_out, hdr_size + len);
+	sg_init_one(&sg_in[0], &can_tx_msg->tx_in, sizeof(can_tx_msg->tx_in));
+	sgs[0] = sg_out;
+	sgs[1] = sg_in;
+
+	/* Protect list and virtio queue operations */
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
+	/* Find free TX priority */
+	if (atomic_read(&priv->tx_inflight[0]) >= priv->tx_limit[0]) {
+		/*
+		 * May happen if
+		 * - tx_limit[0] > max # of TX queue messages
+		 */
+		netif_stop_queue(dev);
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+		kfree(can_tx_msg);
+		netdev_info(dev, "TX: Stop queue, all prios full\n");
+		return NETDEV_TX_BUSY;
+	}
+
+	can_tx_msg->prio = 0; /* Priority is always 0 "normal priority" */
+	can_tx_msg->putidx = virtio_can_alloc_tx_idx(priv, can_tx_msg->prio);
+	BUG_ON(can_tx_msg->putidx < 0);
+
+	/* Normal queue stop when no transmission slots are left */
+	if (atomic_read(&priv->tx_inflight[0]) >= priv->tx_limit[0]) {
+		netif_stop_queue(dev);
+		netdev_info(dev, "TX: Normal stop queue\n");
+	}
+
+	list_add_tail(&can_tx_msg->list, &priv->tx_list);
+
+	/* Push loopback echo. Will be looped back on TX interrupt/TX NAPI */
+	can_put_echo_skb(skb, dev, can_tx_msg->putidx, 0);
+
+	err = virtqueue_add_sgs(vq, sgs, 1u, 1u, can_tx_msg, GFP_ATOMIC);
+	if (err != 0) {
+		list_del(&can_tx_msg->list);
+		virtio_can_free_tx_idx(priv, can_tx_msg->prio,
+				       can_tx_msg->putidx);
+		netif_stop_queue(dev);
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+		kfree(can_tx_msg);
+		if (err == -ENOSPC)
+			netdev_info(dev, "TX: Stop queue, no space left\n");
+		else
+			netdev_warn(dev, "TX: Stop queue, reason = %d\n", err);
+		return NETDEV_TX_BUSY;
+	}
+
+	if (!virtqueue_kick(vq))
+		netdev_err(dev, "%s(): Kick failed\n", __func__);
+
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+	return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops virtio_can_netdev_ops = {
+	.ndo_open = virtio_can_open,
+	.ndo_stop = virtio_can_close,
+	.ndo_start_xmit = virtio_can_start_xmit,
+	.ndo_change_mtu = can_change_mtu,
+};
+
+static int register_virtio_can_dev(struct net_device *dev)
+{
+	dev->flags |= IFF_ECHO;	/* we support local echo */
+	dev->netdev_ops = &virtio_can_netdev_ops;
+
+	return register_candev(dev);
+}
+
+/* Compare with m_can.c/m_can_echo_tx_event() */
+static int virtio_can_read_tx_queue(struct virtqueue *vq)
+{
+	struct virtio_can_priv *can_priv = vq->vdev->priv;
+	struct net_device *dev = can_priv->dev;
+	struct net_device_stats *stats = &dev->stats;
+	struct virtio_can_tx *can_tx_msg;
+	unsigned long flags;
+	unsigned int len;
+	u8 result;
+
+	/* Protect list and virtio queue operations */
+	spin_lock_irqsave(&can_priv->tx_lock, flags);
+
+	can_tx_msg = virtqueue_get_buf(vq, &len);
+	if (!can_tx_msg) {
+		spin_unlock_irqrestore(&can_priv->tx_lock, flags);
+		return 0; /* No more data */
+	}
+
+	if (unlikely(len < sizeof(struct virtio_can_tx_in))) {
+		netdev_err(dev, "TX ACK: Device sent no result code\n");
+		result = VIRTIO_CAN_RESULT_NOT_OK; /* Keep things going */
+	} else {
+		result = can_tx_msg->tx_in.result;
+	}
+
+	if (can_priv->can.state < CAN_STATE_BUS_OFF) {
+		/*
+		 * Here also frames with result != VIRTIO_CAN_RESULT_OK are
+		 * echoed. Intentional to bring a waiting process in an upper
+		 * layer to an end.
+		 * TODO: Any better means to indicate a problem here?
+		 */
+		if (result != VIRTIO_CAN_RESULT_OK)
+			netdev_warn(dev, "TX ACK: Result = %u\n", result);
+
+		stats->tx_bytes += can_get_echo_skb(dev, can_tx_msg->putidx,
+						    NULL);
+		stats->tx_packets++;
+	} else {
+		netdev_dbg(dev, "TX ACK: Controller inactive, drop echo\n");
+		can_free_echo_skb(dev, can_tx_msg->putidx, NULL);
+	}
+
+	list_del(&can_tx_msg->list);
+	virtio_can_free_tx_idx(can_priv, can_tx_msg->prio, can_tx_msg->putidx);
+
+	spin_unlock_irqrestore(&can_priv->tx_lock, flags);
+
+	kfree(can_tx_msg);
+
+	/* Flow control */
+	if (netif_queue_stopped(dev)) {
+		netdev_info(dev, "TX ACK: Wake up stopped queue\n");
+		netif_wake_queue(dev);
+	}
+
+	return 1; /* Queue was not emtpy so there may be more data */
+}
+
+/*
+ * Poll TX used queue for sent CAN messages
+ * See https://wiki.linuxfoundation.org/networking/napi function
+ * int (*poll)(struct napi_struct *napi, int budget);
+ */
+static int virtio_can_tx_poll(struct napi_struct *napi, int quota)
+{
+	struct net_device *dev = napi->dev;
+	struct virtio_can_priv *priv = netdev_priv(dev);
+	struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
+	int work_done = 0;
+
+	while (work_done < quota && virtio_can_read_tx_queue(vq) != 0)
+		work_done++;
+
+	if (work_done < quota)
+		virtqueue_napi_complete(napi, vq, work_done);
+
+	return work_done;
+}
+
+static void virtio_can_tx_intr(struct virtqueue *vq)
+{
+	struct virtio_can_priv *can_priv = vq->vdev->priv;
+
+	virtqueue_disable_cb(vq);
+	napi_schedule(&can_priv->napi_tx);
+}
+
+/*
+ * This function is the NAPI RX poll function and NAPI guarantees that this
+ * function is not invoked simulataneously on multiply processors.
+ * Read a RX message from the used queue and sends it to the upper layer.
+ * (See also m_can.c / m_can_read_fifo()).
+ */
+static int virtio_can_read_rx_queue(struct virtqueue *vq)
+{
+	struct virtio_can_priv *priv = vq->vdev->priv;
+	struct net_device *dev = priv->dev;
+	struct net_device_stats *stats = &dev->stats;
+	struct virtio_can_rx *can_rx;
+	struct canfd_frame *cf;
+	struct sk_buff *skb;
+	unsigned int len;
+	const unsigned int header_size = offsetof(struct virtio_can_rx, sdu);
+	u16 msg_type;
+	u32 can_flags;
+	u32 can_id;
+
+	can_rx = virtqueue_get_buf(vq, &len);
+	if (!can_rx)
+		return 0; /* No more data */
+
+	BUG_ON(len < header_size);
+
+	/* virtio_can_hexdump(can_rx, len, 0u); */
+
+	if (priv->can.state >= CAN_STATE_ERROR_PASSIVE) {
+		netdev_dbg(dev, "%s(): Controller not active\n", __func__);
+		goto putback;
+	}
+
+	msg_type = le16_to_cpu(can_rx->msg_type);
+	if (msg_type != VIRTIO_CAN_RX) {
+		netdev_warn(dev, "RX: Got unknown msg_type %04x\n", msg_type);
+		goto putback;
+	}
+
+	len -= header_size; /* Payload only now */
+	can_flags = le32_to_cpu(can_rx->flags);
+	can_id = le32_to_cpu(can_rx->can_id);
+
+	if ((can_flags & ~CAN_KNOWN_FLAGS) != 0u) {
+		stats->rx_dropped++;
+		netdev_warn(dev, "RX: CAN Id 0x%08x: Invalid flags 0x%x\n",
+			    can_id, can_flags);
+		goto putback;
+	}
+
+	if ((can_flags & VIRTIO_CAN_FLAGS_EXTENDED) != 0u) {
+		if (can_id > CAN_EFF_MASK) {
+			stats->rx_dropped++;
+			netdev_warn(dev, "RX: CAN Ext Id 0x%08x too big\n",
+				    can_id);
+			goto putback;
+		}
+		can_id |= CAN_EFF_FLAG;
+	} else {
+		if (can_id > CAN_SFF_MASK) {
+			stats->rx_dropped++;
+			netdev_warn(dev, "RX: CAN Std Id 0x%08x too big\n",
+				    can_id);
+			goto putback;
+		}
+	}
+
+	if ((can_flags & VIRTIO_CAN_FLAGS_RTR) != 0u) {
+		if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_RTR_FRAMES)) {
+			stats->rx_dropped++;
+			netdev_warn(dev, "RX: CAN Id 0x%08x: RTR not negotiated\n",
+				    can_id);
+			goto putback;
+		}
+		if ((can_flags & VIRTIO_CAN_FLAGS_FD) != 0u) {
+			stats->rx_dropped++;
+			netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with FD not possible\n",
+				    can_id);
+			goto putback;
+		}
+		if (len != 0u) {
+			stats->rx_dropped++;
+			netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with len != 0\n",
+				    can_id);
+			goto putback;
+		}
+		can_id |= CAN_RTR_FLAG;
+	}
+
+	if ((can_flags & VIRTIO_CAN_FLAGS_FD) != 0u) {
+		if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_CAN_FD)) {
+			stats->rx_dropped++;
+			netdev_warn(dev, "RX: CAN Id 0x%08x: FD not negotiated\n",
+				    can_id);
+			goto putback;
+		}
+
+		skb = alloc_canfd_skb(priv->dev, &cf);
+		if (len > CANFD_MAX_DLEN)
+			len = CANFD_MAX_DLEN;
+	} else {
+		if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_CAN_CLASSIC)) {
+			stats->rx_dropped++;
+			netdev_warn(dev, "RX: CAN Id 0x%08x: classic not negotiated\n",
+				    can_id);
+			goto putback;
+		}
+
+		skb = alloc_can_skb(priv->dev, (struct can_frame **)&cf);
+		if (len > CAN_MAX_DLEN)
+			len = CAN_MAX_DLEN;
+	}
+	if (!skb) {
+		stats->rx_dropped++;
+		netdev_warn(dev, "RX: No skb available\n");
+		goto putback;
+	}
+
+	cf->can_id = can_id;
+	cf->len = len;
+	if ((can_flags & VIRTIO_CAN_FLAGS_RTR) == 0u) {
+		/* Copy if no RTR frame. RTR frames have a DLC but no payload */
+		memcpy(cf->data, can_rx->sdu, len);
+	}
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->len;
+
+	/* Use netif_rx() for interrupt context */
+	(void)netif_receive_skb(skb);
+
+putback:
+	/* Put processed RX buffer back into avail queue */
+	(void)virtio_can_add_inbuf(vq, can_rx, sizeof(struct virtio_can_rx));
+
+	return 1; /* Queue was not emtpy so there may be more data */
+}
+
+/*
+ * See m_can_poll() / m_can_handle_state_errors() m_can_handle_state_change().
+ */
+static int virtio_can_handle_busoff(struct net_device *dev)
+{
+	struct virtio_can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	u8 rx_bytes;
+
+	if (!priv->busoff_pending)
+		return 0;
+
+	if (priv->can.state < CAN_STATE_BUS_OFF) {
+		netdev_dbg(dev, "entered error bus off state\n");
+
+		/* bus-off state */
+		priv->can.state = CAN_STATE_BUS_OFF;
+		priv->can.can_stats.bus_off++;
+		can_bus_off(dev);
+	}
+
+	/* propagate the error condition to the CAN stack */
+	skb = alloc_can_err_skb(dev, &cf);
+	if (unlikely(!skb))
+		return 0;
+
+	/* bus-off state */
+	cf->can_id |= CAN_ERR_BUSOFF;
+	rx_bytes = cf->can_dlc;
+
+	/* Ensure that the BusOff indication does not get lost */
+	if (netif_receive_skb(skb) == NET_RX_SUCCESS)
+		priv->busoff_pending = false;
+
+	stats->rx_packets++;
+	stats->rx_bytes += rx_bytes;
+
+	return 1;
+}
+
+/*
+ * Poll RX used queue for received CAN messages
+ * See https://wiki.linuxfoundation.org/networking/napi function
+ * int (*poll)(struct napi_struct *napi, int budget);
+ * Important: "The networking subsystem promises that poll() will not be
+ * invoked simultaneously (for the same napi_struct) on multiple processors"
+ */
+static int virtio_can_rx_poll(struct napi_struct *napi, int quota)
+{
+	struct net_device *dev = napi->dev;
+	struct virtio_can_priv *priv = netdev_priv(dev);
+	struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_RX];
+	int work_done = 0;
+
+	work_done += virtio_can_handle_busoff(dev);
+
+	while (work_done < quota && virtio_can_read_rx_queue(vq) != 0)
+		work_done++;
+
+	if (work_done < quota)
+		virtqueue_napi_complete(napi, vq, work_done);
+
+	return work_done;
+}
+
+static void virtio_can_rx_intr(struct virtqueue *vq)
+{
+	struct virtio_can_priv *can_priv = vq->vdev->priv;
+
+	virtqueue_disable_cb(vq);
+	napi_schedule(&can_priv->napi);
+}
+
+static void virtio_can_evind_intr(struct virtqueue *vq)
+{
+	struct virtio_can_priv *can_priv = vq->vdev->priv;
+	struct net_device *dev = can_priv->dev;
+	struct virtio_can_event_ind *evind;
+	unsigned int len;
+	u16 msg_type;
+
+	for (;;) {
+		/*
+		 * The interrupt function is not assumed to be interrupted by
+		 * itself so locks should not be needed for queue operations.
+		 */
+		evind = virtqueue_get_buf(vq, &len);
+		if (!evind)
+			return; /* No more messages */
+
+		BUG_ON(len < sizeof(struct virtio_can_event_ind));
+
+		msg_type = le16_to_cpu(evind->msg_type);
+		if (msg_type != VIRTIO_CAN_BUSOFF_IND) {
+			netdev_warn(dev, "Evind: Got unknown msg_type %04x\n",
+				    msg_type);
+			goto putback;
+		}
+
+		if (!can_priv->busoff_pending &&
+		    can_priv->can.state < CAN_STATE_BUS_OFF) {
+			can_priv->busoff_pending = true;
+			napi_schedule(&can_priv->napi);
+		}
+
+putback:
+		/* Put processed event ind uffer back into avail queue */
+		(void)virtio_can_add_inbuf(vq, evind,
+					   sizeof(struct virtio_can_event_ind));
+	}
+}
+
+static void virtio_can_populate_vqs(struct virtio_device *vdev)
+
+{
+	struct virtio_can_priv *priv = vdev->priv;
+	struct virtqueue *vq;
+	unsigned int idx;
+	int ret;
+
+	// TODO: Think again a moment if here locks already may be needed!
+
+	/* Fill RX queue */
+	vq = priv->vqs[VIRTIO_CAN_QUEUE_RX];
+	for (idx = 0u; idx < ARRAY_SIZE(priv->rpkt); idx++) {
+		ret = virtio_can_add_inbuf(vq, &priv->rpkt[idx],
+					   sizeof(struct virtio_can_rx));
+		if (ret < 0) {
+			dev_dbg(&vdev->dev, "rpkt fill: ret=%d, idx=%u\n",
+				ret, idx);
+			break;
+		}
+	}
+	dev_dbg(&vdev->dev, "%u rpkt added\n", idx);
+
+	/* Fill event indication queue */
+	vq = priv->vqs[VIRTIO_CAN_QUEUE_INDICATION];
+	for (idx = 0u; idx < ARRAY_SIZE(priv->ipkt); idx++) {
+		ret = virtio_can_add_inbuf(vq, &priv->ipkt[idx],
+					   sizeof(struct virtio_can_event_ind));
+		if (ret < 0) {
+			dev_dbg(&vdev->dev, "ipkt fill: ret=%d, idx=%u\n",
+				ret, idx);
+			break;
+		}
+	}
+	dev_dbg(&vdev->dev, "%u ipkt added\n", idx);
+}
+
+static int virtio_can_find_vqs(struct virtio_can_priv *priv)
+{
+	/*
+	 * The order of RX and TX is exactly the opposite as in console and
+	 * network. Does not play any role but is a bad trap.
+	 */
+	static const char * const io_names[VIRTIO_CAN_QUEUE_COUNT] = {
+		"can-tx",
+		"can-rx",
+		"can-state-ctrl",
+		"can-event-ind"
+	};
+
+	BUG_ON(!priv);
+	BUG_ON(!priv->vdev);
+
+	priv->io_callbacks[VIRTIO_CAN_QUEUE_TX] = virtio_can_tx_intr;
+	priv->io_callbacks[VIRTIO_CAN_QUEUE_RX] = virtio_can_rx_intr;
+	priv->io_callbacks[VIRTIO_CAN_QUEUE_CONTROL] = NULL;
+	priv->io_callbacks[VIRTIO_CAN_QUEUE_INDICATION] = virtio_can_evind_intr;
+
+	/* Find the queues. */
+	return virtio_find_vqs(priv->vdev, VIRTIO_CAN_QUEUE_COUNT, priv->vqs,
+			       priv->io_callbacks, io_names, NULL);
+}
+
+/* Function must not be called before virtio_can_find_vqs() has been run */
+static int virtio_can_del_vq(struct virtio_device *vdev)
+{
+	struct virtio_can_priv *priv = vdev->priv;
+	struct list_head *cursor, *next;
+	struct virtqueue *vq;
+
+	if (!priv)
+		return -EINVAL;
+
+	/* Reset the device */
+	if (vdev->config->reset)
+		vdev->config->reset(vdev);
+
+	/*
+	 * From here we have dead silence from the device side so no locks
+	 * are needed to protect against device side events.
+	 */
+
+	vq = priv->vqs[VIRTIO_CAN_QUEUE_INDICATION];
+	while (virtqueue_detach_unused_buf(vq))
+		; /* Do nothing, content allocated statically */
+
+	vq = priv->vqs[VIRTIO_CAN_QUEUE_CONTROL];
+	while (virtqueue_detach_unused_buf(vq))
+		; /* Do nothing, content allocated statically */
+
+	vq = priv->vqs[VIRTIO_CAN_QUEUE_RX];
+	while (virtqueue_detach_unused_buf(vq))
+		; /* Do nothing, content allocated statically */
+
+	vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
+	while (virtqueue_detach_unused_buf(vq))
+		; /* Do nothing, content to be de-allocated separately */
+
+	/*
+	 * Is keeping track of allocated elements by an own linked list
+	 * really necessary or may this be optimized using only
+	 * virtqueue_detach_unused_buf()?
+	 */
+	list_for_each_safe(cursor, next, &priv->tx_list) {
+		struct virtio_can_tx *can_tx;
+
+		can_tx = list_entry(cursor, struct virtio_can_tx, list);
+		list_del(cursor);
+		kfree(can_tx);
+	}
+
+	if (vdev->config->del_vqs)
+		vdev->config->del_vqs(vdev);
+
+	return 0;
+}
+
+/* See virtio_net.c/virtnet_remove() and also m_can.c/m_can_plat_remove() */
+static void virtio_can_remove(struct virtio_device *vdev)
+{
+	struct virtio_can_priv *priv = vdev->priv;
+	struct net_device *dev = priv->dev;
+
+	unregister_candev(dev);
+
+	/* No calls of netif_napi_del() needed as free_candev() will do this */
+
+	(void)virtio_can_del_vq(vdev);
+
+	virtio_can_free_candev(dev);
+}
+
+static int virtio_can_validate(struct virtio_device *vdev)
+{
+	BUG_ON(!vdev);
+
+	/*
+	 * CAN needs always access to the config space.
+	 * Check that the driver can access the config space
+	 */
+	if (!vdev->config->get) {
+		dev_err(&vdev->dev, "%s failure: config access disabled\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+		dev_err(&vdev->dev,
+			"device does not comply with spec version 1.x\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int virtio_can_probe(struct virtio_device *vdev)
+{
+	struct net_device *dev;
+	struct virtio_can_priv *priv;
+	int err;
+	unsigned int echo_skb_max;
+	unsigned int idx;
+	u16 lo_tx = VIRTIO_CAN_ECHO_SKB_MAX;
+
+	BUG_ON(!vdev);
+
+	echo_skb_max = lo_tx;
+	dev = alloc_candev(sizeof(struct virtio_can_priv), echo_skb_max);
+	if (!dev)
+		return -ENOMEM;
+
+	priv = netdev_priv(dev);
+
+	dev_info(&vdev->dev, "echo_skb_max = %u\n", priv->can.echo_skb_max);
+	priv->tx_putidx_list =
+		kcalloc(echo_skb_max, sizeof(struct list_head), GFP_KERNEL);
+	if (!priv->tx_putidx_list) {
+		free_candev(dev);
+		return -ENOMEM;
+	}
+
+	INIT_LIST_HEAD(&priv->tx_putidx_free);
+	for (idx = 0u; idx < echo_skb_max; idx++)
+		list_add_tail(&priv->tx_putidx_list[idx],
+			      &priv->tx_putidx_free);
+
+	netif_napi_add(dev, &priv->napi, virtio_can_rx_poll,
+		       VIRTIO_CAN_NAPI_WEIGHT);
+	netif_napi_add(dev, &priv->napi_tx, virtio_can_tx_poll,
+		       VIRTIO_CAN_NAPI_WEIGHT);
+
+	SET_NETDEV_DEV(dev, &vdev->dev);
+
+	priv->dev = dev;
+	priv->vdev = vdev;
+	vdev->priv = priv;
+
+	priv->can.do_set_mode = virtio_can_set_mode;
+	priv->can.state = CAN_STATE_STOPPED;
+	/* Set Virtio CAN supported operations */
+	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
+	if (virtio_has_feature(vdev, VIRTIO_CAN_F_CAN_FD)) {
+		dev_info(&vdev->dev, "CAN FD is supported\n");
+		err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD);
+		if (err != 0)
+			goto on_failure;
+	} else {
+		dev_info(&vdev->dev, "CAN FD not supported\n");
+	}
+
+	register_virtio_can_dev(dev);
+
+	/* Initialize virtqueues */
+	err = virtio_can_find_vqs(priv);
+	if (err != 0)
+		goto on_failure;
+
+	/*
+	 * It is possible to consider the number of TX queue places to
+	 * introduce a stricter TX flow control. Question is if this should
+	 * be done permanently this way in the Linux virtio CAN driver.
+	 */
+	if (true) {
+		struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
+		unsigned int tx_slots = vq->num_free;
+
+		if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
+			tx_slots >>= 1;
+		if (lo_tx > tx_slots)
+			lo_tx = tx_slots;
+	}
+
+	dev_info(&vdev->dev, "tx_limit[0] = %u\n", lo_tx);
+	priv->tx_limit[0] = lo_tx;
+
+	INIT_LIST_HEAD(&priv->tx_list);
+
+	spin_lock_init(&priv->tx_lock);
+	mutex_init(&priv->ctrl_lock);
+
+	virtio_can_populate_vqs(vdev);
+
+	napi_enable(&priv->napi);
+	napi_enable(&priv->napi_tx);
+
+	/* Request device going live */
+	virtio_device_ready(vdev); /* Optionally done by virtio_dev_probe() */
+	dev_info(&vdev->dev, "Device live!\n");
+
+	return 0;
+
+on_failure:
+	unregister_candev(dev);
+	virtio_can_free_candev(dev);
+	return err;
+}
+
+#ifdef CONFIG_PM_SLEEP
+/*
+ * Compare with m_can.c/m_can_suspend(), virtio_net.c/virtnet_freeze() and
+ * virtio_card.c/virtsnd_freeze()
+ */
+static int virtio_can_freeze(struct virtio_device *vdev)
+{
+	struct virtio_can_priv *priv = vdev->priv;
+	struct net_device *ndev = priv->dev;
+
+	napi_disable(&priv->napi);
+	napi_disable(&priv->napi_tx);
+
+	if (netif_running(ndev)) {
+		netif_stop_queue(ndev);
+		netif_device_detach(ndev);
+		virtio_can_stop(ndev);
+	}
+
+	priv->can.state = CAN_STATE_SLEEPING;
+
+	(void)virtio_can_del_vq(vdev);
+
+	return 0;
+}
+
+/*
+ * Compare with m_can.c/m_can_resume(), virtio_net.c/virtnet_restore() and
+ * virtio_card.c/virtsnd_restore()
+ */
+static int virtio_can_restore(struct virtio_device *vdev)
+{
+	struct virtio_can_priv *priv = vdev->priv;
+	struct net_device *ndev = priv->dev;
+	int err;
+
+	err = virtio_can_find_vqs(priv);
+	if (err != 0)
+		return err;
+	virtio_can_populate_vqs(vdev);
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	if (netif_running(ndev)) {
+		virtio_can_start(ndev);
+		netif_device_attach(ndev);
+		netif_start_queue(ndev);
+	}
+
+	napi_enable(&priv->napi);
+	napi_enable(&priv->napi_tx);
+
+	return 0;
+}
+#endif /* #ifdef CONFIG_PM_SLEEP */
+
+static struct virtio_device_id virtio_can_id_table[] = {
+	{ VIRTIO_ID_CAN, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static unsigned int features[] = {
+	VIRTIO_CAN_F_CAN_CLASSIC,
+	VIRTIO_CAN_F_CAN_FD,
+	VIRTIO_CAN_F_LATE_TX_ACK,
+	VIRTIO_CAN_F_RTR_FRAMES,
+};
+
+static struct virtio_driver virtio_can_driver = {
+	.feature_table = features,
+	.feature_table_size = ARRAY_SIZE(features),
+	.feature_table_legacy = NULL,
+	.feature_table_size_legacy = 0u,
+	.driver.name =	KBUILD_MODNAME,
+	.driver.owner =	THIS_MODULE,
+	.id_table =	virtio_can_id_table,
+	.validate =	virtio_can_validate,
+	.probe =	virtio_can_probe,
+	.remove =	virtio_can_remove,
+	.config_changed = NULL,
+#ifdef CONFIG_PM_SLEEP
+	.freeze =	virtio_can_freeze,
+	.restore =	virtio_can_restore,
+#endif
+};
+
+module_virtio_driver(virtio_can_driver);
+MODULE_DEVICE_TABLE(virtio, virtio_can_id_table);
+
+MODULE_AUTHOR("OpenSynergy GmbH");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CAN bus driver for Virtio CAN controller");
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];
+};
+
+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];
+};
+
+/* 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 */
-- 
2.17.1

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

[Arnd Bergmann]

() b
On Thu, Aug 25, 2022 at 3:44 PM Harald Mommer
<harald.mommer@opensynergy.com> 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.
>
> Signed-off-by: Harald Mommer <hmo@opensynergy.com>

This looks nice overall, but as you say there is still some work needed in all
the details. I've done a rough first pass at reviewing it, but I have
no specific
understanding of CAN, so these are mostly generic comments about
coding style or network drivers.

>  drivers/net/can/Kconfig                 |    1 +
>  drivers/net/can/Makefile                |    1 +
>  drivers/net/can/virtio_can/Kconfig      |   12 +
>  drivers/net/can/virtio_can/Makefile     |    5 +
>  drivers/net/can/virtio_can/virtio_can.c | 1176 +++++++++++++++++++++++
>  include/uapi/linux/virtio_can.h         |   69 ++

Since the driver is just one file, you probably don't need the subdirectory.

> +struct virtio_can_tx {
> +       struct list_head list;
> +       int prio; /* Currently always 0 "normal priority" */
> +       int putidx;
> +       struct virtio_can_tx_out tx_out;
> +       struct virtio_can_tx_in tx_in;
> +};

Having a linked list of these appears to add a little extra complexity.
If they are always processed in sequence, using an array would be
much simpler, as you just need to remember the index.

> +#ifdef DEBUG
> +static void __attribute__((unused))
> +virtio_can_hexdump(const void *data, size_t length, size_t base)
> +{
> +#define VIRTIO_CAN_MAX_BYTES_PER_LINE 16u

This seems to duplicate print_hex_dump(), maybe just use that?

> +
> +       while (!virtqueue_get_buf(vq, &len) && !virtqueue_is_broken(vq))
> +               cpu_relax();
> +
> +       mutex_unlock(&priv->ctrl_lock);

A busy loop is probably not what you want here. Maybe just
wait_for_completion() until the callback happens?

> +       /* Push loopback echo. Will be looped back on TX interrupt/TX NAPI */
> +       can_put_echo_skb(skb, dev, can_tx_msg->putidx, 0);
> +
> +       err = virtqueue_add_sgs(vq, sgs, 1u, 1u, can_tx_msg, GFP_ATOMIC);
> +       if (err != 0) {
> +               list_del(&can_tx_msg->list);
> +               virtio_can_free_tx_idx(priv, can_tx_msg->prio,
> +                                      can_tx_msg->putidx);
> +               netif_stop_queue(dev);
> +               spin_unlock_irqrestore(&priv->tx_lock, flags);
> +               kfree(can_tx_msg);
> +               if (err == -ENOSPC)
> +                       netdev_info(dev, "TX: Stop queue, no space left\n");
> +               else
> +                       netdev_warn(dev, "TX: Stop queue, reason = %d\n", err);
> +               return NETDEV_TX_BUSY;
> +       }
> +
> +       if (!virtqueue_kick(vq))
> +               netdev_err(dev, "%s(): Kick failed\n", __func__);
> +
> +       spin_unlock_irqrestore(&priv->tx_lock, flags);

There should not be a need for a spinlock or disabling interrupts
in the xmit function. What exactly are you protecting against here?

As a further optimization, you may want to use the xmit_more()
function, as the virtqueue kick is fairly expensive and can be
batched here.

> +       kfree(can_tx_msg);
> +
> +       /* Flow control */
> +       if (netif_queue_stopped(dev)) {
> +               netdev_info(dev, "TX ACK: Wake up stopped queue\n");
> +               netif_wake_queue(dev);
> +       }

You may want to add netdev_sent_queue()/netdev_completed_queue()
based BQL flow control here as well, so you don't have to rely on the
queue filling up completely.

> +static int virtio_can_probe(struct virtio_device *vdev)
> +{
> +       struct net_device *dev;
> +       struct virtio_can_priv *priv;
> +       int err;
> +       unsigned int echo_skb_max;
> +       unsigned int idx;
> +       u16 lo_tx = VIRTIO_CAN_ECHO_SKB_MAX;
> +
> +       BUG_ON(!vdev);

Not a useful debug check, just remove the BUG_ON(!vdev), here and elsewhere

> +
> +       echo_skb_max = lo_tx;
> +       dev = alloc_candev(sizeof(struct virtio_can_priv), echo_skb_max);
> +       if (!dev)
> +               return -ENOMEM;
> +
> +       priv = netdev_priv(dev);
> +
> +       dev_info(&vdev->dev, "echo_skb_max = %u\n", priv->can.echo_skb_max);

Also remove the prints, I assume this is left over from
initial debugging.

> +       priv->can.do_set_mode = virtio_can_set_mode;
> +       priv->can.state = CAN_STATE_STOPPED;
> +       /* Set Virtio CAN supported operations */
> +       priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
> +       if (virtio_has_feature(vdev, VIRTIO_CAN_F_CAN_FD)) {
> +               dev_info(&vdev->dev, "CAN FD is supported\n");

> +       } else {
> +               dev_info(&vdev->dev, "CAN FD not supported\n");
> +       }

Same here. There should be a way to see CAN FD support as an interactive
user, but there is no point printing it to the console.

> +
> +       register_virtio_can_dev(dev);
> +
> +       /* Initialize virtqueues */
> +       err = virtio_can_find_vqs(priv);
> +       if (err != 0)
> +               goto on_failure;

Should the register_virtio_can_dev() be done here? I would expect this to be
the last thing after setting up the queues.

> +static struct virtio_driver virtio_can_driver = {
> +       .feature_table = features,
> +       .feature_table_size = ARRAY_SIZE(features),
> +       .feature_table_legacy = NULL,
> +       .feature_table_size_legacy = 0u,
> +       .driver.name =  KBUILD_MODNAME,
> +       .driver.owner = THIS_MODULE,
> +       .id_table =     virtio_can_id_table,
> +       .validate =     virtio_can_validate,
> +       .probe =        virtio_can_probe,
> +       .remove =       virtio_can_remove,
> +       .config_changed = NULL,
> +#ifdef CONFIG_PM_SLEEP
> +       .freeze =       virtio_can_freeze,
> +       .restore =      virtio_can_restore,
> +#endif

You can remove the #ifdef here and above, and replace that with the
pm_sleep_ptr() macro in the assignment.

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

Maybe a link to the specification here? I assume the definitions in this file
are all lifted from that document, rather than specific to the driver, right?

         Arnd

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

[Oliver Hartkopp]

Hi Harald,

On 8/25/22 15:44, Harald Mommer wrote:

> +/*
> + * This function is the NAPI RX poll function and NAPI guarantees that this
> + * function is not invoked simulataneously on multiply processors.
> + * Read a RX message from the used queue and sends it to the upper layer.
> + * (See also m_can.c / m_can_read_fifo()).
> + */
> +static int virtio_can_read_rx_queue(struct virtqueue *vq)
> +{
> +	struct virtio_can_priv *priv = vq->vdev->priv;
> +	struct net_device *dev = priv->dev;
> +	struct net_device_stats *stats = &dev->stats;
> +	struct virtio_can_rx *can_rx;
> +	struct canfd_frame *cf;
> +	struct sk_buff *skb;
> +	unsigned int len;
> +	const unsigned int header_size = offsetof(struct virtio_can_rx, sdu);
> +	u16 msg_type;
> +	u32 can_flags;
> +	u32 can_id;
> +
> +	can_rx = virtqueue_get_buf(vq, &len);
> +	if (!can_rx)
> +		return 0; /* No more data */
> +
> +	BUG_ON(len < header_size);
> +
> +	/* virtio_can_hexdump(can_rx, len, 0u); */
> +
> +	if (priv->can.state >= CAN_STATE_ERROR_PASSIVE) {
> +		netdev_dbg(dev, "%s(): Controller not active\n", __func__);
> +		goto putback;
> +	}
> +
> +	msg_type = le16_to_cpu(can_rx->msg_type);
> +	if (msg_type != VIRTIO_CAN_RX) {
> +		netdev_warn(dev, "RX: Got unknown msg_type %04x\n", msg_type);
> +		goto putback;
> +	}
> +
> +	len -= header_size; /* Payload only now */
> +	can_flags = le32_to_cpu(can_rx->flags);
> +	can_id = le32_to_cpu(can_rx->can_id);
> +
> +	if ((can_flags & ~CAN_KNOWN_FLAGS) != 0u) {

For your entire patch:

Please remove this pointless " != 0u)" stuff.

	if (can_flags & ~CAN_KNOWN_FLAGS) {

is just ok.

> +		stats->rx_dropped++;
> +		netdev_warn(dev, "RX: CAN Id 0x%08x: Invalid flags 0x%x\n",
> +			    can_id, can_flags);
> +		goto putback;
> +	}
> +
> +	if ((can_flags & VIRTIO_CAN_FLAGS_EXTENDED) != 0u) {

e.g. here too ...

> +		if (can_id > CAN_EFF_MASK) {

The MASK is not a number value.

The check should be

if (can_id & ~CAN_EFF_MASK) {

or you simply mask the can_id value to be really sure without the 
netdev_warn() stuff.

Are you sure that you could get undefined CAN ID values here?

> +			stats->rx_dropped++;
> +			netdev_warn(dev, "RX: CAN Ext Id 0x%08x too big\n",

As it is no value 'too big' is not the right comment here.

> +				    can_id);
> +			goto putback;
> +		}
> +		can_id |= CAN_EFF_FLAG;
> +	} else {
> +		if (can_id > CAN_SFF_MASK) {

same here

> +			stats->rx_dropped++;
> +			netdev_warn(dev, "RX: CAN Std Id 0x%08x too big\n",

and here

> +				    can_id);
> +			goto putback;
> +		}
> +	}
> +
> +	if ((can_flags & VIRTIO_CAN_FLAGS_RTR) != 0u) {
> +		if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_RTR_FRAMES)) {
> +			stats->rx_dropped++;
> +			netdev_warn(dev, "RX: CAN Id 0x%08x: RTR not negotiated\n",
> +				    can_id);
> +			goto putback;
> +		}
> +		if ((can_flags & VIRTIO_CAN_FLAGS_FD) != 0u) {
> +			stats->rx_dropped++;
> +			netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with FD not possible\n",
> +				    can_id);
> +			goto putback;
> +		}
> +		if (len != 0u) {
> +			stats->rx_dropped++;
> +			netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with len != 0\n",
> +				    can_id);

This is not the right handling.

Classical CAN frames with RTR bit set can have a DLC value from 0 .. F 
which is represented in

can_frame.len (for values 0 .. 8)
can_frame.len8_dlc (values 9 .. F; len must be 8)

With the RTR bit set, the CAN controller does not send CAN data, but the 
DLC value is set from 0 .. F.

> +			goto putback;
> +		}
> +		can_id |= CAN_RTR_FLAG;
> +	}
> +
> +	if ((can_flags & VIRTIO_CAN_FLAGS_FD) != 0u) {
> +		if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_CAN_FD)) {
> +			stats->rx_dropped++;
> +			netdev_warn(dev, "RX: CAN Id 0x%08x: FD not negotiated\n",
> +				    can_id);
> +			goto putback;
> +		}
> +
> +		skb = alloc_canfd_skb(priv->dev, &cf);
> +		if (len > CANFD_MAX_DLEN)
> +			len = CANFD_MAX_DLEN;

No netdev_warn() here? ;-)

When you silently sanitize the length value here, you should do the same 
with the can_id checks above and simply do a masking like

can_id &= CAN_SFF_MASK or can_id &= CAN_EFF_MASK


> +	} else {
> +		if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_CAN_CLASSIC)) {
> +			stats->rx_dropped++;
> +			netdev_warn(dev, "RX: CAN Id 0x%08x: classic not negotiated\n",
> +				    can_id);
> +			goto putback;
> +		}
> +
> +		skb = alloc_can_skb(priv->dev, (struct can_frame **)&cf);
> +		if (len > CAN_MAX_DLEN)
> +			len = CAN_MAX_DLEN;
> +	}
> +	if (!skb) {
> +		stats->rx_dropped++;
> +		netdev_warn(dev, "RX: No skb available\n");
> +		goto putback;
> +	}
> +
> +	cf->can_id = can_id;
> +	cf->len = len;
> +	if ((can_flags & VIRTIO_CAN_FLAGS_RTR) == 0u) {
> +		/* Copy if no RTR frame. RTR frames have a DLC but no payload */
> +		memcpy(cf->data, can_rx->sdu, len);

This would need some rework together with the RTR handling too.

> +	}
> +
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->len;
> +
> +	/* Use netif_rx() for interrupt context */

?? What is this comment about?

> +	(void)netif_receive_skb(skb);

Why this "(void)" here and at other places in the patch? Please remove.

Is there no error handling needed when netif_receive_skb() fails? Or ar 
least some statistics rollback?

> +
> +putback:
> +	/* Put processed RX buffer back into avail queue */
> +	(void)virtio_can_add_inbuf(vq, can_rx, sizeof(struct virtio_can_rx));
> +
> +	return 1; /* Queue was not emtpy so there may be more data */
> +}

Best regards,
Oliver

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 9086 bytes --]

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");
| 
| WARNING: From:/Signed-off-by: email address mismatch: 'From: Harald Mommer <harald.mommer@opensynergy.com>' != 'Signed-off-by: Harald Mommer <hmo@opensynergy.com>'
| 
| total: 0 errors, 38 warnings, 1275 lines checked

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oliver Hartkopp]

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@opensynergy.com>' != 'Signed-off-by: Harald Mommer <hmo@opensynergy.com>'
> |
> | total: 0 errors, 38 warnings, 1275 lines checked
> 
> regards,
> Marc
> 

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

[Harald Mommer]

Hello,

currently in the preparation that changed code can go out to the list.

On 25.08.22 20:21, Arnd Bergmann wrote:

>
>>   drivers/net/can/Kconfig                 |    1 +
>>   drivers/net/can/Makefile                |    1 +
>>   drivers/net/can/virtio_can/Kconfig      |   12 +
>>   drivers/net/can/virtio_can/Makefile     |    5 +
>>   drivers/net/can/virtio_can/virtio_can.c | 1176 +++++++++++++++++++++++
>>   include/uapi/linux/virtio_can.h         |   69 ++
> Since the driver is just one file, you probably don't need the subdirectory.
Easy to do, makes the changes smaller.
>> +struct virtio_can_tx {
>> +       struct list_head list;
>> +       int prio; /* Currently always 0 "normal priority" */
>> +       int putidx;
>> +       struct virtio_can_tx_out tx_out;
>> +       struct virtio_can_tx_in tx_in;
>> +};
> Having a linked list of these appears to add a little extra complexity.
> If they are always processed in sequence, using an array would be
> much simpler, as you just need to remember the index.

The messages are not necessarily processed in sequence by the CAN stack. 
CAN is priority based. The lower the CAN ID the higher the priority. So 
a message with CAN ID 0x100 can surpass a message with ID 0x123 if the 
hardware is not just simple basic CAN controller using a single TX 
mailbox with a FIFO queue on top of it.

Thinking about this the code becomes more complex with the array. What I 
get from the device when the message has been processed is a pointer to 
the processed message by virtqueue_get_buf(). I can then simply do a 
list_del(), free the message and done.

>> +#ifdef DEBUG
>> +static void __attribute__((unused))
>> +virtio_can_hexdump(const void *data, size_t length, size_t base)
>> +{
>> +#define VIRTIO_CAN_MAX_BYTES_PER_LINE 16u
> This seems to duplicate print_hex_dump(), maybe just use that?
Checked where it's still used. The code is not disabled by #ifdef DEBUG 
but simply commented out. Under this circumstances it's for now best to 
simply remove the code now and also the commented out places where is 
was used at some time in the past.
>> +
>> +       while (!virtqueue_get_buf(vq, &len) && !virtqueue_is_broken(vq))
>> +               cpu_relax();
>> +
>> +       mutex_unlock(&priv->ctrl_lock);
> A busy loop is probably not what you want here. Maybe just
> wait_for_completion() until the callback happens?

Was done in the same way as elsewhere 
(virtio_console.c/__send_control_msg() & 
virtio_net.c/virtnet_send_command()). Yes, wait_for_completion() is 
better, this avoids polling.

>> +       /* Push loopback echo. Will be looped back on TX interrupt/TX NAPI */
>> +       can_put_echo_skb(skb, dev, can_tx_msg->putidx, 0);
>> +
>> +       err = virtqueue_add_sgs(vq, sgs, 1u, 1u, can_tx_msg, GFP_ATOMIC);
>> +       if (err != 0) {
>> +               list_del(&can_tx_msg->list);
>> +               virtio_can_free_tx_idx(priv, can_tx_msg->prio,
>> +                                      can_tx_msg->putidx);
>> +               netif_stop_queue(dev);
>> +               spin_unlock_irqrestore(&priv->tx_lock, flags);
>> +               kfree(can_tx_msg);
>> +               if (err == -ENOSPC)
>> +                       netdev_info(dev, "TX: Stop queue, no space left\n");
>> +               else
>> +                       netdev_warn(dev, "TX: Stop queue, reason = %d\n", err);
>> +               return NETDEV_TX_BUSY;
>> +       }
>> +
>> +       if (!virtqueue_kick(vq))
>> +               netdev_err(dev, "%s(): Kick failed\n", __func__);
>> +
>> +       spin_unlock_irqrestore(&priv->tx_lock, flags);
> There should not be a need for a spinlock or disabling interrupts
> in the xmit function. What exactly are you protecting against here?

I'm using 2 NAPIs, one for TX and one for RX. The RX NAPI just receives 
RX messages and is of no interest here. The TX NAPI handles the TX 
messages which have been processed by the virtio CAN device in 
virtio_can_read_tx_queue(). If this was done without the TX NAPI this 
would have been done by the TX interrupt directly, no difference.

In virtio_can_start_xmit()

* Reserve putidx - done by an own mechanism using list operations in 
tx_putidx_list

Could be that it's simpler to use idr_alloc() and friends getting those 
numbers to get rid of this own mechanism, not sure yet. But this needs a 
locks as it's based on a linked list and the list operation has to be 
protected.

* Add the TX message to the pending list

Again a list operation which has to be protected.

* Try to send the message

Now it may happen that at the same time while we do something with the 
lists in virtio_can_start_xmit() the function virtio_can_read_tx_queue() 
is active accessing the same queue. Comment above virtqueue_add_sgs(): 
"Caller must ensure that we don't call this with other virtqueue 
operations at the same time (except when noted)."

Also tried, virtqueue_add_sgs() needs this lock.

* And then there is also a list operation on failure of the function

But the code needed to reworked to understand the necessity of each lock 
again.

> As a further optimization, you may want to use the xmit_more()
> function, as the virtqueue kick is fairly expensive and can be
> batched here.
Looked elsewhere how it works and did.
>> +       kfree(can_tx_msg);
>> +
>> +       /* Flow control */
>> +       if (netif_queue_stopped(dev)) {
>> +               netdev_info(dev, "TX ACK: Wake up stopped queue\n");
>> +               netif_wake_queue(dev);
>> +       }
> You may want to add netdev_sent_queue()/netdev_completed_queue()
> based BQL flow control here as well, so you don't have to rely on the
> queue filling up completely.
Not addressed, not yet completely understood.
>> +static int virtio_can_probe(struct virtio_device *vdev)
>> +{
>> +       struct net_device *dev;
>> +       struct virtio_can_priv *priv;
>> +       int err;
>> +       unsigned int echo_skb_max;
>> +       unsigned int idx;
>> +       u16 lo_tx = VIRTIO_CAN_ECHO_SKB_MAX;
>> +
>> +       BUG_ON(!vdev);
> Not a useful debug check, just remove the BUG_ON(!vdev), here and elsewhere
A lot of BUG_ON() were removed when not considered useful, some were 
reworked to contain better error handling code when this was possible, 
others were kept to ease further development. If anyone catches 
something would be seriously broken and had to be fixed in the code. But 
this then we want to know.
>> +
>> +       echo_skb_max = lo_tx;
>> +       dev = alloc_candev(sizeof(struct virtio_can_priv), echo_skb_max);
>> +       if (!dev)
>> +               return -ENOMEM;
>> +
>> +       priv = netdev_priv(dev);
>> +
>> +       dev_info(&vdev->dev, "echo_skb_max = %u\n", priv->can.echo_skb_max);
> Also remove the prints, I assume this is left over from
> initial debugging.
Yes, this thing was overall too noisy.
>> +
>> +       register_virtio_can_dev(dev);
>> +
>> +       /* Initialize virtqueues */
>> +       err = virtio_can_find_vqs(priv);
>> +       if (err != 0)
>> +               goto on_failure;
> Should the register_virtio_can_dev() be done here? I would expect this to be
> the last thing after setting up the queues.
Doing so makes the code somewhat simpler and shorter = better.
>> +static struct virtio_driver virtio_can_driver = {
>> +       .feature_table = features,
>> +       .feature_table_size = ARRAY_SIZE(features),
>> +       .feature_table_legacy = NULL,
>> +       .feature_table_size_legacy = 0u,
>> +       .driver.name =  KBUILD_MODNAME,
>> +       .driver.owner = THIS_MODULE,
>> +       .id_table =     virtio_can_id_table,
>> +       .validate =     virtio_can_validate,
>> +       .probe =        virtio_can_probe,
>> +       .remove =       virtio_can_remove,
>> +       .config_changed = NULL,
>> +#ifdef CONFIG_PM_SLEEP
>> +       .freeze =       virtio_can_freeze,
>> +       .restore =      virtio_can_restore,
>> +#endif
> You can remove the #ifdef here and above, and replace that with the
> pm_sleep_ptr() macro in the assignment.

This pm_sleep_ptr(_ptr) macro returns either the argument when 
CONFIG_PM_SLEEP is defined or NULL. But in struct virtio_driver there is

#ifdef CONFIG_PM   int(*freeze) ...;   int(*restore) ...; #endif

so without CONFIG_PM there are no freeze and restore structure members.

So

   .freeze = pm_sleep_ptr(virtio_can_freeze)

won't work.

>> 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>
> Maybe a link to the specification here? I assume the definitions in this file
> are all lifted from that document, rather than specific to the driver, right?
>
>           Arnd

The driver as made in parallel to the specification work. So there is no 
finished specification yet. To avoid traffic (mistake here) I've not 
sent the patch to the specification to all mailing lists.

Patch to the virtio specification is now here:

https://lore.kernel.org/all/20220825133410.18367-1-harald.mommer@opensynergy.com/

This was made on top of https://github.com/oasis-tcs/virtio-spec.git 
commit 26ed30ccb049

Harald

As mentioned, the updated code will be sent out to the list(s) soon.

-- 
Dipl.-Ing. Harald Mommer
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone:  +49 (30) 60 98 540-0 <== Zentrale
Fax:    +49 (30) 60 98 540-99
E-Mail:harald.mommer@opensynergy.com

www.opensynergy.com

Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah

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

[Harald Mommer]

Hello,

On 27.08.22 11:02, Oliver Hartkopp wrote:
>> +    if ((can_flags & ~CAN_KNOWN_FLAGS) != 0u) {
>
> For your entire patch:
>
> Please remove this pointless " != 0u)" stuff.
>
>     if (can_flags & ~CAN_KNOWN_FLAGS) {
>
> is just ok.
Too much MISRA habit in my head which has no place here. Did so.
>> +        if (can_id > CAN_EFF_MASK) {
>
> The MASK is not a number value.
>
> The check should be
>
> if (can_id & ~CAN_EFF_MASK) {
>
> or you simply mask the can_id value to be really sure without the 
> netdev_warn() stuff.
CAN_EFF_MASK is now treated as bitmask. And decided to remove this 
netdev_warn() stuff as proposed, masked the values. So I will miss 
invalid combinations from the virtio CAN device (may also be buggy) I 
might still be interested in but this excessive netdev_warn() usage was 
in the end also for my taste too much.
>
> Are you sure that you could get undefined CAN ID values here?
>
>> +            stats->rx_dropped++;
>> +            netdev_warn(dev, "RX: CAN Ext Id 0x%08x too big\n",

The proposed virtio CAN specification says: "The type of a CAN message 
identifier is determined by flags. The 3 most significant bits of can_id 
do not bear the information about the type of the CAN message identifier 
and are 0."

=> This trace could have been seen when a buggy device wasn't obeying 
the 2nd sentence.

>> +        if (len != 0u) {
>> +            stats->rx_dropped++;
>> +            netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with len != 0\n",
>> +                    can_id);
>
> This is not the right handling.
>
> Classical CAN frames with RTR bit set can have a DLC value from 0 .. F 
> which is represented in
>
> can_frame.len (for values 0 .. 8)
> can_frame.len8_dlc (values 9 .. F; len must be 8)
>
> With the RTR bit set, the CAN controller does not send CAN data, but 
> the DLC value is set from 0 .. F.

RTR frames! DLC values <> 0 but then no payload. This needed to be 
reworked and fixed.

> When you silently sanitize the length value here, you should do the 
> same with the can_id checks above and simply do a masking like
>
> can_id &= CAN_SFF_MASK or can_id &= CAN_EFF_MASK
>
Did so. Too much netdev_warn() in the code, really. Too much is too much.
>> +    (void)netif_receive_skb(skb);
>
> Why this "(void)" here and at other places in the patch? Please remove.
> Is there no error handling needed when netif_receive_skb() fails? Or 
> ar least some statistics rollback?
Old MISRA habit to silence the warning when no error is expected to be 
possible to occur. This has no place here and was replaced by some 
better error handling evaluating the returned error not updating the 
statistics as proposed. For the (void) below just omitted the (void), I 
see no possible good error handling there and we're not going to run the 
driver through the MISRA checker.
>> +
>> +putback:
>> +    /* Put processed RX buffer back into avail queue */
>> +    (void)virtio_can_add_inbuf(vq, can_rx, sizeof(struct 
>> virtio_can_rx));
>> +
>> +    return 1; /* Queue was not emtpy so there may be more data */
>> +}
>
> Best regards,
> Oliver
>
Regards
Harald

(First answering all those review E-Mails, then will have to fight with 
sending the changed code).

-- 
Dipl.-Ing. Harald Mommer
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone:  +49 (30) 60 98 540-0 <== Zentrale
Fax:    +49 (30) 60 98 540-99
E-Mail: harald.mommer@opensynergy.com

www.opensynergy.com

Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah

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

[Harald Mommer]

Hello,

On 27.08.22 11:39, Marc Kleine-Budde wrote:
> Is there an Open Source implementation of the host side of this
> interface?
there is neither an open source device nor is it currently planned. The 
device I'm developing is closed source.
> Please fix these checkpatch warnings:
>
> | WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> | #65:
> | new file mode 100644
> |
Addressed most of the checkpatch warnings. Regarding the BUG_ON() 
minimized usage. Removed where not necessary, reworked where a better 
error handling was possible, kept where I don't think it will catch ever 
but if it catches I would like to know. Of course I could make the tool 
happy but on the other hand I don't want to fly completely blind in case 
a bug has to be hunted.
> regards,
> Marc

Regards
Harald

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

[Arnd Bergmann]

On Thu, Nov 3, 2022, at 13:26, Harald Mommer wrote:
> On 25.08.22 20:21, Arnd Bergmann wrote:
>>
...
> The messages are not necessarily processed in sequence by the CAN stack. 
> CAN is priority based. The lower the CAN ID the higher the priority. So 
> a message with CAN ID 0x100 can surpass a message with ID 0x123 if the 
> hardware is not just simple basic CAN controller using a single TX 
> mailbox with a FIFO queue on top of it.
>
> Thinking about this the code becomes more complex with the array. What I 
> get from the device when the message has been processed is a pointer to 
> the processed message by virtqueue_get_buf(). I can then simply do a 
> list_del(), free the message and done.

Ok

>>> +#ifdef DEBUG
>>> +static void __attribute__((unused))
>>> +virtio_can_hexdump(const void *data, size_t length, size_t base)
>>> +{
>>> +#define VIRTIO_CAN_MAX_BYTES_PER_LINE 16u
>> This seems to duplicate print_hex_dump(), maybe just use that?
> Checked where it's still used. The code is not disabled by #ifdef DEBUG 
> but simply commented out. Under this circumstances it's for now best to 
> simply remove the code now and also the commented out places where is 
> was used at some time in the past.

Even better.

>>> +
>>> +       while (!virtqueue_get_buf(vq, &len) && !virtqueue_is_broken(vq))
>>> +               cpu_relax();
>>> +
>>> +       mutex_unlock(&priv->ctrl_lock);
>> A busy loop is probably not what you want here. Maybe just
>> wait_for_completion() until the callback happens?
>
> Was done in the same way as elsewhere 
> (virtio_console.c/__send_control_msg() & 
> virtio_net.c/virtnet_send_command()). Yes, wait_for_completion() is 
> better, this avoids polling.

Ok. FWIW, The others seem to do it like this because they
are in non-atomic context where it is not allowed to call
wait_for_completion(), but since you already have the
mutex here, you know that sleeping is permitted. 

>>> +       /* Push loopback echo. Will be looped back on TX interrupt/TX NAPI */
>>> +       can_put_echo_skb(skb, dev, can_tx_msg->putidx, 0);
>>> +
>>> +       err = virtqueue_add_sgs(vq, sgs, 1u, 1u, can_tx_msg, GFP_ATOMIC);
>>> +       if (err != 0) {
>>> +               list_del(&can_tx_msg->list);
>>> +               virtio_can_free_tx_idx(priv, can_tx_msg->prio,
>>> +                                      can_tx_msg->putidx);
>>> +               netif_stop_queue(dev);
>>> +               spin_unlock_irqrestore(&priv->tx_lock, flags);
>>> +               kfree(can_tx_msg);
>>> +               if (err == -ENOSPC)
>>> +                       netdev_info(dev, "TX: Stop queue, no space left\n");
>>> +               else
>>> +                       netdev_warn(dev, "TX: Stop queue, reason = %d\n", err);
>>> +               return NETDEV_TX_BUSY;
>>> +       }
>>> +
>>> +       if (!virtqueue_kick(vq))
>>> +               netdev_err(dev, "%s(): Kick failed\n", __func__);
>>> +
>>> +       spin_unlock_irqrestore(&priv->tx_lock, flags);
>> There should not be a need for a spinlock or disabling interrupts
>> in the xmit function. What exactly are you protecting against here?
>
> I'm using 2 NAPIs, one for TX and one for RX. The RX NAPI just receives 
> RX messages and is of no interest here. The TX NAPI handles the TX 
> messages which have been processed by the virtio CAN device in 
> virtio_can_read_tx_queue(). If this was done without the TX NAPI this 
> would have been done by the TX interrupt directly, no difference.
>
> In virtio_can_start_xmit()
>
> * Reserve putidx - done by an own mechanism using list operations in 
> tx_putidx_list
>
> Could be that it's simpler to use idr_alloc() and friends getting those 
> numbers to get rid of this own mechanism, not sure yet. But this needs a 
> locks as it's based on a linked list and the list operation has to be 
> protected.

Right, makes sense. Lockless transmission should generally work
if your transmission queue is a strictly ordered ring buffer
where you just need to atomically update the index, but you are
right that this doesn't work with a linked list.

This probably directly ties into the specification of your
tx virtqueue: if the device could guarantee that any descriptors
are processed in sequence, you could avoid the spinlock in the
tx path for a small performance optimization, but then you have
to decide on the sequence in the driver already, which impacts
the latency for high-priority frames that get queued to the
device. It's possible that the reordering in the device would
not be as critical if you correctly implement the byte queue
limits.

>>> +       kfree(can_tx_msg);
>>> +
>>> +       /* Flow control */
>>> +       if (netif_queue_stopped(dev)) {
>>> +               netdev_info(dev, "TX ACK: Wake up stopped queue\n");
>>> +               netif_wake_queue(dev);
>>> +       }
>> You may want to add netdev_sent_queue()/netdev_completed_queue()
>> based BQL flow control here as well, so you don't have to rely on the
>> queue filling up completely.
> Not addressed, not yet completely understood.

https://lwn.net/Articles/454390/ is an older article but should still
explain the background. Without byte queue limits, you risk introducing
unbounded TX latency on a congested interface.

Ideally, the host device implementation should only send
back the 'completed' interrupt after a frame has left the
physical hardware. In this case, BQL will manage both the
TX queue in the guest driver and the queue in the host
device to keep the total queue length short enough to
guarantee low latency even for low-priority frames but
long enough to maintain wire-speed throughput.

>>> +
>>> +       register_virtio_can_dev(dev);
>>> +
>>> +       /* Initialize virtqueues */
>>> +       err = virtio_can_find_vqs(priv);
>>> +       if (err != 0)
>>> +               goto on_failure;
>> Should the register_virtio_can_dev() be done here? I would expect this to be
>> the last thing after setting up the queues.
>
> Doing so makes the code somewhat simpler and shorter = better.

The problem is that as soon as an interface is registered,
you can have userspace sending data to it. This may be
a short race, but I fear that this would cause data corruption
if data gets queued before the device is fully operational.

>>> +#ifdef CONFIG_PM_SLEEP
>>> +       .freeze =       virtio_can_freeze,
>>> +       .restore =      virtio_can_restore,
>>> +#endif
>> You can remove the #ifdef here and above, and replace that with the
>> pm_sleep_ptr() macro in the assignment.
>
> This pm_sleep_ptr(_ptr) macro returns either the argument when 
> CONFIG_PM_SLEEP is defined or NULL. But in struct virtio_driver there is
>
> #ifdef CONFIG_PM   int(*freeze) ...;   int(*restore) ...; #endif
>
> so without CONFIG_PM there are no freeze and restore structure members.
>
> So
>
>    .freeze = pm_sleep_ptr(virtio_can_freeze)
>
> won't work.

I think this is a mistake in the virtio_driver definition,
it would be best to send a small patch that removes this
#ifdef along with your driver.

       Arnd

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

[Jan Kiszka]

On 03.11.22 14:55, Harald Mommer wrote:
> Hello,
> 
> On 27.08.22 11:39, Marc Kleine-Budde wrote:
>> Is there an Open Source implementation of the host side of this
>> interface?
> there is neither an open source device nor is it currently planned. The
> device I'm developing is closed source.

Likely not helpful long-term /wrt kernel QA - how should kernelci or
others even have a chance to test the driver? Keep in mind that you are
not proposing a specific driver for an Opensynergy hypervisor, rather
for the open and vendor-agnostic virtio spec.

But QEMU already supports both CAN and virtio, thus should be relatively
easy to augment with this new device.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

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

[Arnd Bergmann]

On Fri, Nov 4, 2022, at 16:32, Jan Kiszka wrote:
> On 03.11.22 14:55, Harald Mommer wrote:
>> 
>> On 27.08.22 11:39, Marc Kleine-Budde wrote:
>>> Is there an Open Source implementation of the host side of this
>>> interface?
>> there is neither an open source device nor is it currently planned. The
>> device I'm developing is closed source.
>
> Likely not helpful long-term /wrt kernel QA - how should kernelci or
> others even have a chance to test the driver? Keep in mind that you are
> not proposing a specific driver for an Opensynergy hypervisor, rather
> for the open and vendor-agnostic virtio spec.
>
> But QEMU already supports both CAN and virtio, thus should be relatively
> easy to augment with this new device.

Agreed, either hooking into the qemu support, or having a separate
vhost-user backend that forwards data to the host stack would be
helpful here, in particular to see how the flow control works.

IIRC when we discussed virtio-can on the stratos list, one of the
issues that was pointed out was filtering of frames for specific
CAN IDs in the host socketcan for assigning individual IDs to
separate guests.  It would be good to understand whether a generic
host implementation has the same problems, and what can be
done in socketcan to help with that.

      Arnd

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

[Vincent Mailhol]

On Fry. 4 nov. 2022 at 20:13, Arnd Bergmann <arnd@kernel.org> wrote:
> On Thu, Nov 3, 2022, at 13:26, Harald Mommer wrote:
> > On 25.08.22 20:21, Arnd Bergmann wrote:
> >>
> ...
> > The messages are not necessarily processed in sequence by the CAN stack.
> > CAN is priority based. The lower the CAN ID the higher the priority. So
> > a message with CAN ID 0x100 can surpass a message with ID 0x123 if the
> > hardware is not just simple basic CAN controller using a single TX
> > mailbox with a FIFO queue on top of it.

Really? I acknowledge that it is priority based *on the bus*, i.e. if
two devices A and B on the same bus try to send CAN ID 0x100 and 0x123
at the same time, then device A will win the CAN arbitration.
However, I am not aware of any devices which reorder their own stack
according to the CAN IDs. If I first send CAN ID 0x123 and then ID
0x100 on the device stack, 0x123 would still go out first, right?

> > Thinking about this the code becomes more complex with the array. What I
> > get from the device when the message has been processed is a pointer to
> > the processed message by virtqueue_get_buf(). I can then simply do a
> > list_del(), free the message and done.
>
> Ok

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

[Harald Mommer]

Hello,

we had here at OpenSynergy an internal discussion about an open source 
virtio-can device implementation.

The outcome of this is now that an open source virtio-can device is to 
be developed.

It has not yet been decided whether the open source device 
implementation will be done using qemu or kvmtool (or something else?). 
Negative or positive feedback for or against one of those is likely to 
influence the decision what will be used as basis for the development. 
Using kvmtool may be easier to do for me (to be investigated in detail) 
but on the other hand we have some people around in the team who have 
the knowledge to support with qemu.


On 04.11.22 18:03, Arnd Bergmann wrote:
> On Fri, Nov 4, 2022, at 16:32, Jan Kiszka wrote:
>> On 03.11.22 14:55, Harald Mommer wrote:
>>> On 27.08.22 11:39, Marc Kleine-Budde wrote:
>>>> Is there an Open Source implementation of the host side of this
>>>> interface?
>>> there is neither an open source device nor is it currently planned. The
>>> device I'm developing is closed source.
>> Likely not helpful long-term /wrt kernel QA - how should kernelci or
>> others even have a chance to test the driver? Keep in mind that you are
>> not proposing a specific driver for an Opensynergy hypervisor, rather
>> for the open and vendor-agnostic virtio spec.
>>
>> But QEMU already supports both CAN and virtio, thus should be relatively
>> easy to augment with this new device.
> Agreed, either hooking into the qemu support, or having a separate
> vhost-user backend that forwards data to the host stack would be
> helpful here, in particular to see how the flow control works.

What I would like to have is considering a CAN frame as sent when it was 
sent on the bus (vs. given to the lower layers where it is only 
scheduled for later transmission but not actually physically sent). This 
behavior is enabled by feature flag VIRTIO_CAN_F_LATE_TX_ACK. But under 
really heavy load conditions this does not work reliably. It looks like 
the own transmitted frames are discarded sometimes in heavy overload.

The reception of the own transmitted frames is used to trigger the state 
transition from "TX pending" => "TX done" for a pending transmitted 
frame in the device. So loosing those own transmitted frames leads to 
the situation that "TX pending" frames stay in this state forever and 
everything gets stuck quickly. So the feature flag 
VIRTIO_CAN_F_LATE_TX_ACK is currently not usable reliably in Linux. 
Either I need to find a good workaround or better a proper way to avoid 
that any of those acknowledgement frames is lost ever. I've no good 
solution found to cope with this yet.

But without VIRTIO_CAN_F_LATE_TX_ACK there is also no working flow 
control. Means I would like to see some day not only "how flow control 
works" but also "that flow control works regardless how the CAN stack is 
tortured".

> IIRC when we discussed virtio-can on the stratos list, one of the
> issues that was pointed out was filtering of frames for specific
> CAN IDs in the host socketcan for assigning individual IDs to
> separate guests.  It would be good to understand whether a generic
> host implementation has the same problems, and what can be
> done in socketcan to help with that.
>
>        Arnd
>
Harald

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

On 03.02.2023 16:02:04, Harald Mommer wrote:
> we had here at OpenSynergy an internal discussion about an open source
> virtio-can device implementation.
> 
> The outcome of this is now that an open source virtio-can device is to be
> developed.
> 
> It has not yet been decided whether the open source device implementation
> will be done using qemu or kvmtool (or something else?). Negative or
> positive feedback for or against one of those is likely to influence the
> decision what will be used as basis for the development. Using kvmtool may
> be easier to do for me (to be investigated in detail) but on the other hand
> we have some people around in the team who have the knowledge to support
> with qemu.

It there some code available yet? We as Pengutronix will be on our
yearly techweek soon and I want to look into the VirtIO CAN stuff.

regards,
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: [virtio-dev] Re: [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver.

[Harald Mommer]

Hello,

there is now an implementation of the device for qemu internally under 
review.

The paperwork with the company lawyer to publish source code has also 
been done.

At the same time we changed the driver and the specification draft also, 
especially the bus off indication is now done by a config space bit 
instead of having a dedicated indication queue. Minor updates to some 
other structures. This means what we have will match an updated driver 
and specification version not yet sent out. Also to be done so that 
people can play around.

So an open source qemu device is in the work and we are close to publish 
this in our public github repository but until now nothing has been 
pushed yet to github.

Regards
Harald

On 14.04.23 21:20, Marc Kleine-Budde wrote:
> On 03.02.2023 16:02:04, Harald Mommer wrote:
>> we had here at OpenSynergy an internal discussion about an open source
>> virtio-can device implementation.
>>
>> The outcome of this is now that an open source virtio-can device is to be
>> developed.
>>
>> It has not yet been decided whether the open source device implementation
>> will be done using qemu or kvmtool (or something else?). Negative or
>> positive feedback for or against one of those is likely to influence the
>> decision what will be used as basis for the development. Using kvmtool may
>> be easier to do for me (to be investigated in detail) but on the other hand
>> we have some people around in the team who have the knowledge to support
>> with qemu.
> It there some code available yet? We as Pengutronix will be on our
> yearly techweek soon and I want to look into the VirtIO CAN stuff.
>
> regards,
> Marc
>

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On 18.04.2023 11:50:56, Harald Mommer wrote:
> there is now an implementation of the device for qemu internally under
> review.

Great news!

> The paperwork with the company lawyer to publish source code has also been
> done.

This was probably more work than the actual coding :)

> At the same time we changed the driver and the specification draft also,
> especially the bus off indication is now done by a config space bit instead
> of having a dedicated indication queue. Minor updates to some other
> structures. This means what we have will match an updated driver and
> specification version not yet sent out. Also to be done so that people can
> play around.

Looking forward to see the code.

> So an open source qemu device is in the work and we are close to publish
> this in our public github repository but until now nothing has been pushed
> yet to github.

Next week is our yearly hacking week. Some working qemu and Linux
drivers would be excellent.

regards,
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: [virtio-dev] [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver.

[Harald Mommer]

Hello Vincent,

searched for the old E-Mail, this was one of that which slipped through. 
Too much of those.

On 05.11.22 10:21, Vincent Mailhol wrote:
> On Fry. 4 nov. 2022 at 20:13, Arnd Bergmann <arnd@kernel.org> wrote:
>> On Thu, Nov 3, 2022, at 13:26, Harald Mommer wrote:
>>> On 25.08.22 20:21, Arnd Bergmann wrote:
>> ...
>>> The messages are not necessarily processed in sequence by the CAN stack.
>>> CAN is priority based. The lower the CAN ID the higher the priority. So
>>> a message with CAN ID 0x100 can surpass a message with ID 0x123 if the
>>> hardware is not just simple basic CAN controller using a single TX
>>> mailbox with a FIFO queue on top of it.
> Really? I acknowledge that it is priority based *on the bus*, i.e. if
> two devices A and B on the same bus try to send CAN ID 0x100 and 0x123
> at the same time, then device A will win the CAN arbitration.
> However, I am not aware of any devices which reorder their own stack
> according to the CAN IDs. If I first send CAN ID 0x123 and then ID
> 0x100 on the device stack, 0x123 would still go out first, right?

The CAN hardware may be a basic CAN hardware: Single mailbox only with a 
TX FIFO on top of this.

No reordering takes place, the CAN hardware will try to arbitrate the 
CAN bus with a low priority CAN message (big CAN ID) while some high 
priority CAN message (small CAN ID) is waiting in the FIFO. This is 
called "internal priority inversion", a property of basic CAN hardware. 
A basic CAN hardware does exactly what you describe.

Should be the FIFO in software it's a bad idea to try to improve this 
doing some software sorting, the processing time needed is likely to 
make things even worse. Therefore no software does this or at least it's 
not recommended to do this.

But the hardware may also be a better one. No FIFO but a lot of TX 
mailboxes. A full CAN hardware tries to arbitrate the bus using the 
highest priority waiting CAN message considering all hardware TX 
mailboxes. Such a better (full CAN) hardware does not cause "internal 
priority inversion" but tries to arbitrate the bus in the correct order 
given by the message IDs.

We don't know about the actually used CAN hardware and how it's used on 
this level we are with our virtio can device. We are using SocketCAN, no 
information about the properties of the underlying hardware is provided 
at some API. May be basic CAN using a FIFO and a single TX mailbox or 
full CAN using a lot of TX mailboxes in parallel.

On the bus it's guaranteed always that the sender with the lowest CAN ID 
winds regardless which hardware is used, the only difference is whether 
we have "internal priority inversion" or not.

If I look at the CAN stack = Software + hardware (and not only software) 
it's correct: The hardware device may re-order if it's a better (full 
CAN) one and thus the actual sending on the bus is not done in the same 
sequence as the messages were provided internally (e.g. at some socket).

Regards
Harald

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

[Vincent Mailhol]

Hi Harald,

On Fri. 12 May 2023 at 22:19, Harald Mommer
<harald.mommer@opensynergy.com> wrote:
> Hello Vincent,
>
> searched for the old E-Mail, this was one of that which slipped through.
> Too much of those.
>
> On 05.11.22 10:21, Vincent Mailhol wrote:
> > On Fry. 4 nov. 2022 at 20:13, Arnd Bergmann <arnd@kernel.org> wrote:
> >> On Thu, Nov 3, 2022, at 13:26, Harald Mommer wrote:
> >>> On 25.08.22 20:21, Arnd Bergmann wrote:
> >> ...
> >>> The messages are not necessarily processed in sequence by the CAN stack.
> >>> CAN is priority based. The lower the CAN ID the higher the priority. So
> >>> a message with CAN ID 0x100 can surpass a message with ID 0x123 if the
> >>> hardware is not just simple basic CAN controller using a single TX
> >>> mailbox with a FIFO queue on top of it.
> > Really? I acknowledge that it is priority based *on the bus*, i.e. if
> > two devices A and B on the same bus try to send CAN ID 0x100 and 0x123
> > at the same time, then device A will win the CAN arbitration.
> > However, I am not aware of any devices which reorder their own stack
> > according to the CAN IDs. If I first send CAN ID 0x123 and then ID
> > 0x100 on the device stack, 0x123 would still go out first, right?
>
> The CAN hardware may be a basic CAN hardware: Single mailbox only with a
> TX FIFO on top of this.
>
> No reordering takes place, the CAN hardware will try to arbitrate the
> CAN bus with a low priority CAN message (big CAN ID) while some high
> priority CAN message (small CAN ID) is waiting in the FIFO. This is
> called "internal priority inversion", a property of basic CAN hardware.
> A basic CAN hardware does exactly what you describe.
>
> Should be the FIFO in software it's a bad idea to try to improve this
> doing some software sorting, the processing time needed is likely to
> make things even worse. Therefore no software does this or at least it's
> not recommended to do this.
>
> But the hardware may also be a better one. No FIFO but a lot of TX
> mailboxes. A full CAN hardware tries to arbitrate the bus using the
> highest priority waiting CAN message considering all hardware TX
> mailboxes. Such a better (full CAN) hardware does not cause "internal
> priority inversion" but tries to arbitrate the bus in the correct order
> given by the message IDs.
>
> We don't know about the actually used CAN hardware and how it's used on
> this level we are with our virtio can device. We are using SocketCAN, no
> information about the properties of the underlying hardware is provided
> at some API. May be basic CAN using a FIFO and a single TX mailbox or
> full CAN using a lot of TX mailboxes in parallel.
>
> On the bus it's guaranteed always that the sender with the lowest CAN ID
> winds regardless which hardware is used, the only difference is whether
> we have "internal priority inversion" or not.
>
> If I look at the CAN stack = Software + hardware (and not only software)
> it's correct: The hardware device may re-order if it's a better (full
> CAN) one and thus the actual sending on the bus is not done in the same
> sequence as the messages were provided internally (e.g. at some socket).

OK. Thanks for the clarification.

So, you are using scatterlist to be able to interface with the
different CAN mailboxes. But then, it means that all the heuristics to
manage those mailboxes are done in the virtio host.

Did you consider exposing the number of supported mailboxes as a
configuration parameter and let the virtio guest manage these? In
Linux, it is supported since below commit:

  commit 038709071328 ("can: dev: enable multi-queue for SocketCAN devices")
  Link: https://git.kernel.org/torvalds/c/038709071328

Generally, from a design perspective, isn't it better to make the
virtio host as dumb as possible and let the host do the management?

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

[Harald Mommer]

Hello Vincent,

On 15.05.23 07:58, Vincent Mailhol wrote:
> Hi Harald,
>
> On Fri. 12 May 2023 at 22:19, Harald Mommer
> <harald.mommer@opensynergy.com>  wrote:
>> Hello Vincent,
>>
>> searched for the old E-Mail, this was one of that which slipped through.
>> Too much of those.
>>
>> On 05.11.22 10:21, Vincent Mailhol wrote:
>>> On Fry. 4 nov. 2022 at 20:13, Arnd Bergmann<arnd@kernel.org>  wrote:
>>>> On Thu, Nov 3, 2022, at 13:26, Harald Mommer wrote:
>>>>> On 25.08.22 20:21, Arnd Bergmann wrote:
>>>> ...
>>>>> The messages are not necessarily processed in sequence by the CAN stack.
>>>>> CAN is priority based. The lower the CAN ID the higher the priority. So
>>>>> a message with CAN ID 0x100 can surpass a message with ID 0x123 if the
>>>>> hardware is not just simple basic CAN controller using a single TX
>>>>> mailbox with a FIFO queue on top of it.
>>> Really? I acknowledge that it is priority based *on the bus*, i.e. if
>>> two devices A and B on the same bus try to send CAN ID 0x100 and 0x123
>>> at the same time, then device A will win the CAN arbitration.
>>> However, I am not aware of any devices which reorder their own stack
>>> according to the CAN IDs. If I first send CAN ID 0x123 and then ID
>>> 0x100 on the device stack, 0x123 would still go out first, right?
>> The CAN hardware may be a basic CAN hardware: Single mailbox only with a
>> TX FIFO on top of this.
>>
>> No reordering takes place, the CAN hardware will try to arbitrate the
>> CAN bus with a low priority CAN message (big CAN ID) while some high
>> priority CAN message (small CAN ID) is waiting in the FIFO. This is
>> called "internal priority inversion", a property of basic CAN hardware.
>> A basic CAN hardware does exactly what you describe.
>>
>> Should be the FIFO in software it's a bad idea to try to improve this
>> doing some software sorting, the processing time needed is likely to
>> make things even worse. Therefore no software does this or at least it's
>> not recommended to do this.
>>
>> But the hardware may also be a better one. No FIFO but a lot of TX
>> mailboxes. A full CAN hardware tries to arbitrate the bus using the
>> highest priority waiting CAN message considering all hardware TX
>> mailboxes. Such a better (full CAN) hardware does not cause "internal
>> priority inversion" but tries to arbitrate the bus in the correct order
>> given by the message IDs.
>>
>> We don't know about the actually used CAN hardware and how it's used on
>> this level we are with our virtio can device. We are using SocketCAN, no
>> information about the properties of the underlying hardware is provided
>> at some API. May be basic CAN using a FIFO and a single TX mailbox or
>> full CAN using a lot of TX mailboxes in parallel.
>>
>> On the bus it's guaranteed always that the sender with the lowest CAN ID
>> winds regardless which hardware is used, the only difference is whether
>> we have "internal priority inversion" or not.
>>
>> If I look at the CAN stack = Software + hardware (and not only software)
>> it's correct: The hardware device may re-order if it's a better (full
>> CAN) one and thus the actual sending on the bus is not done in the same
>> sequence as the messages were provided internally (e.g. at some socket).
> OK. Thanks for the clarification.
>
> So, you are using scatterlist to be able to interface with the
> different CAN mailboxes. But then, it means that all the heuristics to
> manage those mailboxes are done in the virtio host.

There is some heuristic when VIRTIO_CAN_F_LATE_TX_ACK is supported on 
the device side. The feature means that the host marks a TX message as 
done not at the moment when it's scheduled for sending but when it has 
been really sent on the bus.

To do that SocketCAN needs to be configured to receive it's own sent 
message. On RX the device identifies the message which has been sent on 
the bus. The heuristic is going through the list of pending messages, 
check CAN ID and payload and mark the respective message as done.

Problem with SocketCAN: There is a load case (full sending without any 
delay in both directions) where it seems that own sent messages are 
getting lost in the software stack. Thus we get in a state where the 
list of pending messages gets full and TX gets stuck.

The feature flag is not offered in the open source device, it is only 
experimental in our proprietary device and normally disabled.

Without this feature there is no heuristic, just send to SocketCAN and 
put immediately as used (done). But for an AUTOSAR CAN driver this means 
CanIf_TxConfirmation() came too early, not late when the message "has 
been transmitted on the CAN network" but already earlier when the 
message is put to SocketCAN scheduled for transmission.

> Did you consider exposing the number of supported mailboxes as a
> configuration parameter and let the virtio guest manage these? In
> Linux, it is supported since below commit:
>
>    commit 038709071328 ("can: dev: enable multi-queue for SocketCAN devices")
>    Link:https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fgit.kernel.org%2ftorvalds%2fc%2f038709071328&umid=67080c1c-b5d1-4d20-a9eb-ab7f9a062932&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-9ae22f0c43ab3effc4ba0f9fd0327c9852d5d05a
>
> Generally, from a design perspective, isn't it better to make the
> virtio host as dumb as possible and let the host do the management?

I was not aware of this patch.

But thought about different priorities. 2 priorities, low priority for 
CAN messages which may go into some FIFO suffering priority inversion 
and high priority for CAN messages going to mailboxes. The very first 
draft specification had this not knowing about some restrictions in the 
Linux environment. It had the number of places for each priority (low: 
FIFO places, high: mailboxes) in the config space. Everything going into 
a single TX queue but with some priority field. Got the comment on the 
list to use a dedicated queue for high priority messages instead of 
using a priority field in the message itself.

It would be easy to do if there was an AUTOSAR CAN driver used as back 
end, in this case you configure it and know the capabilities and 
configuration.

But looking into for example m_can.c the information is not available. 
Checked now again.

=> The information about underlying hardware properties is not available 
outside the CAN driver

And I also looked now into the patch you sent:

dev.h:

#define alloc_candev(sizeof_priv, echo_skb_max) \
     alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1)
#define alloc_candev_mq(sizeof_priv, echo_skb_max, count) \
     alloc_candev_mqs(sizeof_priv, echo_skb_max, count, count)

=> Every single driver uses alloc_candev(), none uses alloc_candev_mq(). 
So the patch which came in already 2018 is still an offering which is 
not used at all.

To have multiple priorities with queues we needed a way in user land

- to determine the number of queues (priorities)
- to address the queues
- to determine the number of resources behind each queue for flow 
control purposes
- to determine the nature of the queue (basic CAN FIFO with n places or 
full CAN queue with m mailboxes)

There is nothing of this in place.

=> We are currently not in the position to support different priority 
queues in Linux.

BTW: Even then we would probably need the heuristic on the device side 
when VIRTIO_CAN_F_LATE_TX_ACK is negotiated. I don't think it was a good 
idea to use 1 queue for basic CAN and m queues for m full CAN mailboxes, 
probably it was better to have a low priority queue and a high priority 
queue. But as there is nothing in place currently beside this patch you 
mentioned this is an issue to think about in the future.

Regards
Harald