linux-kernel - Re: [RFC PATCH v2 1/2] can: virtio: Initial virtio CAN driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Nov 2022 16:35:22 +0900
From:   Vincent Mailhol <vincent.mailhol@...il.com>
To:     Alexander Stein <alexander.stein@...tq-group.com>
Cc:     virtio-dev@...ts.oasis-open.org, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Harald Mommer <Harald.Mommer@...nsynergy.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Dariusz Stojaczyk <Dariusz.Stojaczyk@...nsynergy.com>,
        Damir Shaikhutdinov <Damir.Shaikhutdinov@...nsynergy.com>
Subject: Re: [RFC PATCH v2 1/2] can: virtio: Initial virtio CAN driver.

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