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@xxxxxxxxxxxxxxx/
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@xxxxxxxxxxxxxxx
www.opensynergy.com
Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah
