Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221104172421.8271-1-Harald.Mommer@opensynergy.com>
 <20221104172421.8271-2-Harald.Mommer@opensynergy.com> <4782632.31r3eYUQgx@steina-w>
In-Reply-To: <4782632.31r3eYUQgx@steina-w>
From:   Vincent Mailhol <vincent.mailhol@gmail.com>
Date:   Mon, 7 Nov 2022 16:35:22 +0900
Message-ID: <CAMZ6RqLALOYFWQJ4C4HTaRw7y-waUbqOX0WzrWVNiQG51QexHw@mail.gmail.com>
Subject: Re: [RFC PATCH v2 1/2] can: virtio: Initial virtio CAN driver.
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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 */