Easy to do, makes the changes smaller.
drivers/net/can/Kconfig | 1 +Since the driver is just one file, you probably don't need the subdirectory.
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 ++
+struct virtio_can_tx {Having a linked list of these appears to add a little extra complexity.
+ 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;
+};
If they are always processed in sequence, using an array would be
much simpler, as you just need to remember the index.
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.+#ifdef DEBUGThis seems to duplicate print_hex_dump(), maybe just use that?
+static void __attribute__((unused))
+virtio_can_hexdump(const void *data, size_t length, size_t base)
+{
+#define VIRTIO_CAN_MAX_BYTES_PER_LINE 16u
+A busy loop is probably not what you want here. Maybe just
+ while (!virtqueue_get_buf(vq, &len) && !virtqueue_is_broken(vq))
+ cpu_relax();
+
+ mutex_unlock(&priv->ctrl_lock);
wait_for_completion() until the callback happens?
+ /* Push loopback echo. Will be looped back on TX interrupt/TX NAPI */There should not be a need for a spinlock or disabling interrupts
+ 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);
in the xmit function. What exactly are you protecting against here?
As a further optimization, you may want to use the xmit_more()Looked elsewhere how it works and did.
function, as the virtqueue kick is fairly expensive and can be
batched here.
Not addressed, not yet completely understood.+ kfree(can_tx_msg);You may want to add netdev_sent_queue()/netdev_completed_queue()
+
+ /* Flow control */
+ if (netif_queue_stopped(dev)) {
+ netdev_info(dev, "TX ACK: Wake up stopped queue\n");
+ netif_wake_queue(dev);
+ }
based BQL flow control here as well, so you don't have to rely on the
queue filling up completely.
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.+static int virtio_can_probe(struct virtio_device *vdev)Not a useful debug check, just remove the BUG_ON(!vdev), here and elsewhere
+{
+ 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);
Yes, this thing was overall too noisy.+Also remove the prints, I assume this is left over from
+ 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);
initial debugging.
Doing so makes the code somewhat simpler and shorter = better.+Should the register_virtio_can_dev() be done here? I would expect this to be
+ register_virtio_can_dev(dev);
+
+ /* Initialize virtqueues */
+ err = virtio_can_find_vqs(priv);
+ if (err != 0)
+ goto on_failure;
the last thing after setting up the queues.
+static struct virtio_driver virtio_can_driver = {You can remove the #ifdef here and above, and replace that with the
+ .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
pm_sleep_ptr() macro in the assignment.
diff --git a/include/uapi/linux/virtio_can.h b/include/uapi/linux/virtio_can.hMaybe a link to the specification here? I assume the definitions in this file
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>
are all lifted from that document, rather than specific to the driver, right?
Arnd