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