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

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

[Geoff Levand]

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 | 102 +++++++++++--------
 1 file changed, 58 insertions(+), 44 deletions(-)

-- 
2.34.1

[PATCH net v4 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 | 56 ++++++++++++--------
 1 file changed, 34 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..7a8b5e1e77a6 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -365,51 +365,63 @@ 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 {
+		unsigned int total_bytes;
+		unsigned int offset;
+	} aligned_buf;
+	dma_addr_t cpu_addr;
 
 	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);
+	aligned_buf.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
+		GELIC_NET_MAX_MTU + (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 = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
 	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);
+
+	cpu_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
+		DMA_FROM_DEVICE);
+
+	descr->buf_addr = cpu_to_be32(cpu_addr);
+
 	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 v4 1/2] net/ps3_gelic_net: Fix RX sk_buff length

[Alexander H Duyck]

On Sun, 2023-02-05 at 22:10 +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)
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 56 ++++++++++++--------
>  1 file changed, 34 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..7a8b5e1e77a6 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -365,51 +365,63 @@ 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 {
> +		unsigned int total_bytes;
> +		unsigned int offset;
> +	} aligned_buf;
> +	dma_addr_t cpu_addr;
>  
>  	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);
> +	aligned_buf.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
> +		GELIC_NET_MAX_MTU + (GELIC_NET_RXBUF_ALIGN - 1);
> +

This value isn't aligned to anything as there have been no steps taken
to align it. In fact it is guaranteed to be off by 2. Did you maybe
mean to use an "&" somewhere?

> +	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;

Why remove this comment?

>  	}
> -	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 = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);

Originally this was being written using cpu_to_be32. WIth this you are
writing it raw w/ the cpu endianness. Is there a byte ordering issue
here?

>  	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);

Rather than messing with all this it might be easier to just drop
offset in favor of NET_SKB_PAD since that should be offset in all cases
where dev_alloc_skb is being used. With that the reserve could just be
a constant.

> -	/* 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);
> +
> +	cpu_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
> +		DMA_FROM_DEVICE);
> +
> +	descr->buf_addr = cpu_to_be32(cpu_addr);
> +
>  	if (!descr->buf_addr) {

This check should be for dma_mapping_error based on "cpu_addr". There
are some configs that don't return NULL to indicate a mapping error.

>  		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;
>  }
>  
>  /**

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

[Geoff Levand]

Hi Alexander,

Thanks for the review.

On 2/6/23 08:25, Alexander H Duyck wrote:
> On Sun, 2023-02-05 at 22:10 +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.
>>

>>  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 {
>> +		unsigned int total_bytes;
>> +		unsigned int offset;
>> +	} aligned_buf;
>> +	dma_addr_t cpu_addr;
>>  
>>  	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);
>> +	aligned_buf.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
>> +		GELIC_NET_MAX_MTU + (GELIC_NET_RXBUF_ALIGN - 1);
>> +
> 
> This value isn't aligned to anything as there have been no steps taken
> to align it. In fact it is guaranteed to be off by 2. Did you maybe
> mean to use an "&" somewhere?

total_bytes here means the total number of bytes to allocate that will
allow for the desired alignment.  This value a bit too much though since
we really just need it to end on a GELIC_NET_RXBUF_ALIGN boundary, so 
adding ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN) should be enough.
I'll fix that in the next patch version.

>> +	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;
> 
> Why remove this comment?

If we return -ENOMEM this descriptor shouldn't be used.

>>  	}
>> -	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 = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
> 
> Originally this was being written using cpu_to_be32. WIth this you are
> writing it raw w/ the cpu endianness. Is there a byte ordering issue
> here?

No. The PS3 has a big endian CPU, so we really don't need any
of the endian conversions.

> 
>>  	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);
> 
> Rather than messing with all this it might be easier to just drop
> offset in favor of NET_SKB_PAD since that should be offset in all cases
> where dev_alloc_skb is being used. With that the reserve could just be
> a constant.

GELIC_NET_RXBUF_ALIGN is a property of the gelic hardware device.  I
would think if NET_SKB_PAD would work it would just be by coincidence.

>> -	/* 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);
>> +
>> +	cpu_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
>> +		DMA_FROM_DEVICE);
>> +
>> +	descr->buf_addr = cpu_to_be32(cpu_addr);
>> +
>>  	if (!descr->buf_addr) {
> 
> This check should be for dma_mapping_error based on "cpu_addr". There
> are some configs that don't return NULL to indicate a mapping error.

As was requested, I have put those corrections into the second patch
of this series.

-Geoff

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

[Alexander Duyck]

On Sun, Feb 12, 2023 at 9:06 AM Geoff Levand <geoff@infradead.org> wrote:
>
> Hi Alexander,
>
> Thanks for the review.
>
> On 2/6/23 08:25, Alexander H Duyck wrote:
> > On Sun, 2023-02-05 at 22:10 +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.
> >>
>
> >>  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 {
> >> +            unsigned int total_bytes;
> >> +            unsigned int offset;
> >> +    } aligned_buf;
> >> +    dma_addr_t cpu_addr;
> >>
> >>      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);
> >> +    aligned_buf.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
> >> +            GELIC_NET_MAX_MTU + (GELIC_NET_RXBUF_ALIGN - 1);
> >> +
> >
> > This value isn't aligned to anything as there have been no steps taken
> > to align it. In fact it is guaranteed to be off by 2. Did you maybe
> > mean to use an "&" somewhere?
>
> total_bytes here means the total number of bytes to allocate that will
> allow for the desired alignment.  This value a bit too much though since
> we really just need it to end on a GELIC_NET_RXBUF_ALIGN boundary, so
> adding ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN) should be enough.
> I'll fix that in the next patch version.
>
> >> +    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;
> >
> > Why remove this comment?
>
> If we return -ENOMEM this descriptor shouldn't be used.

Right. But the comment is essentially saying that. The question is why
remove the documentation?

> >>      }
> >> -    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 = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
> >
> > Originally this was being written using cpu_to_be32. WIth this you are
> > writing it raw w/ the cpu endianness. Is there a byte ordering issue
> > here?
>
> No. The PS3 has a big endian CPU, so we really don't need any
> of the endian conversions.

This doesn't introduce an ordering error, but it introduces a type
error. The bufsize variable was defined as a CPU ordered variable
whereas buf_size is a __be32. If you enable sparse checking it should
return an error.

I would recommend keeping the cpu_to_be32. If your architecture is big
endian it will do nothing and add no overhead. If at some point in the
future someone were to plug this IP into another architecture it would
take care of sorting out the byte ordering for you.

> >
> >>      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);
> >
> > Rather than messing with all this it might be easier to just drop
> > offset in favor of NET_SKB_PAD since that should be offset in all cases
> > where dev_alloc_skb is being used. With that the reserve could just be
> > a constant.
>
> GELIC_NET_RXBUF_ALIGN is a property of the gelic hardware device.  I
> would think if NET_SKB_PAD would work it would just be by coincidence.

NET_SKB_PAD is the alignment generated when you use dev_alloc_skb.
What I was getting at is that "offset" could probably be removed in
favor of just using NET_SKB_PAD.

[PATCH net v4 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 | 52 ++++++++++----------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index 7a8b5e1e77a6..5622b512e2e4 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -309,22 +309,34 @@ 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--) {
+				if (descr->bus_addr) {
+					dma_unmap_single(ctodev(card),
+						descr->bus_addr,
+						GELIC_DESCR_SIZE,
+						DMA_BIDIRECTIONAL);
+				}
+			}
+			return -ENOMEM;
+		}
 
 		descr->next = descr + 1;
 		descr->prev = descr - 1;
@@ -334,8 +346,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
 	start_descr->prev = (descr - 1);
 
 	/* chain bus addr of hw descriptor */
-	descr = start_descr;
-	for (i = 0; i < no; i++, descr++) {
+	for (i = 0, descr = start_descr; i < no; i++, descr++) {
 		descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
 	}
 
@@ -346,14 +357,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;
 }
 
 /**
@@ -407,19 +410,18 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
 	cpu_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
 		DMA_FROM_DEVICE);
 
-	descr->buf_addr = cpu_to_be32(cpu_addr);
-
-	if (!descr->buf_addr) {
+	if (unlikely(dma_mapping_error(dev, cpu_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;
 	}
 
+	descr->buf_addr = cpu_to_be32(cpu_addr);
+
 	gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
 	return 0;
 }
@@ -775,6 +777,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) {
@@ -789,11 +792,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

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

[Alexander H Duyck]

On Sun, 2023-02-05 at 22:10 +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)
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 52 ++++++++++----------
>  1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index 7a8b5e1e77a6..5622b512e2e4 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -309,22 +309,34 @@ 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);

Are bus_addr and the CPU the same byte ordering? Just wondering since
this is being passed raw. I would have expected it to go through a
cpu_to_be32.

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

The expectation for dma_mapping_error is that the address is in cpu
order. So in this case it is partially correct since bus_addr wasn't
byte swapped, but generally the dma address used should be a CPU byte
ordered variable.

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

So I am pretty sure this is broken. Usually for something like this I
will resort to just doing a while (i--) as "i == 0" should be a valid
buffer to have to unmap.

Maybe something like:
			while (i--) {
				descr--;

Also I think you can get rid of the if since descr->bus_addr should be
valid for all values since you populated it just a few lines above for
each value of i.

> +					dma_unmap_single(ctodev(card),
> +						descr->bus_addr,
> +						GELIC_DESCR_SIZE,
> +						DMA_BIDIRECTIONAL);
> +				}
> +			}
> +			return -ENOMEM;
> +		}
>  
>  		descr->next = descr + 1;
>  		descr->prev = descr - 1;
> @@ -334,8 +346,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
>  	start_descr->prev = (descr - 1);
>  
>  	/* chain bus addr of hw descriptor */
> -	descr = start_descr;
> -	for (i = 0; i < no; i++, descr++) {
> +	for (i = 0, descr = start_descr; i < no; i++, descr++) {
>  		descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
>  	}
>  

This seems like an unrelated change that was snuck in.

> @@ -346,14 +357,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;
>  }
>  
>  /**
> @@ -407,19 +410,18 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>  	cpu_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
>  		DMA_FROM_DEVICE);
>  
> -	descr->buf_addr = cpu_to_be32(cpu_addr);
> -
> -	if (!descr->buf_addr) {
> +	if (unlikely(dma_mapping_error(dev, cpu_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;
>  	}
>  
> +	descr->buf_addr = cpu_to_be32(cpu_addr);
> +

Okay, so this addresses the comment I had in the earlier patch.

>  	gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
>  	return 0;
>  }
> @@ -775,6 +777,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) {
> @@ -789,11 +792,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;
>  	}

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

[Geoff Levand]

Hi Alexander,

On 2/6/23 08:37, Alexander H Duyck wrote:
> On Sun, 2023-02-05 at 22:10 +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)
>> Signed-off-by: Geoff Levand <geoff@infradead.org>
>> ---
>>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 52 ++++++++++----------
>>  1 file changed, 27 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index 7a8b5e1e77a6..5622b512e2e4 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -309,22 +309,34 @@ 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);
> 
> Are bus_addr and the CPU the same byte ordering? Just wondering since
> this is being passed raw. I would have expected it to go through a
> cpu_to_be32.

As I mentioned in my reply to the first patch, the PS3's CPU is
big endian, so we really don't need any of the endian conversions.

>> -		if (!descr->bus_addr)
>> -			goto iommu_error;
>> +		if (unlikely(dma_mapping_error(dev, descr->bus_addr))) {
> 
> The expectation for dma_mapping_error is that the address is in cpu
> order. So in this case it is partially correct since bus_addr wasn't
> byte swapped, but generally the dma address used should be a CPU byte
> ordered variable.
> 
>> +			dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
>> +				__LINE__);
>> +
>> +			for (i--, descr--; i > 0; i--, descr--) {
>> +				if (descr->bus_addr) {
> 
> So I am pretty sure this is broken. Usually for something like this I
> will resort to just doing a while (i--) as "i == 0" should be a valid
> buffer to have to unmap.
> 
> Maybe something like:
> 			while (i--) {
> 				descr--;
> 
> Also I think you can get rid of the if since descr->bus_addr should be
> valid for all values since you populated it just a few lines above for
> each value of i.

OK, I'll change that.
>> +					dma_unmap_single(ctodev(card),
>> +						descr->bus_addr,
>> +						GELIC_DESCR_SIZE,
>> +						DMA_BIDIRECTIONAL);
>> +				}
>> +			}
>> +			return -ENOMEM;
>> +		}
>>  
>>  		descr->next = descr + 1;
>>  		descr->prev = descr - 1;
>> @@ -334,8 +346,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>  	start_descr->prev = (descr - 1);
>>  
>>  	/* chain bus addr of hw descriptor */
>> -	descr = start_descr;
>> -	for (i = 0; i < no; i++, descr++) {
>> +	for (i = 0, descr = start_descr; i < no; i++, descr++) {
>>  		descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
>>  	}
>>  
> 
> This seems like an unrelated change that was snuck in.

I'll remove that.

Once again, thanks for the review.

-Geoff

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

[Alexander Duyck]

On Sun, Feb 12, 2023 at 9:06 AM Geoff Levand <geoff@infradead.org> wrote:
>
> Hi Alexander,
>
> On 2/6/23 08:37, Alexander H Duyck wrote:
> > On Sun, 2023-02-05 at 22:10 +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)
> >> Signed-off-by: Geoff Levand <geoff@infradead.org>
> >> ---
> >>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 52 ++++++++++----------
> >>  1 file changed, 27 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> >> index 7a8b5e1e77a6..5622b512e2e4 100644
> >> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> >> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> >> @@ -309,22 +309,34 @@ 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);
> >
> > Are bus_addr and the CPU the same byte ordering? Just wondering since
> > this is being passed raw. I would have expected it to go through a
> > cpu_to_be32.
>
> As I mentioned in my reply to the first patch, the PS3's CPU is
> big endian, so we really don't need any of the endian conversions.

My advice would be to still make use of the cpu_to_be32 macros. It
will take care of any possible byte ordering issues should you work
with a different CPU architecture in the future. Otherwise if you are
certain that the values will always be CPU ordered you might try
changing the type rather than using __be32 for your descriptor
variable types.

For example most PCIe hardware is using a little endian architecture
and on x86 we don't need to do the byte swapping since the cpu is
little endian. However we still use cpu_to_le32 throughout most
drivers.

You might read through the documentation for sparse at:
https://www.kernel.org/doc/html/latest/dev-tools/sparse.html