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 > <[email protected]> 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 > > > <[email protected]> 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? Thanks, Matias.
