Re: [PATCH net-next v5 02/14] 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
    
In upcomming changes for adding vlan support struct net_device
will not be available in all cases, therefore use struct device
instead.

> diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> index 9b739594cc028..74d441ae19769 100644
> --- a/drivers/net/ethernet/renesas/rswitch_main.c
> +++ b/drivers/net/ethernet/renesas/rswitch_main.c
[ ... ]
> @@ -304,26 +304,30 @@ 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,
> -				  sizeof(struct rswitch_ext_ts_desc) *
> -				  (gq->ring_size + 1), gq->rx_ring, gq->ring_dma);
> -		gq->rx_ring = NULL;
> -
> +		if (gq->rx_ring) {
> +			dma_free_coherent(dev,
> +					  sizeof(struct rswitch_ext_ts_desc) *
> +					  (gq->ring_size + 1), gq->rx_ring, gq->ring_dma);
> +			gq->rx_ring = NULL;
> +		}
>  		for (i = 0; i < gq->ring_size; i++)
> -			skb_free_frag(gq->rx_bufs[i]);
> +			if (gq->rx_bufs[i])
> +				skb_free_frag(gq->rx_bufs[i]);

[Severity: High]
This is a pre-existing issue, but does this cleanup path leak RX DMA
mappings?

It looks like we call skb_free_frag() for the RX buffers but we never call
dma_unmap_single(). Furthermore, because the rx_ring is freed before the
loop, any DMA addresses stored in the descriptors are lost, making it
impossible to unmap them.

>  		kfree(gq->rx_bufs);
>  		gq->rx_bufs = NULL;
>  	} else {
> -		dma_free_coherent(ndev->dev.parent,
> -				  sizeof(struct rswitch_ext_desc) *
> -				  (gq->ring_size + 1), gq->tx_ring, gq->ring_dma);
> -		gq->tx_ring = NULL;
> +		if (gq->tx_ring) {
> +			dma_free_coherent(dev,
> +					  sizeof(struct rswitch_ext_desc) *
> +					  (gq->ring_size + 1), gq->tx_ring, gq->ring_dma);
> +			gq->tx_ring = NULL;
> +		}
>  		kfree(gq->skbs);
>  		gq->skbs = NULL;
>  		kfree(gq->unmap_addrs);

[Severity: High]
This isn't a bug introduced by this patch, but does the TX teardown also
leak resources?

It appears it does not iterate over pending gq->skbs to unmap and free
them before freeing the array itself.

[ ... ]
> @@ -483,18 +486,17 @@ static int rswitch_gwca_queue_ext_ts_fill(struct net_device *ndev,
>  		index = (i + start_index) % gq->ring_size;
>  		desc = &gq->rx_ring[index];
>  		if (!gq->dir_tx) {
> -			dma_addr = dma_map_single(ndev->dev.parent,
> +			dma_addr = dma_map_single(dev,
>  						  gq->rx_bufs[index] + RSWITCH_HEADROOM,
>  						  RSWITCH_MAP_BUF_SIZE,
>  						  DMA_FROM_DEVICE);
> -			if (dma_mapping_error(ndev->dev.parent, dma_addr))
> +			if (dma_mapping_error(dev, dma_addr))
>  				goto err;
>  
>  			desc->desc.info_ds = cpu_to_le16(RSWITCH_DESC_BUF_SIZE);
>  			rswitch_desc_set_dptr(&desc->desc, dma_addr);
>  			dma_wmb();
>  			desc->desc.die_dt = DT_FEMPTY | DIE;
> -			desc->info1 = cpu_to_le64(INFO1_SPN(rdev->etha->index));
>  		} else {
>  			desc->desc.die_dt = DT_EEMPTY | DIE;
>  		}
> @@ -507,8 +509,10 @@ static int rswitch_gwca_queue_ext_ts_fill(struct net_device *ndev,
>  		for (; i-- > 0; ) {
>  			index = (i + start_index) % gq->ring_size;
>  			desc = &gq->rx_ring[index];
> +			desc->desc.die_dt = 0x00;
> +			dma_wmb();
>  			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);
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but is there a hardware race condition in
this error path?

Earlier in the loop, buffers are mapped and immediately exposed to the
running hardware by setting desc->desc.die_dt = DT_FEMPTY | DIE. If a
subsequent allocation in the batch fails, we jump to this error path to
revoke previously armed descriptors by clearing die_dt and calling
dma_unmap_single().

If the active hardware has already fetched the descriptor and started a
DMA write before we revoke it, will unmapping it lead to IOMMU faults or
memory corruption?

Should the descriptors only be armed after all allocations and mappings in
the batch have succeeded?