Re: [PATCH net 2/2] virtio_net: unmap AF_XDP header with tx virtqueue

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
virtio_net: unmap AF_XDP header with tx virtqueue

The patch changes the error unwind in virtnet_xsk_pool_enable() so that the
shared AF_XDP transmit header is unmapped with sq->vq instead of rq->vq,
matching the virtqueue used for the mapping.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6160aa8ba595e..a6f90f579b6e8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -5959,7 +5959,7 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
>  err_rq:
>  	xsk_pool_dma_unmap(pool, 0);
>  err_xsk_map:
> -	virtqueue_unmap_single_attrs(rq->vq, hdr_dma, vi->hdr_len,
> +	virtqueue_unmap_single_attrs(sq->vq, hdr_dma, vi->hdr_len,
>  				     DMA_TO_DEVICE, 0);

[Severity: Low]

The changelog says "Use sq->vq on the setup error path as well so the DMA
unmap matches the mapping side", which reads like a real wrong-virtqueue
unmap is being fixed.  Is there a configuration where the old rq->vq unmap
actually released a different mapping?

Earlier in virtnet_xsk_pool_enable() the function bails out unless both
virtqueues share one non-NULL dma dev:

	if (virtqueue_dma_dev(rq->vq) != virtqueue_dma_dev(sq->vq))
		return -EINVAL;

	dma_dev = virtqueue_dma_dev(rq->vq);
	if (!dma_dev)
		return -EINVAL;

So on any path that can reach err_xsk_map, rq->vq and sq->vq have the same
dma dev.  virtqueue_dma_dev() only returns non-NULL when the device has no
map ops, and virtqueue_unmap_single_attrs() in drivers/virtio/virtio_ring.c
then only looks at the per-device flag before going to
virtqueue_unmap_page_attrs():

	if (!vq->use_map_api)
		return;

	virtqueue_unmap_page_attrs(_vq, addr, size, dir, attrs);

use_map_api is set from vring_use_map_api(vdev), so it is the same for both
queues too.  With that, the emitted dma_unmap_page_attrs() call is identical
before and after the patch (same device, address, size and direction).

Would it be clearer to state "no functional change intended" and explain
that the two virtqueues are equivalent today, so this is a symmetry cleanup
that only starts to matter if the dma dev equality precondition is relaxed?
Alternatively, if a reachable mismatch does exist, could the changelog name
that configuration and carry a Fixes: tag?  As written, the message with no
Fixes: tag can still be picked up as a DMA fix by stable/AUTOSEL tooling.