[PATCH] (drivers/ethernet/atheros/atl1) test DMA mapping for error code

[PATCH] (drivers/ethernet/atheros/atl1) test DMA mapping for error code

[Thomas Fourier]

According to Shuah Khan[1], all `dma_map()` functions should be tested
before using the pointer. This patch checks for errors in `dma_map()`
calls and in case of failure, unmaps the previously dma_mapped regions
and returns an error.

[1] https://events.static.linuxfound.org/sites/events/files/slides/Shuah_Khan_dma_map_error.pdf

Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
---
 drivers/net/ethernet/atheros/atlx/atl1.c | 38 ++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/atheros/atlx/atl1.c b/drivers/net/ethernet/atheros/atlx/atl1.c
index cfdb546a09e7..99d7a37b4ddf 100644
--- a/drivers/net/ethernet/atheros/atlx/atl1.c
+++ b/drivers/net/ethernet/atheros/atlx/atl1.c
@@ -1869,6 +1869,13 @@ 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;
+			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,7 +2190,7 @@ 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,
+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;
@@ -2195,12 +2202,14 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
 	unsigned int f;
 	int retval;
 	u16 next_to_use;
+	u16 first_mapped;
 	u16 data_len;
 	u8 hdr_len;
 
 	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 +2225,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 +2253,8 @@ 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 +2267,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 +2292,8 @@ 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 +2302,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 = NULL;
+
+		if (++first_mapped == tdp_ring->count)
+			first_mapped = 0;
+	}
+	return -ENOMEM;
 }
 
 static void atl1_tx_queue(struct atl1_adapter *adapter, u16 count,
@@ -2419,7 +2452,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;
 	atl1_tx_queue(adapter, count, ptpd);
 	atl1_update_mailbox(adapter);
 	return NETDEV_TX_OK;
-- 
2.43.0

Re: [PATCH] (drivers/ethernet/atheros/atl1) test DMA mapping for error code

[Charalampos Mitrodimas]

Thomas Fourier <fourier.thomas@gmail.com> writes:

> According to Shuah Khan[1], all `dma_map()` functions should be tested
> before using the pointer. This patch checks for errors in `dma_map()`
> calls and in case of failure, unmaps the previously dma_mapped regions
> and returns an error.
>
> [1] https://events.static.linuxfound.org/sites/events/files/slides/Shuah_Khan_dma_map_error.pdf
>
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> ---
>  drivers/net/ethernet/atheros/atlx/atl1.c | 38 ++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> [...]

Hi Thomas,

This doesn't seem to build. You can also see it here[1].

[1]: https://patchwork.kernel.org/project/netdevbpf/patch/20250612150542.85239-2-fourier.thomas@gmail.com/

Re: [PATCH] (drivers/ethernet/atheros/atl1) test DMA mapping for error code

[Jakub Kicinski]

On Thu, 12 Jun 2025 17:24:02 +0000 Charalampos Mitrodimas wrote:
> This doesn't seem to build. You can also see it here[1].
> 
> [1]: https://patchwork.kernel.org/project/netdevbpf/patch/20250612150542.85239-2-fourier.thomas@gmail.com/

Please don't share links to the patchwork checks.
Confirm its not a false positive and copy the output into the email.
Patchwork checks are *not* a public CI for people to yolo their
untested code into. Sharing links may give such impression.

Re: [PATCH] (drivers/ethernet/atheros/atl1) test DMA mapping for error code

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.16-rc1 next-20250612]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Fourier/drivers-ethernet-atheros-atl1-test-DMA-mapping-for-error-code/20250612-231733
base:   linus/master
patch link:    https://lore.kernel.org/r/20250612150542.85239-2-fourier.thomas%40gmail.com
patch subject: [PATCH] (drivers/ethernet/atheros/atl1) test DMA mapping for error code
config: x86_64-rhel-9.4 (https://download.01.org/0day-ci/archive/20250613/202506131458.MnU50xNO-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250613/202506131458.MnU50xNO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506131458.MnU50xNO-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/net/ethernet/atheros/atlx/atl1.c: In function 'atl1_tx_map':
>> drivers/net/ethernet/atheros/atlx/atl1.c:2315:34: warning: assignment to 'dma_addr_t' {aka 'long long unsigned int'} from 'void *' makes integer from pointer without a cast [-Wint-conversion]
    2315 |                 buffer_info->dma = NULL;
         |                                  ^
>> drivers/net/ethernet/atheros/atlx/atl1.c:2317:39: error: 'tdp_ring' undeclared (first use in this function); did you mean 'tpd_ring'?
    2317 |                 if (++first_mapped == tdp_ring->count)
         |                                       ^~~~~~~~
         |                                       tpd_ring
   drivers/net/ethernet/atheros/atlx/atl1.c:2317:39: note: each undeclared identifier is reported only once for each function it appears in


vim +2317 drivers/net/ethernet/atheros/atlx/atl1.c

  2192	
  2193	static int atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
  2194		struct tx_packet_desc *ptpd)
  2195	{
  2196		struct atl1_tpd_ring *tpd_ring = &adapter->tpd_ring;
  2197		struct atl1_buffer *buffer_info;
  2198		u16 buf_len = skb->len;
  2199		struct page *page;
  2200		unsigned long offset;
  2201		unsigned int nr_frags;
  2202		unsigned int f;
  2203		int retval;
  2204		u16 next_to_use;
  2205		u16 first_mapped;
  2206		u16 data_len;
  2207		u8 hdr_len;
  2208	
  2209		buf_len -= skb->data_len;
  2210		nr_frags = skb_shinfo(skb)->nr_frags;
  2211		next_to_use = atomic_read(&tpd_ring->next_to_use);
  2212		first_mapped = next_to_use;
  2213		buffer_info = &tpd_ring->buffer_info[next_to_use];
  2214		BUG_ON(buffer_info->skb);
  2215		/* put skb in last TPD */
  2216		buffer_info->skb = NULL;
  2217	
  2218		retval = (ptpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK;
  2219		if (retval) {
  2220			/* TSO */
  2221			hdr_len = skb_tcp_all_headers(skb);
  2222			buffer_info->length = hdr_len;
  2223			page = virt_to_page(skb->data);
  2224			offset = offset_in_page(skb->data);
  2225			buffer_info->dma = dma_map_page(&adapter->pdev->dev, page,
  2226							offset, hdr_len,
  2227							DMA_TO_DEVICE);
  2228			if (dma_mapping_error(&adapter->pdev->dev, buffer_info->dma))
  2229				goto dma_err;
  2230	
  2231			if (++next_to_use == tpd_ring->count)
  2232				next_to_use = 0;
  2233	
  2234			if (buf_len > hdr_len) {
  2235				int i, nseg;
  2236	
  2237				data_len = buf_len - hdr_len;
  2238				nseg = (data_len + ATL1_MAX_TX_BUF_LEN - 1) /
  2239					ATL1_MAX_TX_BUF_LEN;
  2240				for (i = 0; i < nseg; i++) {
  2241					buffer_info =
  2242					    &tpd_ring->buffer_info[next_to_use];
  2243					buffer_info->skb = NULL;
  2244					buffer_info->length =
  2245					    (ATL1_MAX_TX_BUF_LEN >=
  2246					     data_len) ? ATL1_MAX_TX_BUF_LEN : data_len;
  2247					data_len -= buffer_info->length;
  2248					page = virt_to_page(skb->data +
  2249						(hdr_len + i * ATL1_MAX_TX_BUF_LEN));
  2250					offset = offset_in_page(skb->data +
  2251						(hdr_len + i * ATL1_MAX_TX_BUF_LEN));
  2252					buffer_info->dma = dma_map_page(&adapter->pdev->dev,
  2253									page, offset,
  2254									buffer_info->length,
  2255									DMA_TO_DEVICE);
  2256					if (dma_mapping_error(&adapter->pdev->dev, buffer_info->dma))
  2257						goto dma_err;
  2258					if (++next_to_use == tpd_ring->count)
  2259						next_to_use = 0;
  2260				}
  2261			}
  2262		} else {
  2263			/* not TSO */
  2264			buffer_info->length = buf_len;
  2265			page = virt_to_page(skb->data);
  2266			offset = offset_in_page(skb->data);
  2267			buffer_info->dma = dma_map_page(&adapter->pdev->dev, page,
  2268							offset, buf_len,
  2269							DMA_TO_DEVICE);
  2270			if (dma_mapping_error(&adapter->pdev->dev, buffer_info->dma))
  2271				goto dma_err;
  2272			if (++next_to_use == tpd_ring->count)
  2273				next_to_use = 0;
  2274		}
  2275	
  2276		for (f = 0; f < nr_frags; f++) {
  2277			const skb_frag_t *frag = &skb_shinfo(skb)->frags[f];
  2278			u16 i, nseg;
  2279	
  2280			buf_len = skb_frag_size(frag);
  2281	
  2282			nseg = (buf_len + ATL1_MAX_TX_BUF_LEN - 1) /
  2283				ATL1_MAX_TX_BUF_LEN;
  2284			for (i = 0; i < nseg; i++) {
  2285				buffer_info = &tpd_ring->buffer_info[next_to_use];
  2286				BUG_ON(buffer_info->skb);
  2287	
  2288				buffer_info->skb = NULL;
  2289				buffer_info->length = (buf_len > ATL1_MAX_TX_BUF_LEN) ?
  2290					ATL1_MAX_TX_BUF_LEN : buf_len;
  2291				buf_len -= buffer_info->length;
  2292				buffer_info->dma = skb_frag_dma_map(&adapter->pdev->dev,
  2293					frag, i * ATL1_MAX_TX_BUF_LEN,
  2294					buffer_info->length, DMA_TO_DEVICE);
  2295				if (dma_mapping_error(&adapter->pdev->dev, buffer_info->dma))
  2296					goto dma_err;
  2297	
  2298				if (++next_to_use == tpd_ring->count)
  2299					next_to_use = 0;
  2300			}
  2301		}
  2302	
  2303		/* last tpd's buffer-info */
  2304		buffer_info->skb = skb;
  2305	
  2306		return 0;
  2307	
  2308	 dma_err:
  2309		while (first_mapped != next_to_use) {
  2310			buffer_info = &tpd_ring->buffer_info[first_mapped];
  2311			dma_unmap_page(&adapter->pdev->dev,
  2312				       buffer_info->dma,
  2313				       buffer_info->length,
  2314				       DMA_TO_DEVICE);
> 2315			buffer_info->dma = NULL;
  2316	
> 2317			if (++first_mapped == tdp_ring->count)
  2318				first_mapped = 0;
  2319		}
  2320		return -ENOMEM;
  2321	}
  2322	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2] (drivers/ethernet/atheros/atl1) test DMA mapping for error code

[Thomas Fourier]

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.

In `atl1_alloc_rx_buffers()`, the buffer is deallocated ans marked as such.

In `atl1_tx_map()`, the arleady 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

Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
---
 drivers/net/ethernet/atheros/atlx/atl1.c | 39 ++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/atheros/atlx/atl1.c b/drivers/net/ethernet/atheros/atlx/atl1.c
index cfdb546a09e7..d3cd51ccf621 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;
+			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,7 +2191,7 @@ 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,
+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;
@@ -2195,12 +2203,14 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
 	unsigned int f;
 	int retval;
 	u16 next_to_use;
+	u16 first_mapped;
 	u16 data_len;
 	u8 hdr_len;
 
 	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,8 @@ 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 +2268,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 +2293,8 @@ 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 +2303,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 +2453,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;
 	atl1_tx_queue(adapter, count, ptpd);
 	atl1_update_mailbox(adapter);
 	return NETDEV_TX_OK;
-- 
2.43.0

Re: [PATCH v2] (drivers/ethernet/atheros/atl1) test DMA mapping for error code

[Jakub Kicinski]

On Fri, 13 Jun 2025 11:54:08 +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.

Please read:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

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

[Thomas Fourier]

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.

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>
---
 drivers/net/ethernet/atheros/atlx/atl1.c | 43 ++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 3 deletions(-)

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;
+			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;
 	atl1_tx_queue(adapter, count, ptpd);
 	atl1_update_mailbox(adapter);
 	return NETDEV_TX_OK;
-- 
2.43.0

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

[Jakub Kicinski]

On Mon, 16 Jun 2025 15:59:55 +0200 Thomas Fourier wrote:
> According to Shuah Khan[1]

Sorry for a non-technical question -- are you part of some outreach /
training program? The presentation you linked is from 2013, I wonder
what made you pick up this particular task.

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

[Thomas Fourier]

On 16/06/2025 19:46, Jakub Kicinski wrote:
> On Mon, 16 Jun 2025 15:59:55 +0200 Thomas Fourier wrote:
>> According to Shuah Khan[1]
> Sorry for a non-technical question -- are you part of some outreach /
> training program? The presentation you linked is from 2013, I wonder
> what made you pick up this particular task.

I am doing a master thesis on static analysis and I am writing a checker
with

Smatch to test if error codes are well-checked.  My supervisor suggested
that

I look at DMA mapping errors checks to see if people were interested in such

patches and because they are quite easy to statically assert.

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

[Jakub Kicinski]

On Tue, 17 Jun 2025 17:47:50 +0200 Thomas Fourier wrote:
> On 16/06/2025 19:46, Jakub Kicinski wrote:
> > On Mon, 16 Jun 2025 15:59:55 +0200 Thomas Fourier wrote:  
> >> According to Shuah Khan[1]  
> > Sorry for a non-technical question -- are you part of some outreach /
> > training program? The presentation you linked is from 2013, I wonder
> > what made you pick up this particular task.  
> 
> I am doing a master thesis on static analysis and I am writing a checker
> with

I see.

> Smatch to test if error codes are well-checked.  My supervisor suggested
> that
> 
> I look at DMA mapping errors checks to see if people were interested in such
> 
> patches and because they are quite easy to statically assert.

Given the community's mixed experience with researches, would you
be willing to share the name of the institution ?

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

[Jakub Kicinski]

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;