Date:   Wed, 17 May 2023 13:43:10 +0200
From:   Harald Mommer <harald.mommer@...nsynergy.com>
To:     Vincent Mailhol <vincent.mailhol@...il.com>,
        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,
        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.

Hello,

(still sorting my E-Mail mess)

On 07.11.22 08:35, Vincent Mailhol wrote:
> 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://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds%2flinux.git%2f&umid=127a2be7-331e-4823-805f-4578232f5495&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-52b59e6b756f1b97e14cc9439116d9b7a4ad93da
>> commit/?id=1a3e3034c049503ec6992a4a7d573e7fff31fac4

1.) This is CANFD_MAX_DLEN. It is not sensible to use that define 
instead as this is a Linux define and this header here is not Linux 
specific.

BTW, a microcontroller implementation supporting CAN classic only may 
allocate only 8u bytes (CAN_MAX_DLEN) for the payload if memory usage 
was an issue. However I doubt that very small controllers are an 
audience for virtio.

The data structure has been updated in the meantime in the sources, spec 
still to be sent. The newer structure should be prepared for CAN XL 
later however CAN XL is currently not supported.

> 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?

The message type costs 2 bytes, if additional alignment is needed it 
causes a cost of 4 bytes in the worst case.

  * Virtio uses shared memory to transfer those messages which is very
    fast. From the speed perspective it costs almost nothing to have the
    message type. It's not a slow serial line where every byte sent counts.
  * The target machines for Virtio contain usually many megabytes if not
    gigabytes. The additional message identifier plays no role for those
    machines.

So the cost for the message type is relatively small. You may think 
we're always transferring CAN messages on Rxq and Txq so we can save the 
message type anyway. This is true for today and may be totally wrong 
already in a few months.

Those queues are communication channels which can transport any 
information. After the specification has been published some day someone 
may want to extend the specification having to transfer a complete 
different message over Txq or Rxq.

Easy with message type:

 1. Add a feature flag to indicate that the device now also understands
    msg_type FOO_NEW
 2. Define the structure for FOO_NEW having __le_16 msg_type as first
    member. The rest can be defined freely without any restrictions.

Without message type 2. can become ugly. The implementer is forced to 
get into the existing scheme, this may be difficult and the result may 
not look nice.

Better we spend the few bytes now to have better options in the future. 
We don't know what is in 5 years or even next year.

===

The question "why not reusing exactly the existing Linux CAN structure" 
is indeed something which has to be thought about. Cons: Virtio is not 
directly a Linux specification. Pro: It's tempting. After I've learned 
that qemu is also doing exactly this it becomes more and more tempting. 
No conclusion today.

>>> +};
>>> +
>>> +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.
True for today. Could be optimized in the specification. Resulting 
object code of the implementation would be the same 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 */

Powered by blists - more mailing lists