From: Vincent Mailhol <vincent.mailhol@gmail.com>
To: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: virtio-dev@lists.oasis-open.org, linux-can@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Harald Mommer <Harald.Mommer@opensynergy.com>,
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>,
Damir Shaikhutdinov <Damir.Shaikhutdinov@opensynergy.com>
Subject: Re: [RFC PATCH v2 1/2] can: virtio: Initial virtio CAN driver.
Date: Mon, 7 Nov 2022 16:35:22 +0900 [thread overview]
Message-ID: <CAMZ6RqLALOYFWQJ4C4HTaRw7y-waUbqOX0WzrWVNiQG51QexHw@mail.gmail.com> (raw)
In-Reply-To: <4782632.31r3eYUQgx@steina-w>
On Mon. 7 Nov. 2022 at 16:15, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
> Am Freitag, 4. November 2022, 18:24:20 CET schrieb Harald Mommer:
> > From: Harald Mommer <harald.mommer@opensynergy.com>
> >
> > - 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.
> >
> > This is version 2 of the driver after having gotten review comments.
> >
> > Signed-off-by: Harald Mommer <Harald.Mommer@opensynergy.com>
> > Cc: Damir Shaikhutdinov <Damir.Shaikhutdinov@opensynergy.com>
[...]
> > 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];
>
> 64u is CANFD_MAX_DLEN, right? Is it sensible to use that define instead?
> I guess if CAN XL support is to be added at some point, a dedicated struct is
> needed, to fit for CANXL_MAX_DLEN (2048 [1]).
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> commit/?id=1a3e3034c049503ec6992a4a7d573e7fff31fac4
To add to Alexander's comment, what is the reason to have the msg_type
flag? The struct can_frame, canfd_frame and canxl_frame are done such
that it is feasible to decide the type (Classic, FD, XL) from the
content of the structure. Why not just reusing the current structures?
> > +};
> > +
> > +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];
> > +};
>
> I have no experience with virtio drivers, but is there a need for dedicated
> structs for Tx and Rx? They are identical anyway.
>
> Best regards,
> Alexander
>
> > +
> > +/* 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 */
next prev parent reply other threads:[~2022-11-07 7:35 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-04 17:24 [RFC PATCH v2 0/2] can: virtio: virtio-can driver Harald Mommer
2022-11-04 17:24 ` [RFC PATCH v2 1/2] can: virtio: Initial virtio CAN driver Harald Mommer
2022-11-05 9:11 ` Vincent Mailhol
2022-11-05 10:11 ` Vincent Mailhol
2022-11-07 7:09 ` Alexander Stein
2022-11-07 7:35 ` Vincent Mailhol [this message]
2023-05-17 11:43 ` Harald Mommer
2022-11-04 17:24 ` [RFC PATCH v2 2/2] can: virtio: Add virtio_can to MAINTAINERS file Harald Mommer
2022-11-05 10:14 ` Vincent Mailhol
2022-11-08 19:13 ` Harald Mommer
2022-11-08 19:22 ` Harald Mommer
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=CAMZ6RqLALOYFWQJ4C4HTaRw7y-waUbqOX0WzrWVNiQG51QexHw@mail.gmail.com \
--to=vincent.mailhol@gmail.com \
--cc=Damir.Shaikhutdinov@opensynergy.com \
--cc=Dariusz.Stojaczyk@opensynergy.com \
--cc=Harald.Mommer@opensynergy.com \
--cc=alexander.stein@ew.tq-group.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.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=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).