diff mbox series

[net,v8,1/2] net/ps3_gelic_net: Fix RX sk_buff length

Message ID 4581be2478ecc3292a3864e24fe9a42dac533b89.1678570942.git.geoff@infradead.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net/ps3_gelic_net: DMA related fixes | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers fail 2 blamed authors not CCed: jeff@garzik.org mokuno@sm.sony.co.jp; 7 maintainers not CCed: christophe.leroy@csgroup.eu jeff@garzik.org mokuno@sm.sony.co.jp edumazet@google.com npiggin@gmail.com mpe@ellerman.id.au linuxppc-dev@lists.ozlabs.org
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 64 lines checked
netdev/kdoc fail Errors and warnings before: 3 this patch: 5
netdev/source_inline success Was 0 now: 0

Commit Message

Geoff Levand March 11, 2023, 9:46 p.m. UTC 
The Gelic Ethernet device needs to have the RX sk_buffs aligned to
GELIC_NET_RXBUF_ALIGN, and also 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.

Also, correct the maximum and minimum MTU sizes, and add a new
preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME.

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 | 21 +++++++++++---------
 drivers/net/ethernet/toshiba/ps3_gelic_net.h |  5 +++--
 2 files changed, 15 insertions(+), 11 deletions(-)

Comments

Alexander Duyck March 11, 2023, 10:37 p.m. UTC | #1 
On Sat, 2023-03-11 at 21:46 +0000, Geoff Levand wrote:
> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
> GELIC_NET_RXBUF_ALIGN, and also 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.
> 
> Also, correct the maximum and minimum MTU sizes, and add a new
> preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME.
> 
> 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 | 21 +++++++++++---------
>  drivers/net/ethernet/toshiba/ps3_gelic_net.h |  5 +++--
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index cf8de8a7a8a1..56557fc8d18a 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -44,6 +44,14 @@ MODULE_AUTHOR("SCE Inc.");
>  MODULE_DESCRIPTION("Gelic Network driver");
>  MODULE_LICENSE("GPL");
>  
> +/**
> + * 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 const unsigned int gelic_rx_skb_size =
> +	ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) +
> +	GELIC_NET_RXBUF_ALIGN - 1;
> 

After a bit more digging I now understand the need for the
"GELIC_NET_RXBUF_ALIGN - 1". It shouldn't be added here. The device
will not be able to DMA into it. It is being used to align the buffer
itself to 128B. I am assuming it must be 128B aligned in BOTH size and
offset.

>  /* set irq_mask */
>  int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
> @@ -370,21 +378,16 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>  				  struct gelic_descr *descr)
>  {
>  	int offset;
> -	unsigned int bufsize;
>  
>  	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 = netdev_alloc_skb(*card->netdev, gelic_rx_skb_size);

I would leave the "+  GELIC_NET_RXBUF_ALIGN - 1" here. so it should be
	descr->skb = netdev_alloc_skb(*card->netdev, 		
				      gelic_rx_skb_size +
				      GELIC_NET_RXBUF_ALIGN - 1);

Also I would leav the comment as it makes it a bit clearer what is
going on here.

>  	if (!descr->skb) {
>  		descr->buf_addr = 0; /* tell DMAC don't touch memory */
>  		return -ENOMEM;
>  	}
> -	descr->buf_size = cpu_to_be32(bufsize);
> +	descr->buf_size = cpu_to_be32(gelic_rx_skb_size);
>  	descr->dmac_cmd_status = 0;
>  	descr->result_size = 0;
>  	descr->valid_size = 0;

The part I missed was the lines of code that didn't make it into the
patch that like between the two code blocks here above and below. They
are doing an AND with your align mask and then adding the difference to
the skb reserve to pad it to be 128B aligned.

> @@ -397,7 +400,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>  	/* io-mmu-map the skb */
>  	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
>  						     descr->skb->data,
> -						     GELIC_NET_MAX_MTU,
> +						     gelic_rx_skb_size,
>  						     DMA_FROM_DEVICE));
>  	if (!descr->buf_addr) {
>  		dev_kfree_skb_any(descr->skb);
> @@ -915,7 +918,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
>  	data_error = be32_to_cpu(descr->data_error);
>  	/* unmap skb buffer */
>  	dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
> -			 GELIC_NET_MAX_MTU,
> +			 gelic_rx_skb_size,
>  			 DMA_FROM_DEVICE);
>  
>  	skb_put(skb, be32_to_cpu(descr->valid_size)?
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> index 68f324ed4eaf..0d98defb011e 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> @@ -19,8 +19,9 @@
>  #define GELIC_NET_RX_DESCRIPTORS        128 /* num of descriptors */
>  #define GELIC_NET_TX_DESCRIPTORS        128 /* num of descriptors */
>  
> -#define GELIC_NET_MAX_MTU               VLAN_ETH_FRAME_LEN
> -#define GELIC_NET_MIN_MTU               VLAN_ETH_ZLEN
> +#define GELIC_NET_MAX_FRAME             2312
> +#define GELIC_NET_MAX_MTU               2294
> +#define GELIC_NET_MIN_MTU               64
>  #define GELIC_NET_RXBUF_ALIGN           128
>  #define GELIC_CARD_RX_CSUM_DEFAULT      1 /* hw chksum */
>  #define GELIC_NET_WATCHDOG_TIMEOUT      5*HZ
Geoff Levand March 18, 2023, 5:37 p.m. UTC | #2 
Hi,

Sorry for the delay in my reply, I was away last weekend and
couldn't do any PS3 work.

On 3/11/23 14:37, Alexander H Duyck wrote:
> On Sat, 2023-03-11 at 21:46 +0000, Geoff Levand wrote:
>> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
>> GELIC_NET_RXBUF_ALIGN, and also 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.
>>
>> Also, correct the maximum and minimum MTU sizes, and add a new
>> preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME.
>>
>> 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 | 21 +++++++++++---------
>>  drivers/net/ethernet/toshiba/ps3_gelic_net.h |  5 +++--
>>  2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index cf8de8a7a8a1..56557fc8d18a 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -44,6 +44,14 @@ MODULE_AUTHOR("SCE Inc.");
>>  MODULE_DESCRIPTION("Gelic Network driver");
>>  MODULE_LICENSE("GPL");
>>  
>> +/**
>> + * 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 const unsigned int gelic_rx_skb_size =
>> +	ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) +
>> +	GELIC_NET_RXBUF_ALIGN - 1;
>>
> 
> After a bit more digging I now understand the need for the
> "GELIC_NET_RXBUF_ALIGN - 1". It shouldn't be added here. The device
> will not be able to DMA into it. It is being used to align the buffer
> itself to 128B. I am assuming it must be 128B aligned in BOTH size and
> offset.

I want gelic_rx_skb_size to be the value to use with netdev_alloc_skb.

Also, I think just using GELIC_NET_MAX_FRAME for dma_map_single is
enough. Using gelic_rx_skb_size for dma_map_single would work, but the
gelic hardware device could only fill GELIC_NET_MAX_FRAME of the
bigger gelic_rx_skb_size DMA mapping.

>>  /* set irq_mask */
>>  int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
>> @@ -370,21 +378,16 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>>  				  struct gelic_descr *descr)
>>  {
>>  	int offset;
>> -	unsigned int bufsize;
>>  
>>  	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 = netdev_alloc_skb(*card->netdev, gelic_rx_skb_size);
> 
> I would leave the "+  GELIC_NET_RXBUF_ALIGN - 1" here. so it should be
> 	descr->skb = netdev_alloc_skb(*card->netdev, 		
> 				      gelic_rx_skb_size +
> 				      GELIC_NET_RXBUF_ALIGN - 1);
> 
> Also I would leav the comment as it makes it a bit clearer what is
> going on here.

With the use of GELIC_NET_MAX_FRAME as the DMA mapping size
gelic_rx_skb_size becomes local to the gelic_descr_prepare_rx
routine, so I put the comment about the size requirements into
the the comment for the gelic_descr_prepare_rx routine.

> 
>>  	if (!descr->skb) {
>>  		descr->buf_addr = 0; /* tell DMAC don't touch memory */
>>  		return -ENOMEM;
>>  	}
>> -	descr->buf_size = cpu_to_be32(bufsize);
>> +	descr->buf_size = cpu_to_be32(gelic_rx_skb_size);
>>  	descr->dmac_cmd_status = 0;
>>  	descr->result_size = 0;
>>  	descr->valid_size = 0;
> 
> The part I missed was the lines of code that didn't make it into the
> patch that like between the two code blocks here above and below. They
> are doing an AND with your align mask and then adding the difference to
> the skb reserve to pad it to be 128B aligned.
> 
>> @@ -397,7 +400,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>>  	/* io-mmu-map the skb */
>>  	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
>>  						     descr->skb->data,
>> -						     GELIC_NET_MAX_MTU,
>> +						     gelic_rx_skb_size,
>>  						     DMA_FROM_DEVICE));
>>  	if (!descr->buf_addr) {
>>  		dev_kfree_skb_any(descr->skb);
>> @@ -915,7 +918,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
>>  	data_error = be32_to_cpu(descr->data_error);
>>  	/* unmap skb buffer */
>>  	dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
>> -			 GELIC_NET_MAX_MTU,
>> +			 gelic_rx_skb_size,
>>  			 DMA_FROM_DEVICE);
>>  
>>  	skb_put(skb, be32_to_cpu(descr->valid_size)?
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
>> index 68f324ed4eaf..0d98defb011e 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
>> @@ -19,8 +19,9 @@
>>  #define GELIC_NET_RX_DESCRIPTORS        128 /* num of descriptors */
>>  #define GELIC_NET_TX_DESCRIPTORS        128 /* num of descriptors */
>>  
>> -#define GELIC_NET_MAX_MTU               VLAN_ETH_FRAME_LEN
>> -#define GELIC_NET_MIN_MTU               VLAN_ETH_ZLEN
>> +#define GELIC_NET_MAX_FRAME             2312
>> +#define GELIC_NET_MAX_MTU               2294
>> +#define GELIC_NET_MIN_MTU               64
>>  #define GELIC_NET_RXBUF_ALIGN           128
>>  #define GELIC_CARD_RX_CSUM_DEFAULT      1 /* hw chksum */
>>  #define GELIC_NET_WATCHDOG_TIMEOUT      5*HZ

-Geoff
diff mbox series

Patch

diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index cf8de8a7a8a1..56557fc8d18a 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -44,6 +44,14 @@  MODULE_AUTHOR("SCE Inc.");
 MODULE_DESCRIPTION("Gelic Network driver");
 MODULE_LICENSE("GPL");
 
+/**
+ * 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 const unsigned int gelic_rx_skb_size =
+	ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) +
+	GELIC_NET_RXBUF_ALIGN - 1;
 
 /* set irq_mask */
 int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
@@ -370,21 +378,16 @@  static int gelic_descr_prepare_rx(struct gelic_card *card,
 				  struct gelic_descr *descr)
 {
 	int offset;
-	unsigned int bufsize;
 
 	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 = netdev_alloc_skb(*card->netdev, gelic_rx_skb_size);
 	if (!descr->skb) {
 		descr->buf_addr = 0; /* tell DMAC don't touch memory */
 		return -ENOMEM;
 	}
-	descr->buf_size = cpu_to_be32(bufsize);
+	descr->buf_size = cpu_to_be32(gelic_rx_skb_size);
 	descr->dmac_cmd_status = 0;
 	descr->result_size = 0;
 	descr->valid_size = 0;
@@ -397,7 +400,7 @@  static int gelic_descr_prepare_rx(struct gelic_card *card,
 	/* io-mmu-map the skb */
 	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
 						     descr->skb->data,
-						     GELIC_NET_MAX_MTU,
+						     gelic_rx_skb_size,
 						     DMA_FROM_DEVICE));
 	if (!descr->buf_addr) {
 		dev_kfree_skb_any(descr->skb);
@@ -915,7 +918,7 @@  static void gelic_net_pass_skb_up(struct gelic_descr *descr,
 	data_error = be32_to_cpu(descr->data_error);
 	/* unmap skb buffer */
 	dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
-			 GELIC_NET_MAX_MTU,
+			 gelic_rx_skb_size,
 			 DMA_FROM_DEVICE);
 
 	skb_put(skb, be32_to_cpu(descr->valid_size)?
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
index 68f324ed4eaf..0d98defb011e 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
@@ -19,8 +19,9 @@ 
 #define GELIC_NET_RX_DESCRIPTORS        128 /* num of descriptors */
 #define GELIC_NET_TX_DESCRIPTORS        128 /* num of descriptors */
 
-#define GELIC_NET_MAX_MTU               VLAN_ETH_FRAME_LEN
-#define GELIC_NET_MIN_MTU               VLAN_ETH_ZLEN
+#define GELIC_NET_MAX_FRAME             2312
+#define GELIC_NET_MAX_MTU               2294
+#define GELIC_NET_MIN_MTU               64
 #define GELIC_NET_RXBUF_ALIGN           128
 #define GELIC_CARD_RX_CSUM_DEFAULT      1 /* hw chksum */
 #define GELIC_NET_WATCHDOG_TIMEOUT      5*HZ