Re: [PATCH v6] can: virtio: Add virtio CAN driver

[Francesco Valla <francesco@valla.it>]

On Fri, Dec 26, 2025 at 09:52:48PM +0100, Matias Ezequiel Vara Larsen wrote:
> > > > > +static int virtio_can_read_tx_queue(struct virtqueue *vq)
> > > > > +{
> > > > > +	struct virtio_can_priv *can_priv = vq->vdev->priv;
> > > > > +	struct net_device *dev = can_priv->dev;
> > > > > +	struct virtio_can_tx *can_tx_msg;
> > > > > +	struct net_device_stats *stats;
> > > > > +	unsigned long flags;
> > > > > +	unsigned int len;
> > > > > +	u8 result;
> > > > > +
> > > > > +	stats = &dev->stats;
> > > > > +
> > > > > +	/* Protect list and virtio queue operations */
> > > > > +	spin_lock_irqsave(&can_priv->tx_lock, flags);
> > > > 
> > > > The section below seems a pretty big one to protect behind a spin lock. 
> > > > 
> > > 
> > > How can I split it? 
> > > 
> > 
> > Question here is: what needs to be protected? As far as I can tell, the
> > only entity needing some kind of locking here is the queue, while both
> > ida_* and tx_inflight operations are already covered (the former by
> > design [1], the second because it's implemented using an atomic.
> > 
> > If I'm not wrong (but I might be, so please double check) this can be
> > limited to:
> > 
> > 	/* Protect queue operations */
> > 	scoped_guard(spinlock_irqsave, &priv->tx_lock)
> > 		err = virtqueue_add_sgs(vq, sgs, 1u, 1u, can_tx_msg, GFP_ATOMIC);
> > 
> > 
> > Maybe the whole locking pattern is a leftover from a previous version, 
> > where a list of TX messages was kept?
> > 
> 
> I followed this approach for the three queues. I wonder why the rx queue
> and the ctrl queue use a mutex instead of spinlock? I added a mutex for
> the operations to the rx queue.

If I interpreted correctly (but maybe Harald can shine some light on this):

- the virtio_can_send_ctrl_msg() uses a mutex because it could be
  executed in parallel by different actors (read: userspace processes
  invoking up and down operations); moreover, is the whole control
  operation that needs to be protected and not only the access to the
  virtqueue_ functions, so a mutex should make sense;
- the rx queue shouldn't actually need any locking, since it is accessed
  only by the poll function and the network framework should guarantee
  that no multiple parallel poll operations are called on a napi;
- the tx queue needs instead to be protected, since it could
  concurrently be accessed by both the start_xmit callback and the poll
  function; a spinlock there makes more sense, as accesses should be
  very short.

> 
> Matias
> 
> 

Thank you

Regards,
Francesco