[PATCH net v5 0/2] net/ps3_gelic_net: DMA related fixes

[PATCH net v5 0/2] net/ps3_gelic_net: DMA related fixes

[Geoff Levand]

v5: Some additional patch cleanups.
v4: More patch cleanups.
v3: Cleaned up patches as requested.

Geoff Levand (2):
  net/ps3_gelic_net: Fix RX sk_buff length
  net/ps3_gelic_net: Use dma_mapping_error

 drivers/net/ethernet/toshiba/ps3_gelic_net.c | 94 +++++++++++---------
 1 file changed, 52 insertions(+), 42 deletions(-)

-- 
2.34.1

[PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error

[Geoff Levand]

The current Gelic Etherenet driver was checking the return value of its
dma_map_single call, and not using the dma_mapping_error() routine.

Fixes runtime problems like these:

  DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
  WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc

Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
Signed-off-by: Geoff Levand <geoff@infradead.org>
---
 drivers/net/ethernet/toshiba/ps3_gelic_net.c | 41 ++++++++++----------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index 2bb68e60d0d5..0e52bb99e344 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -309,22 +309,30 @@ static int gelic_card_init_chain(struct gelic_card *card,
 				 struct gelic_descr_chain *chain,
 				 struct gelic_descr *start_descr, int no)
 {
-	int i;
+	struct device *dev = ctodev(card);
 	struct gelic_descr *descr;
+	int i;
 
-	descr = start_descr;
-	memset(descr, 0, sizeof(*descr) * no);
+	memset(start_descr, 0, no * sizeof(*start_descr));
 
 	/* set up the hardware pointers in each descriptor */
-	for (i = 0; i < no; i++, descr++) {
+	for (i = 0, descr = start_descr; i < no; i++, descr++) {
 		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
 		descr->bus_addr =
 			dma_map_single(ctodev(card), descr,
 				       GELIC_DESCR_SIZE,
 				       DMA_BIDIRECTIONAL);
 
-		if (!descr->bus_addr)
-			goto iommu_error;
+		if (unlikely(dma_mapping_error(dev, descr->bus_addr))) {
+			dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
+				__LINE__);
+
+			for (i--, descr--; i >= 0; i--, descr--) {
+				dma_unmap_single(ctodev(card), descr->bus_addr,
+					GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
+			}
+			return -ENOMEM;
+		}
 
 		descr->next = descr + 1;
 		descr->prev = descr - 1;
@@ -346,14 +354,6 @@ static int gelic_card_init_chain(struct gelic_card *card,
 	(descr - 1)->next_descr_addr = 0;
 
 	return 0;
-
-iommu_error:
-	for (i--, descr--; 0 <= i; i--, descr--)
-		if (descr->bus_addr)
-			dma_unmap_single(ctodev(card), descr->bus_addr,
-					 GELIC_DESCR_SIZE,
-					 DMA_BIDIRECTIONAL);
-	return -ENOMEM;
 }
 
 /**
@@ -408,13 +408,12 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
 	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
 		DMA_FROM_DEVICE);
 
-	if (!descr->buf_addr) {
+	if (unlikely(dma_mapping_error(dev, descr->buf_addr))) {
+		dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
 		dev_kfree_skb_any(descr->skb);
 		descr->buf_addr = 0;
 		descr->buf_size = 0;
 		descr->skb = NULL;
-		dev_info(dev,
-			 "%s:Could not iommu-map rx buffer\n", __func__);
 		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
 		return -ENOMEM;
 	}
@@ -774,6 +773,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
 				  struct gelic_descr *descr,
 				  struct sk_buff *skb)
 {
+	struct device *dev = ctodev(card);
 	dma_addr_t buf;
 
 	if (card->vlan_required) {
@@ -788,11 +788,10 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
 		skb = skb_tmp;
 	}
 
-	buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
+	buf = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE);
 
-	if (!buf) {
-		dev_err(ctodev(card),
-			"dma map 2 failed (%p, %i). Dropping packet\n",
+	if (unlikely(dma_mapping_error(dev, buf))) {
+		dev_err(dev, "dma map 2 failed (%p, %i). Dropping packet\n",
 			skb->data, skb->len);
 		return -ENOMEM;
 	}
-- 
2.34.1

[PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length

[Geoff Levand]

The Gelic Ethernet device needs to have the RX sk_buffs aligned to
GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a
multiple of GELIC_NET_RXBUF_ALIGN.

The current Gelic Ethernet driver was not allocating sk_buffs large
enough to allow for this alignment.

Fixes various randomly occurring runtime network errors.

Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
Signed-off-by: Geoff Levand <geoff@infradead.org>
---
 drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++--------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index cf8de8a7a8a1..2bb68e60d0d5 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -365,51 +365,62 @@ static int gelic_card_init_chain(struct gelic_card *card,
  *
  * allocates a new rx skb, iommu-maps it and attaches it to the descriptor.
  * Activate the descriptor state-wise
+ *
+ * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length
+ * must be a multiple of GELIC_NET_RXBUF_ALIGN.
  */
 static int gelic_descr_prepare_rx(struct gelic_card *card,
 				  struct gelic_descr *descr)
 {
-	int offset;
-	unsigned int bufsize;
+	struct device *dev = ctodev(card);
+	struct {
+		const unsigned int buffer_bytes;
+		const unsigned int total_bytes;
+		unsigned int offset;
+	} aligned_buf = {
+		.buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
+		.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
+			ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
+	};
 
 	if (gelic_descr_get_status(descr) !=  GELIC_DESCR_DMA_NOT_IN_USE)
 		dev_info(ctodev(card), "%s: ERROR status\n", __func__);
-	/* we need to round up the buffer size to a multiple of 128 */
-	bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
 
-	/* and we need to have it 128 byte aligned, therefore we allocate a
-	 * bit more */
-	descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
+	descr->skb = dev_alloc_skb(aligned_buf.total_bytes);
+
 	if (!descr->skb) {
-		descr->buf_addr = 0; /* tell DMAC don't touch memory */
+		descr->buf_addr = 0;
 		return -ENOMEM;
 	}
-	descr->buf_size = cpu_to_be32(bufsize);
+
+	aligned_buf.offset =
+		PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN) -
+			descr->skb->data;
+
+	descr->buf_size = aligned_buf.buffer_bytes;
 	descr->dmac_cmd_status = 0;
 	descr->result_size = 0;
 	descr->valid_size = 0;
 	descr->data_error = 0;
 
-	offset = ((unsigned long)descr->skb->data) &
-		(GELIC_NET_RXBUF_ALIGN - 1);
-	if (offset)
-		skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
-	/* io-mmu-map the skb */
-	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
-						     descr->skb->data,
-						     GELIC_NET_MAX_MTU,
-						     DMA_FROM_DEVICE));
+	skb_reserve(descr->skb, aligned_buf.offset);
+
+	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
+		DMA_FROM_DEVICE);
+
 	if (!descr->buf_addr) {
 		dev_kfree_skb_any(descr->skb);
+		descr->buf_addr = 0;
+		descr->buf_size = 0;
 		descr->skb = NULL;
-		dev_info(ctodev(card),
+		dev_info(dev,
 			 "%s:Could not iommu-map rx buffer\n", __func__);
 		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
 		return -ENOMEM;
-	} else {
-		gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
-		return 0;
 	}
+
+	gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
+	return 0;
 }
 
 /**
-- 
2.34.1

Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length

[Paolo Abeni]

+Alex
On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote:
> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
> GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a
> multiple of GELIC_NET_RXBUF_ALIGN.
> 
> The current Gelic Ethernet driver was not allocating sk_buffs large
> enough to allow for this alignment.
> 
> Fixes various randomly occurring runtime network errors.
> 
> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)

Please use the correct format for the Fixes tag: <hash> ("<msg>"). Note
the missing quotes.

> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++--------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index cf8de8a7a8a1..2bb68e60d0d5 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -365,51 +365,62 @@ static int gelic_card_init_chain(struct gelic_card *card,
>   *
>   * allocates a new rx skb, iommu-maps it and attaches it to the descriptor.
>   * Activate the descriptor state-wise
> + *
> + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length
> + * must be a multiple of GELIC_NET_RXBUF_ALIGN.
>   */
>  static int gelic_descr_prepare_rx(struct gelic_card *card,
>  				  struct gelic_descr *descr)
>  {
> -	int offset;
> -	unsigned int bufsize;
> +	struct device *dev = ctodev(card);
> +	struct {
> +		const unsigned int buffer_bytes;
> +		const unsigned int total_bytes;
> +		unsigned int offset;
> +	} aligned_buf = {
> +		.buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
> +		.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
> +			ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
> +	};
>  
>  	if (gelic_descr_get_status(descr) !=  GELIC_DESCR_DMA_NOT_IN_USE)
>  		dev_info(ctodev(card), "%s: ERROR status\n", __func__);
> -	/* we need to round up the buffer size to a multiple of 128 */
> -	bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
>  
> -	/* and we need to have it 128 byte aligned, therefore we allocate a
> -	 * bit more */
> -	descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
> +	descr->skb = dev_alloc_skb(aligned_buf.total_bytes);
> +
>  	if (!descr->skb) {
> -		descr->buf_addr = 0; /* tell DMAC don't touch memory */
> +		descr->buf_addr = 0;
>  		return -ENOMEM;
>  	}
> -	descr->buf_size = cpu_to_be32(bufsize);
> +
> +	aligned_buf.offset =
> +		PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN) -
> +			descr->skb->data;
> +
> +	descr->buf_size = aligned_buf.buffer_bytes;
>  	descr->dmac_cmd_status = 0;
>  	descr->result_size = 0;
>  	descr->valid_size = 0;
>  	descr->data_error = 0;
>  
> -	offset = ((unsigned long)descr->skb->data) &
> -		(GELIC_NET_RXBUF_ALIGN - 1);
> -	if (offset)
> -		skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> -	/* io-mmu-map the skb */
> -	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
> -						     descr->skb->data,
> -						     GELIC_NET_MAX_MTU,
> -						     DMA_FROM_DEVICE));
> +	skb_reserve(descr->skb, aligned_buf.offset);
> +
> +	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
> +		DMA_FROM_DEVICE);

As already noted by Alex, you should preserve the cpu_to_be32(). If the
running arch is be32, it has 0 performance and/or code size overhead,
and it helps readability and maintainability.

Please be sure to check the indentation of new code with checkpatch.

When reposting, please be sure to CC people that gave feedback on
previous iterations.

Cheers,

Paolo

Re: [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error

[Paolo Abeni]

+Alex
On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote:
> The current Gelic Etherenet driver was checking the return value of its
> dma_map_single call, and not using the dma_mapping_error() routine.
> 
> Fixes runtime problems like these:
> 
>   DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
>   WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
> 
> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)

Please use the correct format for the above tag.

> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 41 ++++++++++----------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index 2bb68e60d0d5..0e52bb99e344 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -309,22 +309,30 @@ static int gelic_card_init_chain(struct gelic_card *card,
>  				 struct gelic_descr_chain *chain,
>  				 struct gelic_descr *start_descr, int no)
>  {
> -	int i;
> +	struct device *dev = ctodev(card);
>  	struct gelic_descr *descr;
> +	int i;
>  
> -	descr = start_descr;
> -	memset(descr, 0, sizeof(*descr) * no);
> +	memset(start_descr, 0, no * sizeof(*start_descr));
>  
>  	/* set up the hardware pointers in each descriptor */
> -	for (i = 0; i < no; i++, descr++) {
> +	for (i = 0, descr = start_descr; i < no; i++, descr++) {
>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>  		descr->bus_addr =
>  			dma_map_single(ctodev(card), descr,
>  				       GELIC_DESCR_SIZE,
>  				       DMA_BIDIRECTIONAL);
>  
> -		if (!descr->bus_addr)
> -			goto iommu_error;
> +		if (unlikely(dma_mapping_error(dev, descr->bus_addr))) {

Not a big issue, but I think the existing goto is preferable to the
following indentation

> +			dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
> +				__LINE__);
> +
> +			for (i--, descr--; i >= 0; i--, descr--) {

Again not a big deal, but I think the construct suggested by Alex in
the previous patch is more clear.


> +				dma_unmap_single(ctodev(card), descr->bus_addr,
> +					GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
> +			}
> +			return -ENOMEM;
> +		}
>  
>  		descr->next = descr + 1;
>  		descr->prev = descr - 1;
> @@ -346,14 +354,6 @@ static int gelic_card_init_chain(struct gelic_card *card,
>  	(descr - 1)->next_descr_addr = 0;
>  
>  	return 0;
> -
> -iommu_error:
> -	for (i--, descr--; 0 <= i; i--, descr--)
> -		if (descr->bus_addr)
> -			dma_unmap_single(ctodev(card), descr->bus_addr,
> -					 GELIC_DESCR_SIZE,
> -					 DMA_BIDIRECTIONAL);
> -	return -ENOMEM;
>  }
>  
>  /**
> @@ -408,13 +408,12 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>  	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
>  		DMA_FROM_DEVICE);
>  
> -	if (!descr->buf_addr) {
> +	if (unlikely(dma_mapping_error(dev, descr->buf_addr))) {
> +		dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
>  		dev_kfree_skb_any(descr->skb);
>  		descr->buf_addr = 0;
>  		descr->buf_size = 0;
>  		descr->skb = NULL;
> -		dev_info(dev,
> -			 "%s:Could not iommu-map rx buffer\n", __func__);

You touched the above line in the previous patch. Since it does lot
look functional-related to the fix here you can as well drop the
message in the previous patch.


Cheers,

Paolo

Re: [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error

[Alexander Lobakin]

From: Geoff Levand <geoff@infradead.org>
Date: Sun, 12 Feb 2023 18:00:58 +0000

> The current Gelic Etherenet driver was checking the return value of its
> dma_map_single call, and not using the dma_mapping_error() routine.
> 
> Fixes runtime problems like these:
> 
>   DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
>   WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
> 
> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 41 ++++++++++----------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index 2bb68e60d0d5..0e52bb99e344 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -309,22 +309,30 @@ static int gelic_card_init_chain(struct gelic_card *card,
>  				 struct gelic_descr_chain *chain,
>  				 struct gelic_descr *start_descr, int no)
>  {
> -	int i;
> +	struct device *dev = ctodev(card);
>  	struct gelic_descr *descr;
> +	int i;
>  
> -	descr = start_descr;
> -	memset(descr, 0, sizeof(*descr) * no);
> +	memset(start_descr, 0, no * sizeof(*start_descr));
>  
>  	/* set up the hardware pointers in each descriptor */
> -	for (i = 0; i < no; i++, descr++) {
> +	for (i = 0, descr = start_descr; i < no; i++, descr++) {
>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>  		descr->bus_addr =
>  			dma_map_single(ctodev(card), descr,
>  				       GELIC_DESCR_SIZE,
>  				       DMA_BIDIRECTIONAL);
>  
> -		if (!descr->bus_addr)
> -			goto iommu_error;
> +		if (unlikely(dma_mapping_error(dev, descr->bus_addr))) {

dma_mapping_error() already has unlikely() inside.

> +			dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
> +				__LINE__);
> +
> +			for (i--, descr--; i >= 0; i--, descr--) {
> +				dma_unmap_single(ctodev(card), descr->bus_addr,
> +					GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
> +			}
> +			return -ENOMEM;
> +		}
>  
>  		descr->next = descr + 1;
>  		descr->prev = descr - 1;
> @@ -346,14 +354,6 @@ static int gelic_card_init_chain(struct gelic_card *card,
>  	(descr - 1)->next_descr_addr = 0;
>  
>  	return 0;
> -
> -iommu_error:
> -	for (i--, descr--; 0 <= i; i--, descr--)
> -		if (descr->bus_addr)
> -			dma_unmap_single(ctodev(card), descr->bus_addr,
> -					 GELIC_DESCR_SIZE,
> -					 DMA_BIDIRECTIONAL);
> -	return -ENOMEM;
>  }
>  
>  /**
> @@ -408,13 +408,12 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>  	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
>  		DMA_FROM_DEVICE);
>  
> -	if (!descr->buf_addr) {
> +	if (unlikely(dma_mapping_error(dev, descr->buf_addr))) {

Same.

> +		dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);

It is fast path. You're not allowed to use plain printk on fast path,
since you may generate then thousands of messages per second.
Consider looking at _ratelimit family of functions.

>  		dev_kfree_skb_any(descr->skb);
>  		descr->buf_addr = 0;
>  		descr->buf_size = 0;
>  		descr->skb = NULL;
> -		dev_info(dev,
> -			 "%s:Could not iommu-map rx buffer\n", __func__);
>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>  		return -ENOMEM;
>  	}
> @@ -774,6 +773,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
>  				  struct gelic_descr *descr,
>  				  struct sk_buff *skb)
>  {
> +	struct device *dev = ctodev(card);
>  	dma_addr_t buf;
>  
>  	if (card->vlan_required) {
> @@ -788,11 +788,10 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
>  		skb = skb_tmp;
>  	}
>  
> -	buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
> +	buf = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE);
>  
> -	if (!buf) {
> -		dev_err(ctodev(card),
> -			"dma map 2 failed (%p, %i). Dropping packet\n",
> +	if (unlikely(dma_mapping_error(dev, buf))) {
> +		dev_err(dev, "dma map 2 failed (%p, %i). Dropping packet\n",

Same, same (for both lines).

>  			skb->data, skb->len);
>  		return -ENOMEM;
>  	}
Thanks,
Olek

Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length

[Alexander Lobakin]

From: Geoff Levand <geoff@infradead.org>
Date: Sun, 12 Feb 2023 18:00:58 +0000

> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
> GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a
> multiple of GELIC_NET_RXBUF_ALIGN.
> 
> The current Gelic Ethernet driver was not allocating sk_buffs large
> enough to allow for this alignment.
> 
> Fixes various randomly occurring runtime network errors.
> 
> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++--------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index cf8de8a7a8a1..2bb68e60d0d5 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -365,51 +365,62 @@ static int gelic_card_init_chain(struct gelic_card *card,
>   *
>   * allocates a new rx skb, iommu-maps it and attaches it to the descriptor.
>   * Activate the descriptor state-wise
> + *
> + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length
> + * must be a multiple of GELIC_NET_RXBUF_ALIGN.
>   */
>  static int gelic_descr_prepare_rx(struct gelic_card *card,
>  				  struct gelic_descr *descr)
>  {
> -	int offset;
> -	unsigned int bufsize;
> +	struct device *dev = ctodev(card);
> +	struct {
> +		const unsigned int buffer_bytes;
> +		const unsigned int total_bytes;
> +		unsigned int offset;
> +	} aligned_buf = {
> +		.buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
> +		.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
> +			ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
> +	};
>  
>  	if (gelic_descr_get_status(descr) !=  GELIC_DESCR_DMA_NOT_IN_USE)
>  		dev_info(ctodev(card), "%s: ERROR status\n", __func__);
> -	/* we need to round up the buffer size to a multiple of 128 */
> -	bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
>  
> -	/* and we need to have it 128 byte aligned, therefore we allocate a
> -	 * bit more */
> -	descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
> +	descr->skb = dev_alloc_skb(aligned_buf.total_bytes);

I highly recommend using {napi,netdev}_alloc_frag_align() +
{napi_,}build_skb() to not waste memory. It will align everything for
ye, so you won't need all this.

Also, dunno why create an onstack struct for 3 integers :D

> +
>  	if (!descr->skb) {
> -		descr->buf_addr = 0; /* tell DMAC don't touch memory */
> +		descr->buf_addr = 0;
>  		return -ENOMEM;
>  	}
> -	descr->buf_size = cpu_to_be32(bufsize);
> +
> +	aligned_buf.offset =
> +		PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN) -
> +			descr->skb->data;
> +
> +	descr->buf_size = aligned_buf.buffer_bytes;
>  	descr->dmac_cmd_status = 0;
>  	descr->result_size = 0;
>  	descr->valid_size = 0;
>  	descr->data_error = 0;
>  
> -	offset = ((unsigned long)descr->skb->data) &
> -		(GELIC_NET_RXBUF_ALIGN - 1);
> -	if (offset)
> -		skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> -	/* io-mmu-map the skb */
> -	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
> -						     descr->skb->data,
> -						     GELIC_NET_MAX_MTU,
> -						     DMA_FROM_DEVICE));
> +	skb_reserve(descr->skb, aligned_buf.offset);
> +
> +	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
> +		DMA_FROM_DEVICE);
> +
>  	if (!descr->buf_addr) {
>  		dev_kfree_skb_any(descr->skb);
> +		descr->buf_addr = 0;
> +		descr->buf_size = 0;
>  		descr->skb = NULL;
> -		dev_info(ctodev(card),
> +		dev_info(dev,
>  			 "%s:Could not iommu-map rx buffer\n", __func__);
>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>  		return -ENOMEM;
> -	} else {
> -		gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> -		return 0;
>  	}
> +
> +	gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> +	return 0;
>  }
>  
>  /**

Thanks,
Olek

Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length

[Geoff Levand]

Hi,

On 2/14/23 09:34, Alexander Lobakin wrote:
>> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
>> GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a
>> multiple of GELIC_NET_RXBUF_ALIGN.
>>
>> The current Gelic Ethernet driver was not allocating sk_buffs large
>> enough to allow for this alignment.
>>
>> Fixes various randomly occurring runtime network errors.
>>
>> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
>> Signed-off-by: Geoff Levand <geoff@infradead.org>
>> ---
>>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++--------
>>  1 file changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index cf8de8a7a8a1..2bb68e60d0d5 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -365,51 +365,62 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>   *
>>   * allocates a new rx skb, iommu-maps it and attaches it to the descriptor.
>>   * Activate the descriptor state-wise
>> + *
>> + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length
>> + * must be a multiple of GELIC_NET_RXBUF_ALIGN.
>>   */
>>  static int gelic_descr_prepare_rx(struct gelic_card *card,
>>  				  struct gelic_descr *descr)
>>  {
>> -	int offset;
>> -	unsigned int bufsize;
>> +	struct device *dev = ctodev(card);
>> +	struct {
>> +		const unsigned int buffer_bytes;
>> +		const unsigned int total_bytes;
>> +		unsigned int offset;
>> +	} aligned_buf = {
>> +		.buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
>> +		.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
>> +			ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
>> +	};
>>  
>>  	if (gelic_descr_get_status(descr) !=  GELIC_DESCR_DMA_NOT_IN_USE)
>>  		dev_info(ctodev(card), "%s: ERROR status\n", __func__);
>> -	/* we need to round up the buffer size to a multiple of 128 */
>> -	bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
>>  
>> -	/* and we need to have it 128 byte aligned, therefore we allocate a
>> -	 * bit more */
>> -	descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
>> +	descr->skb = dev_alloc_skb(aligned_buf.total_bytes);
> 
> I highly recommend using {napi,netdev}_alloc_frag_align() +
> {napi_,}build_skb() to not waste memory. It will align everything for
> ye, so you won't need all this.

I converted this over to use napi_alloc_frag_align and napi_build_skb, and
then cleanup with skb_free_frag.  I couldn't find any good documentation for
those napi routines though.

For napi_alloc_frag_align, it seems the align parameter should be the
alignment required by the gelic hardware device, so GELIC_NET_RXBUF_ALIGN.
But for the fragsz parameter I couldn't quite figure out from the existing
users of it what exactly it should be.  

I assumed it needed to be the maximum number of bytes the hardware device can
fill, so I set it to be ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN), but
with that I got skb_over_panic errors (skb->tail > skb->end).  I added some
debugging code and found that when it hit the panic skb->end was always 384
bytes short.  I changed GELIC_NET_MAX_MTU to be 1920 which is
ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN) + 384, and it seems to be
working OK.  Does this seem OK?  Maybe the current GELIC_NET_MAX_MTU value is
incorrect.  I did not write that driver, and I don't have any good
documentation for the gelic device.

Thanks for any help you can give.

-Geoff

Re: [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error

[Geoff Levand]

On 2/14/23 02:58, Paolo Abeni wrote:
> +Alex
> On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote:
>> The current Gelic Etherenet driver was checking the return value of its
>> dma_map_single call, and not using the dma_mapping_error() routine.
>>
>> Fixes runtime problems like these:
>>
>>   DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
>>   WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
>>
>> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
> 
> Please use the correct format for the above tag.
> 
>> Signed-off-by: Geoff Levand <geoff@infradead.org>
>> ---
>>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 41 ++++++++++----------
>>  1 file changed, 20 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index 2bb68e60d0d5..0e52bb99e344 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -309,22 +309,30 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>  				 struct gelic_descr_chain *chain,
>>  				 struct gelic_descr *start_descr, int no)
>>  {
>> -	int i;
>> +	struct device *dev = ctodev(card);
>>  	struct gelic_descr *descr;
>> +	int i;
>>  
>> -	descr = start_descr;
>> -	memset(descr, 0, sizeof(*descr) * no);
>> +	memset(start_descr, 0, no * sizeof(*start_descr));
>>  
>>  	/* set up the hardware pointers in each descriptor */
>> -	for (i = 0; i < no; i++, descr++) {
>> +	for (i = 0, descr = start_descr; i < no; i++, descr++) {
>>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>>  		descr->bus_addr =
>>  			dma_map_single(ctodev(card), descr,
>>  				       GELIC_DESCR_SIZE,
>>  				       DMA_BIDIRECTIONAL);
>>  
>> -		if (!descr->bus_addr)
>> -			goto iommu_error;
>> +		if (unlikely(dma_mapping_error(dev, descr->bus_addr))) {
> 
> Not a big issue, but I think the existing goto is preferable to the
> following indentation

In a latter patch I put the common cleanup code into a separate static
routine and call that here.
 
>> +			dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
>> +				__LINE__);
>> +
>> +			for (i--, descr--; i >= 0; i--, descr--) {
> 
> Again not a big deal, but I think the construct suggested by Alex in
> the previous patch is more clear.

Well, I like this better...

>> +				dma_unmap_single(ctodev(card), descr->bus_addr,
>> +					GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
>> +			}
>> +			return -ENOMEM;
>> +		}
>>  
>>  		descr->next = descr + 1;
>>  		descr->prev = descr - 1;
>> @@ -346,14 +354,6 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>  	(descr - 1)->next_descr_addr = 0;
>>  
>>  	return 0;
>> -
>> -iommu_error:
>> -	for (i--, descr--; 0 <= i; i--, descr--)
>> -		if (descr->bus_addr)
>> -			dma_unmap_single(ctodev(card), descr->bus_addr,
>> -					 GELIC_DESCR_SIZE,
>> -					 DMA_BIDIRECTIONAL);
>> -	return -ENOMEM;
>>  }
>>  
>>  /**
>> @@ -408,13 +408,12 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>>  	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
>>  		DMA_FROM_DEVICE);
>>  
>> -	if (!descr->buf_addr) {
>> +	if (unlikely(dma_mapping_error(dev, descr->buf_addr))) {
>> +		dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
>>  		dev_kfree_skb_any(descr->skb);
>>  		descr->buf_addr = 0;
>>  		descr->buf_size = 0;
>>  		descr->skb = NULL;
>> -		dev_info(dev,
>> -			 "%s:Could not iommu-map rx buffer\n", __func__);
> 
> You touched the above line in the previous patch. Since it does lot
> look functional-related to the fix here you can as well drop the
> message in the previous patch.

Sure, I'll consider it.

Thanks for the review.

-Geoff

Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length

[Geoff Levand]

Hi,

On 2/14/23 02:46, Paolo Abeni wrote:
> +Alex
> On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote:
>> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
>> GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a
>> multiple of GELIC_NET_RXBUF_ALIGN.
>>
>> The current Gelic Ethernet driver was not allocating sk_buffs large
>> enough to allow for this alignment.
>>
>> Fixes various randomly occurring runtime network errors.
>>
>> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
> 
> Please use the correct format for the Fixes tag: <hash> ("<msg>"). Note
> the missing quotes.

I'll add those.
 
>> -	/* io-mmu-map the skb */
>> -	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
>> -						     descr->skb->data,
>> -						     GELIC_NET_MAX_MTU,
>> -						     DMA_FROM_DEVICE));
>> +	skb_reserve(descr->skb, aligned_buf.offset);
>> +
>> +	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
>> +		DMA_FROM_DEVICE);
> 
> As already noted by Alex, you should preserve the cpu_to_be32(). If the
> running arch is be32, it has 0 performance and/or code size overhead,
> and it helps readability and maintainability.

I'll add those in for the next patch set.

> Please be sure to check the indentation of new code with checkpatch.

checkpatch reports a CHECK for my indentation. As is discussed in
the kernel's coding style guide, coding style is about
maintainability, and as the maintainer of this driver I think
the indentation I have used is more readable and hence easier to
maintain.

Thanks for the review.

-Geoff

Re: [PATCH net v5 2/2] net/ps3_gelic_net: Use dma_mapping_error

[Geoff Levand]

Hi,

On 2/14/23 09:28, Alexander Lobakin wrote:
> From: Geoff Levand <geoff@infradead.org>
> Date: Sun, 12 Feb 2023 18:00:58 +0000
> 
>> The current Gelic Etherenet driver was checking the return value of its
>> dma_map_single call, and not using the dma_mapping_error() routine.
>>
>> Fixes runtime problems like these:
>>
>>   DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
>>   WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
>>
>> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
>> Signed-off-by: Geoff Levand <geoff@infradead.org>
>> ---
>>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 41 ++++++++++----------
>>  1 file changed, 20 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index 2bb68e60d0d5..0e52bb99e344 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c

>> -		if (!descr->bus_addr)
>> -			goto iommu_error;
>> +		if (unlikely(dma_mapping_error(dev, descr->bus_addr))) {
> 
> dma_mapping_error() already has unlikely() inside.

OK, I'll remove that in the next patch set.

>> +			dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
>> +				__LINE__);

> It is fast path. You're not allowed to use plain printk on fast path,
> since you may generate then thousands of messages per second.
> Consider looking at _ratelimit family of functions.

OK, I'll fix that.

Thanks for the review.

-Geoff