From: Arnd Bergmann <arnd@kernel.org>
To: Harald Mommer <harald.mommer@opensynergy.com>
Cc: virtio-dev@lists.oasis-open.org, linux-can@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Wolfgang Grandegger <wg@grandegger.com>,
Marc Kleine-Budde <mkl@pengutronix.de>,
"David S . Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Dariusz Stojaczyk <Dariusz.Stojaczyk@opensynergy.com>,
Harald Mommer <hmo@opensynergy.com>,
Stratos Mailing List <stratos-dev@op-lists.linaro.org>
Subject: Re: [virtio-dev] [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver.
Date: Thu, 25 Aug 2022 20:21:06 +0200 [thread overview]
Message-ID: <CAK8P3a1biW1qygRS8Mf0F5n8e6044+W-5v+Gnv+gh+Cyzj-Vjg@mail.gmail.com> (raw)
In-Reply-To: <20220825134449.18803-1-harald.mommer@opensynergy.com>
() 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
next prev parent reply other threads:[~2022-08-25 18:21 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-25 13:44 [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver Harald Mommer
2022-08-25 18:21 ` Arnd Bergmann [this message]
2022-11-03 12:26 ` [virtio-dev] " Harald Mommer
2022-11-04 10:50 ` Arnd Bergmann
2022-11-05 9:21 ` Vincent Mailhol
2023-05-12 13:19 ` Harald Mommer
2023-05-15 5:58 ` Vincent Mailhol
2023-05-23 13:39 ` Harald Mommer
2022-08-27 9:02 ` Oliver Hartkopp
2022-11-03 13:02 ` Harald Mommer
2022-08-27 9:39 ` Marc Kleine-Budde
2022-08-27 11:12 ` Oliver Hartkopp
2022-11-03 13:55 ` Harald Mommer
2022-11-04 15:32 ` [virtio-dev] " Jan Kiszka
2022-11-04 17:03 ` Arnd Bergmann
2023-02-03 15:02 ` Harald Mommer
2023-04-14 19:20 ` Marc Kleine-Budde
2023-04-18 9:50 ` Harald Mommer
2023-04-18 12:06 ` Marc Kleine-Budde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAK8P3a1biW1qygRS8Mf0F5n8e6044+W-5v+Gnv+gh+Cyzj-Vjg@mail.gmail.com \
--to=arnd@kernel.org \
--cc=Dariusz.Stojaczyk@opensynergy.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=harald.mommer@opensynergy.com \
--cc=hmo@opensynergy.com \
--cc=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=stratos-dev@op-lists.linaro.org \
--cc=virtio-dev@lists.oasis-open.org \
--cc=wg@grandegger.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).