Re: [PATCH net] ethernet: alt1: Fix missing DMA mapping tests

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 16 Jun 2025 15:59:55 +0200 Thomas Fourier wrote:
> According to Shuah Khan[1], all `dma_map()` functions should be tested
> before using the pointer.  This patch checks for errors after all
> `dma_map()` calls.

The link and reference to Shuah's presentation is not necessary.
Just say that the DMA functions can fail so we need the error check.
 
> In `atl1_alloc_rx_buffers()`, when the dma mapping fails,
> the buffer is deallocated ans marked as such.
> 
> In `atl1_tx_map()`, the previously dma_mapped buffers are de-mapped and an
> error is returned.
> 
> [1] https://events.static.linuxfound.org/sites/events/files/slides/Shuah_Khan_dma_map_error.pdf
>
> Fixes: f3cc28c79760 ("Add Attansic L1 ethernet driver.")
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>

Please don't send the new versions in reply to old ones.
For those of us who use their inbox as a FIFO of pending submissions
it messes up ordering.

> diff --git a/drivers/net/ethernet/atheros/atlx/atl1.c b/drivers/net/ethernet/atheros/atlx/atl1.c
> index cfdb546a09e7..dd0e01d3a023 100644
> --- a/drivers/net/ethernet/atheros/atlx/atl1.c
> +++ b/drivers/net/ethernet/atheros/atlx/atl1.c
> @@ -1869,6 +1869,14 @@ static u16 atl1_alloc_rx_buffers(struct atl1_adapter *adapter)
>  		buffer_info->dma = dma_map_page(&pdev->dev, page, offset,
>  						adapter->rx_buffer_len,
>  						DMA_FROM_DEVICE);
> +		if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
> +			buffer_info->alloced = 0;
> +			buffer_info->skb = NULL;
> +			buffer_info->dma = 0;

maybe you could move the map call a little further up, before all the
buffer_info fields are populated? So we dont have to clean them up?

> +			kfree_skb(skb);
> +			adapter->soft_stats.rx_dropped++;
> +			break;
> +		}
>  		rfd_desc->buffer_addr = cpu_to_le64(buffer_info->dma);
>  		rfd_desc->buf_len = cpu_to_le16(adapter->rx_buffer_len);
>  		rfd_desc->coalese = 0;
> @@ -2183,8 +2191,8 @@ static int atl1_tx_csum(struct atl1_adapter *adapter, struct sk_buff *skb,
>  	return 0;
>  }
>  
> -static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
> -	struct tx_packet_desc *ptpd)
> +static int atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
> +		       struct tx_packet_desc *ptpd)
>  {
>  	struct atl1_tpd_ring *tpd_ring = &adapter->tpd_ring;
>  	struct atl1_buffer *buffer_info;
> @@ -2194,6 +2202,7 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
>  	unsigned int nr_frags;
>  	unsigned int f;
>  	int retval;
> +	u16 first_mapped;
>  	u16 next_to_use;
>  	u16 data_len;
>  	u8 hdr_len;
> @@ -2201,6 +2210,7 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
>  	buf_len -= skb->data_len;
>  	nr_frags = skb_shinfo(skb)->nr_frags;
>  	next_to_use = atomic_read(&tpd_ring->next_to_use);
> +	first_mapped = next_to_use;
>  	buffer_info = &tpd_ring->buffer_info[next_to_use];
>  	BUG_ON(buffer_info->skb);
>  	/* put skb in last TPD */
> @@ -2216,6 +2226,8 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
>  		buffer_info->dma = dma_map_page(&adapter->pdev->dev, page,
>  						offset, hdr_len,
>  						DMA_TO_DEVICE);
> +		if (dma_mapping_error(&adapter->pdev->dev, buffer_info->dma))
> +			goto dma_err;
>  
>  		if (++next_to_use == tpd_ring->count)
>  			next_to_use = 0;
> @@ -2242,6 +2254,9 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
>  								page, offset,
>  								buffer_info->length,
>  								DMA_TO_DEVICE);
> +				if (dma_mapping_error(&adapter->pdev->dev,
> +						      buffer_info->dma))
> +					goto dma_err;
>  				if (++next_to_use == tpd_ring->count)
>  					next_to_use = 0;
>  			}
> @@ -2254,6 +2269,8 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
>  		buffer_info->dma = dma_map_page(&adapter->pdev->dev, page,
>  						offset, buf_len,
>  						DMA_TO_DEVICE);
> +		if (dma_mapping_error(&adapter->pdev->dev, buffer_info->dma))
> +			goto dma_err;
>  		if (++next_to_use == tpd_ring->count)
>  			next_to_use = 0;
>  	}
> @@ -2277,6 +2294,9 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
>  			buffer_info->dma = skb_frag_dma_map(&adapter->pdev->dev,
>  				frag, i * ATL1_MAX_TX_BUF_LEN,
>  				buffer_info->length, DMA_TO_DEVICE);
> +			if (dma_mapping_error(&adapter->pdev->dev,
> +					      buffer_info->dma))
> +				goto dma_err;
>  
>  			if (++next_to_use == tpd_ring->count)
>  				next_to_use = 0;
> @@ -2285,6 +2305,22 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
>  
>  	/* last tpd's buffer-info */
>  	buffer_info->skb = skb;
> +
> +	return 0;
> +
> + dma_err:
> +	while (first_mapped != next_to_use) {
> +		buffer_info = &tpd_ring->buffer_info[first_mapped];
> +		dma_unmap_page(&adapter->pdev->dev,
> +			       buffer_info->dma,
> +			       buffer_info->length,
> +			       DMA_TO_DEVICE);
> +		buffer_info->dma = 0;
> +
> +		if (++first_mapped == tpd_ring->count)
> +			first_mapped = 0;
> +	}
> +	return -ENOMEM;
>  }
>  
>  static void atl1_tx_queue(struct atl1_adapter *adapter, u16 count,
> @@ -2419,7 +2455,8 @@ static netdev_tx_t atl1_xmit_frame(struct sk_buff *skb,
>  		}
>  	}
>  
> -	atl1_tx_map(adapter, skb, ptpd);
> +	if (atl1_tx_map(adapter, skb, ptpd))
> +		return NETDEV_TX_BUSY;

You should drop the packet. Occasional packet loss is better than
having a packet that we can't map for some platform reasons blocking
the queue

>  	atl1_tx_queue(adapter, count, ptpd);
>  	atl1_update_mailbox(adapter);
>  	return NETDEV_TX_OK;