Re: [PATCH net-next v4 02/13] net: renesas: rswitch: use device instead of net_device

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: renesas: rswitch: use device instead of net_device

This commit updates the driver to use struct device instead of struct
net_device for queue allocation and freeing. This prepares the code for
upcoming VLAN support where the net_device might not always be available.

> diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> --- a/drivers/net/ethernet/renesas/rswitch_main.c
> +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> @@ -304,13 +304,13 @@ static int rswitch_gwca_queue_alloc_rx_buf(struct rswitch_gwca_queue *gq,
>  	return -ENOMEM;
>  }
>  
> -static void rswitch_gwca_queue_free(struct net_device *ndev,
> +static void rswitch_gwca_queue_free(struct device *dev,
>  				    struct rswitch_gwca_queue *gq)
>  {
>  	unsigned int i;
>  
>  	if (!gq->dir_tx) {
> -		dma_free_coherent(ndev->dev.parent,
> +		dma_free_coherent(dev,
>  				  sizeof(struct rswitch_ext_ts_desc) *
>  				  (gq->ring_size + 1), gq->rx_ring, gq->ring_dma);

This wasn't introduced by this patch, but if rswitch_gwca_queue_alloc()
fails during or before the dma_alloc_coherent() step, gq->rx_ring will
remain NULL.

Since dma_free_coherent() is not guaranteed to handle NULL virtual
addresses safely across all architectures, could this pass a NULL pointer
and cause a crash? Does it make sense to conditionalize the frees here
and in the tx path?

>  		gq->rx_ring = NULL;

I notice another pre-existing issue in the unquoted context just below
this. When rswitch_gwca_queue_alloc() fails, gq->rx_bufs can contain NULL
pointers.

Since the loop calls skb_free_frag(gq->rx_bufs[i]), and skb_free_frag()
wraps page_frag_free() which does not check for NULL, would this result
in an out-of-bounds page struct dereference and a kernel panic?

Would it be safer to add a NULL check before calling skb_free_frag()?

[ ... ]

> @@ -508,7 +506,7 @@ static int rswitch_gwca_queue_ext_ts_fill(struct net_device *ndev,
>  			index = (i + start_index) % gq->ring_size;
>  			desc = &gq->rx_ring[index];
>  			dma_addr = rswitch_desc_get_dptr(&desc->desc);
> -			dma_unmap_single(ndev->dev.parent, dma_addr,
> +			dma_unmap_single(dev, dma_addr,
>  					 RSWITCH_MAP_BUF_SIZE, DMA_FROM_DEVICE);
>  		}
>  	}

I noticed a concurrency issue in this pre-existing error path. Earlier in
this function, the driver transfers ownership of each descriptor to the
hardware by setting desc->desc.die_dt and executing a dma_wmb().

If a subsequent dma_map_single() fails, this error path unmaps the
previously mapped buffers but does not revoke hardware ownership by
clearing desc->desc.die_dt.

Because the hardware was already granted ownership, could it actively DMA
incoming packets into the unmapped memory before the device is finally
halted? Would it be safer to clear the ownership flag and issue a memory
barrier before unmapping the buffers?
-- 
pw-bot: cr