On Mon. 7 Nov. 2022 at 16:15, Alexander Stein
<alexander.stein@xxxxxxxxxxxxxxx> wrote:
> Am Freitag, 4. November 2022, 18:24:20 CET schrieb Harald Mommer:
> > From: Harald Mommer <harald.mommer@xxxxxxxxxxxxxxx>
> >
> > - 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@xxxxxxxxxxxxxxx>
> > Cc: Damir Shaikhutdinov <Damir.Shaikhutdinov@xxxxxxxxxxxxxxx>
[...]
> > 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 */
