linux-kernel - Re: [PATCH v5] 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] [day] [month] [year] [list]
Message-ID: <aO0/1oCiuOgxZMrg@fedora>
Date: Mon, 13 Oct 2025 20:07:18 +0200
From: Matias Ezequiel Vara Larsen <mvaralar@...hat.com>
To: Francesco Valla <francesco@...la.it>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, Paolo Abeni <pabeni@...hat.com>,
	Harald Mommer <harald.mommer@...nsynergy.com>,
	Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@...nsynergy.com>,
	Wolfgang Grandegger <wg@...ndegger.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Jason Wang <jasowang@...hat.com>,
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
	Damir Shaikhutdinov <Damir.Shaikhutdinov@...nsynergy.com>,
	linux-kernel@...r.kernel.org, linux-can@...r.kernel.org,
	netdev@...r.kernel.org, virtualization@...ts.linux.dev,
	development@...aril.me
Subject: Re: [PATCH v5] can: virtio: Initial virtio CAN driver.

On Mon, Oct 13, 2025 at 06:29:44PM +0200, Francesco Valla wrote:
> Hello Matias,
> 
> On Monday, 13 October 2025 at 11:52:49 Matias Ezequiel Vara Larsen <mvaralar@...hat.com> wrote:
> > On Fri, Oct 10, 2025 at 11:20:22PM +0200, Francesco Valla wrote:
> > > On Friday, 10 October 2025 at 17:46:25 Matias Ezequiel Vara Larsen <mvaralar@...hat.com> wrote:
> > > > On Thu, Sep 11, 2025 at 10:59:40PM +0200, Francesco Valla wrote:
> > > > > Hello Mikhail, Harald,
> > > > > 
> > > > > hoping there will be a v6 of this patch soon, a few comments:
> > > > > 
> > > > 
> > > > I am working on the v6 by addressing the comments in this thread.
> > > > 
> > > > > On Monday, 8 January 2024 at 14:10:35 Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@...nsynergy.com> wrote:
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > +
> > > > > > +/* virtio_can private data structure */
> > > > > > +struct virtio_can_priv {
> > > > > > +	struct can_priv can;	/* must be the first member */
> > > > > > +	/* NAPI for RX messages */
> > > > > > +	struct napi_struct napi;
> > > > > > +	/* NAPI for TX messages */
> > > > > > +	struct napi_struct napi_tx;
> > > > > > +	/* The network device we're associated with */
> > > > > > +	struct net_device *dev;
> > > > > > +	/* The virtio device we're associated with */
> > > > > > +	struct virtio_device *vdev;
> > > > > > +	/* The virtqueues */
> > > > > > +	struct virtqueue *vqs[VIRTIO_CAN_QUEUE_COUNT];
> > > > > > +	/* I/O callback function pointers for the virtqueues */
> > > > > > +	vq_callback_t *io_callbacks[VIRTIO_CAN_QUEUE_COUNT];
> > > > > > +	/* Lock for TX operations */
> > > > > > +	spinlock_t tx_lock;
> > > > > > +	/* Control queue lock. Defensive programming, may be not needed */
> > > > > > +	struct mutex ctrl_lock;
> > > > > > +	/* Wait for control queue processing without polling */
> > > > > > +	struct completion ctrl_done;
> > > > > > +	/* List of virtio CAN TX message */
> > > > > > +	struct list_head tx_list;
> > > > > > +	/* Array of receive queue messages */
> > > > > > +	struct virtio_can_rx rpkt[128];
> > > > > 
> > > > > This array should probably be allocated dynamically at probe - maybe
> > > > > using a module parameter instead of a hardcoded value as length? 
> > > > > 
> > > > 
> > > > If I allocate this array in probe(), I would not know sdu[] in advance
> > > > if I defined it as a flexible array. That made me wonder: can sdu[] be
> > > > defined as flexible array for rx? 
> > > > 
> > > > Thanks.
> > > > 
> > > 
> > > One thing that can be done is to define struct virtio_can_rx as:
> > > 
> > > struct virtio_can_rx {
> > > #define VIRTIO_CAN_RX                   0x0101
> > > 	__le16 msg_type;
> > > 	__le16 length; /* 0..8 CC, 0..64 CAN-FD, 0..2048 CAN-XL, 12 bits */
> > > 	__u8 reserved_classic_dlc; /* If CAN classic length = 8 then DLC can be 8..15 */
> > > 	__u8 padding;
> > > 	__le16 reserved_xl_priority; /* May be needed for CAN XL priority */
> > > 	__le32 flags;
> > > 	__le32 can_id;
> > > 	__u8 sdu[] __counted_by(length);
> > > };
> > > 
> > > and then allocate the rpkt[] array using the maximum length for SDU:
> > > 
> > > priv->rpkt = kcalloc(num_rx_buffers,
> > > 		sizeof(struct virtio_can_rx) + VIRTIO_CAN_MAX_DLEN,
> > > 		GFP_KERNEL);
> > > 
> > > In this way, the size of each member of rpkt[] is known and is thus
> > > suitable for virtio_can_populate_vqs().
> > > 
> > > 
> > 
> > Thanks for your answer. What is the value of VIRTIO_CAN_MAX_DLEN? I
> > can't find it nor in the code or in the spec. I guess is 64 bytes? Also,
> > IIUC, using __counted_by() would not end up saving space but adding an
> > extra check for the compiler. Am I right? In that case, can't I just use
> > a fixed array of VIRTIO_CAN_MAX_DLEN bytes?
> 
> My bad, I forgot to say that VIRTIO_CAN_MAX_DLEN has to be defined, but:
> given some more thoughts, maybe this can be a dynamic value based on
> the features received from the virtio framework, to avoid wasting memory?
> 
> E.g.:
> 
> if (virtio_has_feature(VIRTIO_CAN_F_CAN_FD))
> 	sdu_len = CANFD_MAX_DLEN;
> else
> 	sdu_len = CAN_MAX_DLEN;
> 
> priv->rpkt = kcalloc(num_rx_buffers, sizeof(struct virtio_can_rx) + sdu_len,
> 		GFP_KERNEL);
> 
> 
> My understanding of __counted_by() is the same: additional checks but nothing
> more.
> 
> 
> CAN-XL appears to be not supported by the virtio specs v1.4 [1], but if/when
> it will be, the addition of an additional case would be simple.
> 
> [1] https://github.com/oasis-tcs/virtio-spec/blob/virtio-1.4/device-types/can/description.tex#L33
> 

Sounds good, I'll add that.

Matias