[PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[Sean Anderson]

If coalece_count is greater than 255 it will not fit in the register and
will overflow. This can be reproduced by running

    # ethtool -C ethX rx-frames 256

which will result in a timeout of 0us instead. Fix this by clamping the
counts to the maximum value.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
---

Changes in v2:
- Use FIELD_MAX to extract the max value from the mask
- Expand the commit message with an example on how to reproduce this
  issue

 drivers/net/ethernet/xilinx/xilinx_axienet.h      | 5 ++---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
index 1223fcc1a8da..54db69893565 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
@@ -109,11 +109,10 @@
 #define XAXIDMA_BD_CTRL_TXEOF_MASK	0x04000000 /* Last tx packet */
 #define XAXIDMA_BD_CTRL_ALL_MASK	0x0C000000 /* All control bits */
 
-#define XAXIDMA_DELAY_MASK		0xFF000000 /* Delay timeout counter */
-#define XAXIDMA_COALESCE_MASK		0x00FF0000 /* Coalesce counter */
+#define XAXIDMA_DELAY_MASK		((u32)0xFF000000) /* Delay timeout counter */
+#define XAXIDMA_COALESCE_MASK		((u32)0x00FF0000) /* Coalesce counter */
 
 #define XAXIDMA_DELAY_SHIFT		24
-#define XAXIDMA_COALESCE_SHIFT		16
 
 #define XAXIDMA_IRQ_IOC_MASK		0x00001000 /* Completion intr */
 #define XAXIDMA_IRQ_DELAY_MASK		0x00002000 /* Delay interrupt */
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 9eb300fc3590..89b63695293d 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local *lp, u32 coalesce_usec)
 static void axienet_dma_start(struct axienet_local *lp)
 {
 	/* Start updating the Rx channel control register */
-	lp->rx_dma_cr = (lp->coalesce_count_rx << XAXIDMA_COALESCE_SHIFT) |
+	lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
+				   min(lp->coalesce_count_rx,
+				       FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
 			XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_ERROR_MASK;
 	/* Only set interrupt delay timer if not generating an interrupt on
 	 * the first RX packet. Otherwise leave at 0 to disable delay interrupt.
@@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local *lp)
 	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);
 
 	/* Start updating the Tx channel control register */
-	lp->tx_dma_cr = (lp->coalesce_count_tx << XAXIDMA_COALESCE_SHIFT) |
+	lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
+				   min(lp->coalesce_count_tx,
+				       FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
 			XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_ERROR_MASK;
 	/* Only set interrupt delay timer if not generating an interrupt on
 	 * the first TX packet. Otherwise leave at 0 to disable delay interrupt.
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[Nelson, Shannon]

On 9/9/2024 4:09 PM, Sean Anderson wrote:
> 
> If coalece_count is greater than 255 it will not fit in the register and

s/coalece_count/coalesce_count/

Otherwise

Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>


> will overflow. This can be reproduced by running
> 
>      # ethtool -C ethX rx-frames 256
> 
> which will result in a timeout of 0us instead. Fix this by clamping the
> counts to the maximum value.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
> ---
> 
> Changes in v2:
> - Use FIELD_MAX to extract the max value from the mask
> - Expand the commit message with an example on how to reproduce this
>    issue
> 
>   drivers/net/ethernet/xilinx/xilinx_axienet.h      | 5 ++---
>   drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++--
>   2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> index 1223fcc1a8da..54db69893565 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> @@ -109,11 +109,10 @@
>   #define XAXIDMA_BD_CTRL_TXEOF_MASK     0x04000000 /* Last tx packet */
>   #define XAXIDMA_BD_CTRL_ALL_MASK       0x0C000000 /* All control bits */
> 
> -#define XAXIDMA_DELAY_MASK             0xFF000000 /* Delay timeout counter */
> -#define XAXIDMA_COALESCE_MASK          0x00FF0000 /* Coalesce counter */
> +#define XAXIDMA_DELAY_MASK             ((u32)0xFF000000) /* Delay timeout counter */
> +#define XAXIDMA_COALESCE_MASK          ((u32)0x00FF0000) /* Coalesce counter */
> 
>   #define XAXIDMA_DELAY_SHIFT            24
> -#define XAXIDMA_COALESCE_SHIFT         16
> 
>   #define XAXIDMA_IRQ_IOC_MASK           0x00001000 /* Completion intr */
>   #define XAXIDMA_IRQ_DELAY_MASK         0x00002000 /* Delay interrupt */
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 9eb300fc3590..89b63695293d 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local *lp, u32 coalesce_usec)
>   static void axienet_dma_start(struct axienet_local *lp)
>   {
>          /* Start updating the Rx channel control register */
> -       lp->rx_dma_cr = (lp->coalesce_count_rx << XAXIDMA_COALESCE_SHIFT) |
> +       lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
> +                                  min(lp->coalesce_count_rx,
> +                                      FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>                          XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_ERROR_MASK;
>          /* Only set interrupt delay timer if not generating an interrupt on
>           * the first RX packet. Otherwise leave at 0 to disable delay interrupt.
> @@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local *lp)
>          axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);
> 
>          /* Start updating the Tx channel control register */
> -       lp->tx_dma_cr = (lp->coalesce_count_tx << XAXIDMA_COALESCE_SHIFT) |
> +       lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
> +                                  min(lp->coalesce_count_tx,
> +                                      FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>                          XAXIDMA_IRQ_IOC_MASK | XAXIDMA_IRQ_ERROR_MASK;
>          /* Only set interrupt delay timer if not generating an interrupt on
>           * the first TX packet. Otherwise leave at 0 to disable delay interrupt.
> --
> 2.35.1.1320.gc452695387.dirty
> 
> 

RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Tuesday, September 10, 2024 4:39 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S .
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> netdev@vger.kernel.org
> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>;
> Daniel Borkmann <daniel@iogearbox.net>; linux-arm-
> kernel@lists.infradead.org; Simek, Michal <michal.simek@amd.com>; Sean
> Anderson <sean.anderson@linux.dev>
> Subject: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
> overflow
> 
> If coalece_count is greater than 255 it will not fit in the register and
> will overflow. This can be reproduced by running
> 
>     # ethtool -C ethX rx-frames 256
> 
> which will result in a timeout of 0us instead. Fix this by clamping the
> counts to the maximum value.
After this fix - what is o/p we get on rx-frames read? I think silent clamping is not a great 
idea and user won't know about it.  One alternative is to add check in set_coalesc 
count for valid range? (Similar to axienet_ethtools_set_ringparam so that user is notified 
for incorrect range)

> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet
> driver")
> ---
> 
> Changes in v2:
> - Use FIELD_MAX to extract the max value from the mask
> - Expand the commit message with an example on how to reproduce this
>   issue
> 
>  drivers/net/ethernet/xilinx/xilinx_axienet.h      | 5 ++---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++--
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> index 1223fcc1a8da..54db69893565 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> @@ -109,11 +109,10 @@
>  #define XAXIDMA_BD_CTRL_TXEOF_MASK	0x04000000 /* Last tx packet
> */
>  #define XAXIDMA_BD_CTRL_ALL_MASK	0x0C000000 /* All control bits
> */
> 
> -#define XAXIDMA_DELAY_MASK		0xFF000000 /* Delay timeout
> counter */
> -#define XAXIDMA_COALESCE_MASK		0x00FF0000 /* Coalesce
> counter */
> +#define XAXIDMA_DELAY_MASK		((u32)0xFF000000) /* Delay
> timeout counter */

Adding typecast here looks odd. Any reason for it? 
If needed we do it in specific case where it is required.

> +#define XAXIDMA_COALESCE_MASK		((u32)0x00FF0000) /*
> Coalesce counter */
> 
>  #define XAXIDMA_DELAY_SHIFT		24
> -#define XAXIDMA_COALESCE_SHIFT		16
> 
>  #define XAXIDMA_IRQ_IOC_MASK		0x00001000 /* Completion
> intr */
>  #define XAXIDMA_IRQ_DELAY_MASK		0x00002000 /* Delay
> interrupt */
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 9eb300fc3590..89b63695293d 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local
> *lp, u32 coalesce_usec)
>  static void axienet_dma_start(struct axienet_local *lp)
>  {
>  	/* Start updating the Rx channel control register */
> -	lp->rx_dma_cr = (lp->coalesce_count_rx <<
> XAXIDMA_COALESCE_SHIFT) |
> +	lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
> +				   min(lp->coalesce_count_rx,
> +
> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>  			XAXIDMA_IRQ_IOC_MASK |
> XAXIDMA_IRQ_ERROR_MASK;
>  	/* Only set interrupt delay timer if not generating an interrupt on
>  	 * the first RX packet. Otherwise leave at 0 to disable delay interrupt.
> @@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local
> *lp)
>  	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);
> 
>  	/* Start updating the Tx channel control register */
> -	lp->tx_dma_cr = (lp->coalesce_count_tx <<
> XAXIDMA_COALESCE_SHIFT) |
> +	lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
> +				   min(lp->coalesce_count_tx,
> +
> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>  			XAXIDMA_IRQ_IOC_MASK |
> XAXIDMA_IRQ_ERROR_MASK;
>  	/* Only set interrupt delay timer if not generating an interrupt on
>  	 * the first TX packet. Otherwise leave at 0 to disable delay interrupt.
> --
> 2.35.1.1320.gc452695387.dirty

Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[Sean Anderson]

On 9/11/24 03:01, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Tuesday, September 10, 2024 4:39 AM
>> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S .
>> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
>> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
>> netdev@vger.kernel.org
>> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
>> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>;
>> Daniel Borkmann <daniel@iogearbox.net>; linux-arm-
>> kernel@lists.infradead.org; Simek, Michal <michal.simek@amd.com>; Sean
>> Anderson <sean.anderson@linux.dev>
>> Subject: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
>> overflow
>> 
>> If coalece_count is greater than 255 it will not fit in the register and
>> will overflow. This can be reproduced by running
>> 
>>     # ethtool -C ethX rx-frames 256
>> 
>> which will result in a timeout of 0us instead. Fix this by clamping the
>> counts to the maximum value.
> After this fix - what is o/p we get on rx-frames read? I think silent clamping is not a great 
> idea and user won't know about it.  One alternative is to add check in set_coalesc 
> count for valid range? (Similar to axienet_ethtools_set_ringparam so that user is notified 
> for incorrect range)

The value reported will be unclamped. In [1] I improve the driver to
return the actual (clamped) value.

Remember that without this commit, we have silent wraparound instead. I
think clamping is much friendlier, since you at least get something
close to the rx-frames value, instead of zero!

This commit is just a fix for the overflow issue. To ensure it is
appropriate for backporting I have omitted any other
changes/improvements.

--Sean

[1] https://lore.kernel.org/netdev/20240909235208.1331065-6-sean.anderson@linux.dev/

>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet
>> driver")
>> ---
>> 
>> Changes in v2:
>> - Use FIELD_MAX to extract the max value from the mask
>> - Expand the commit message with an example on how to reproduce this
>>   issue
>> 
>>  drivers/net/ethernet/xilinx/xilinx_axienet.h      | 5 ++---
>>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++--
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> index 1223fcc1a8da..54db69893565 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> @@ -109,11 +109,10 @@
>>  #define XAXIDMA_BD_CTRL_TXEOF_MASK	0x04000000 /* Last tx packet
>> */
>>  #define XAXIDMA_BD_CTRL_ALL_MASK	0x0C000000 /* All control bits
>> */
>> 
>> -#define XAXIDMA_DELAY_MASK		0xFF000000 /* Delay timeout
>> counter */
>> -#define XAXIDMA_COALESCE_MASK		0x00FF0000 /* Coalesce
>> counter */
>> +#define XAXIDMA_DELAY_MASK		((u32)0xFF000000) /* Delay
>> timeout counter */
> 
> Adding typecast here looks odd. Any reason for it? 
> If needed we do it in specific case where it is required.
> 
>> +#define XAXIDMA_COALESCE_MASK		((u32)0x00FF0000) /*
>> Coalesce counter */
>> 
>>  #define XAXIDMA_DELAY_SHIFT		24
>> -#define XAXIDMA_COALESCE_SHIFT		16
>> 
>>  #define XAXIDMA_IRQ_IOC_MASK		0x00001000 /* Completion
>> intr */
>>  #define XAXIDMA_IRQ_DELAY_MASK		0x00002000 /* Delay
>> interrupt */
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> index 9eb300fc3590..89b63695293d 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> @@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local
>> *lp, u32 coalesce_usec)
>>  static void axienet_dma_start(struct axienet_local *lp)
>>  {
>>  	/* Start updating the Rx channel control register */
>> -	lp->rx_dma_cr = (lp->coalesce_count_rx <<
>> XAXIDMA_COALESCE_SHIFT) |
>> +	lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
>> +				   min(lp->coalesce_count_rx,
>> +
>> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>>  			XAXIDMA_IRQ_IOC_MASK |
>> XAXIDMA_IRQ_ERROR_MASK;
>>  	/* Only set interrupt delay timer if not generating an interrupt on
>>  	 * the first RX packet. Otherwise leave at 0 to disable delay interrupt.
>> @@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local
>> *lp)
>>  	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);
>> 
>>  	/* Start updating the Tx channel control register */
>> -	lp->tx_dma_cr = (lp->coalesce_count_tx <<
>> XAXIDMA_COALESCE_SHIFT) |
>> +	lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
>> +				   min(lp->coalesce_count_tx,
>> +
>> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>>  			XAXIDMA_IRQ_IOC_MASK |
>> XAXIDMA_IRQ_ERROR_MASK;
>>  	/* Only set interrupt delay timer if not generating an interrupt on
>>  	 * the first TX packet. Otherwise leave at 0 to disable delay interrupt.
>> --
>> 2.35.1.1320.gc452695387.dirty
> 

RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Thursday, September 12, 2024 8:01 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S . Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org;
> Gupta, Suraj <Suraj.Gupta2@amd.com>; Katakam, Harini
> <harini.katakam@amd.com>
> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>; Daniel
> Borkmann <daniel@iogearbox.net>; linux-arm-kernel@lists.infradead.org; Simek,
> Michal <michal.simek@amd.com>
> Subject: Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
> overflow
> 
> On 9/11/24 03:01, Pandey, Radhey Shyam wrote:
> >> -----Original Message-----
> >> From: Sean Anderson <sean.anderson@linux.dev>
> >> Sent: Tuesday, September 10, 2024 4:39 AM
> >> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S .
> >> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> >> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> >> netdev@vger.kernel.org
> >> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
> >> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>;
> >> Daniel Borkmann <daniel@iogearbox.net>; linux-arm-
> >> kernel@lists.infradead.org; Simek, Michal <michal.simek@amd.com>; Sean
> >> Anderson <sean.anderson@linux.dev>
> >> Subject: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
> >> overflow
> >>
> >> If coalece_count is greater than 255 it will not fit in the register and
> >> will overflow. This can be reproduced by running
> >>
> >>     # ethtool -C ethX rx-frames 256
> >>
> >> which will result in a timeout of 0us instead. Fix this by clamping the
> >> counts to the maximum value.
> > After this fix - what is o/p we get on rx-frames read? I think silent clamping is not a
> great
> > idea and user won't know about it.  One alternative is to add check in set_coalesc
> > count for valid range? (Similar to axienet_ethtools_set_ringparam so that user is
> notified
> > for incorrect range)
> 
> The value reported will be unclamped. In [1] I improve the driver to
> return the actual (clamped) value.
> 
> Remember that without this commit, we have silent wraparound instead. I
> think clamping is much friendlier, since you at least get something
> close to the rx-frames value, instead of zero!
> 
> This commit is just a fix for the overflow issue. To ensure it is
> appropriate for backporting I have omitted any other
> changes/improvements.

But the point is the fix also can be to avoid setting coalesce count 
to invalid (or not supported range) value - like done in existing 
axienet_ethtools_set_ringparam() implementation.

And we don't clamp on every dma_start().

> 
> --Sean
> 
> [1] https://lore.kernel.org/netdev/20240909235208.1331065-6-
> sean.anderson@linux.dev/
> 
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet
> >> driver")
> >> ---
> >>
> >> Changes in v2:
> >> - Use FIELD_MAX to extract the max value from the mask
> >> - Expand the commit message with an example on how to reproduce this
> >>   issue
> >>
> >>  drivers/net/ethernet/xilinx/xilinx_axienet.h      | 5 ++---
> >>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++--
> >>  2 files changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> >> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> >> index 1223fcc1a8da..54db69893565 100644
> >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> >> @@ -109,11 +109,10 @@
> >>  #define XAXIDMA_BD_CTRL_TXEOF_MASK	0x04000000 /* Last tx packet
> >> */
> >>  #define XAXIDMA_BD_CTRL_ALL_MASK	0x0C000000 /* All control bits
> >> */
> >>
> >> -#define XAXIDMA_DELAY_MASK		0xFF000000 /* Delay timeout
> >> counter */
> >> -#define XAXIDMA_COALESCE_MASK		0x00FF0000 /* Coalesce
> >> counter */
> >> +#define XAXIDMA_DELAY_MASK		((u32)0xFF000000) /* Delay
> >> timeout counter */
> >
> > Adding typecast here looks odd. Any reason for it?
> > If needed we do it in specific case where it is required.
> >
> >> +#define XAXIDMA_COALESCE_MASK		((u32)0x00FF0000) /*
> >> Coalesce counter */
> >>
> >>  #define XAXIDMA_DELAY_SHIFT		24
> >> -#define XAXIDMA_COALESCE_SHIFT		16
> >>
> >>  #define XAXIDMA_IRQ_IOC_MASK		0x00001000 /* Completion
> >> intr */
> >>  #define XAXIDMA_IRQ_DELAY_MASK		0x00002000 /* Delay
> >> interrupt */
> >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> >> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> >> index 9eb300fc3590..89b63695293d 100644
> >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> >> @@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local
> >> *lp, u32 coalesce_usec)
> >>  static void axienet_dma_start(struct axienet_local *lp)
> >>  {
> >>  	/* Start updating the Rx channel control register */
> >> -	lp->rx_dma_cr = (lp->coalesce_count_rx <<
> >> XAXIDMA_COALESCE_SHIFT) |
> >> +	lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
> >> +				   min(lp->coalesce_count_rx,
> >> +
> >> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
> >>  			XAXIDMA_IRQ_IOC_MASK |
> >> XAXIDMA_IRQ_ERROR_MASK;
> >>  	/* Only set interrupt delay timer if not generating an interrupt on
> >>  	 * the first RX packet. Otherwise leave at 0 to disable delay interrupt.
> >> @@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local
> >> *lp)
> >>  	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);
> >>
> >>  	/* Start updating the Tx channel control register */
> >> -	lp->tx_dma_cr = (lp->coalesce_count_tx <<
> >> XAXIDMA_COALESCE_SHIFT) |
> >> +	lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
> >> +				   min(lp->coalesce_count_tx,
> >> +
> >> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
> >>  			XAXIDMA_IRQ_IOC_MASK |
> >> XAXIDMA_IRQ_ERROR_MASK;
> >>  	/* Only set interrupt delay timer if not generating an interrupt on
> >>  	 * the first TX packet. Otherwise leave at 0 to disable delay interrupt.
> >> --
> >> 2.35.1.1320.gc452695387.dirty
> >

Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[Sean Anderson]

On 9/11/24 03:01, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Tuesday, September 10, 2024 4:39 AM
>> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S .
>> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
>> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
>> netdev@vger.kernel.org
>> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
>> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>;
>> Daniel Borkmann <daniel@iogearbox.net>; linux-arm-
>> kernel@lists.infradead.org; Simek, Michal <michal.simek@amd.com>; Sean
>> Anderson <sean.anderson@linux.dev>
>> Subject: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
>> overflow
>> 
>> If coalece_count is greater than 255 it will not fit in the register and
>> will overflow. This can be reproduced by running
>> 
>>     # ethtool -C ethX rx-frames 256
>> 
>> which will result in a timeout of 0us instead. Fix this by clamping the
>> counts to the maximum value.
> After this fix - what is o/p we get on rx-frames read? I think silent clamping is not a great 
> idea and user won't know about it.  One alternative is to add check in set_coalesc 
> count for valid range? (Similar to axienet_ethtools_set_ringparam so that user is notified 
> for incorrect range)
> 
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet
>> driver")
>> ---
>> 
>> Changes in v2:
>> - Use FIELD_MAX to extract the max value from the mask
>> - Expand the commit message with an example on how to reproduce this
>>   issue
>> 
>>  drivers/net/ethernet/xilinx/xilinx_axienet.h      | 5 ++---
>>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++--
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> index 1223fcc1a8da..54db69893565 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> @@ -109,11 +109,10 @@
>>  #define XAXIDMA_BD_CTRL_TXEOF_MASK	0x04000000 /* Last tx packet
>> */
>>  #define XAXIDMA_BD_CTRL_ALL_MASK	0x0C000000 /* All control bits
>> */
>> 
>> -#define XAXIDMA_DELAY_MASK		0xFF000000 /* Delay timeout
>> counter */
>> -#define XAXIDMA_COALESCE_MASK		0x00FF0000 /* Coalesce
>> counter */
>> +#define XAXIDMA_DELAY_MASK		((u32)0xFF000000) /* Delay
>> timeout counter */
> 
> Adding typecast here looks odd. Any reason for it? 
> If needed we do it in specific case where it is required.

Sorry, I missed this on my first read.

This is necessary for...

>> +#define XAXIDMA_COALESCE_MASK		((u32)0x00FF0000) /*
>> Coalesce counter */
>> 
>>  #define XAXIDMA_DELAY_SHIFT		24
>> -#define XAXIDMA_COALESCE_SHIFT		16
>> 
>>  #define XAXIDMA_IRQ_IOC_MASK		0x00001000 /* Completion
>> intr */
>>  #define XAXIDMA_IRQ_DELAY_MASK		0x00002000 /* Delay
>> interrupt */
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> index 9eb300fc3590..89b63695293d 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> @@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local
>> *lp, u32 coalesce_usec)
>>  static void axienet_dma_start(struct axienet_local *lp)
>>  {
>>  	/* Start updating the Rx channel control register */
>> -	lp->rx_dma_cr = (lp->coalesce_count_rx <<
>> XAXIDMA_COALESCE_SHIFT) |
>> +	lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
>> +				   min(lp->coalesce_count_rx,
>> +
>> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |

this expression. Since min will complain if the types of its arguments
differ. Since coalesce_count_rx is a u32, and integer constants default
to int, I cast the mask to u32 above. Although a U suffix would have
also worked.

--Sean

>>  			XAXIDMA_IRQ_IOC_MASK |
>> XAXIDMA_IRQ_ERROR_MASK;
>>  	/* Only set interrupt delay timer if not generating an interrupt on
>>  	 * the first RX packet. Otherwise leave at 0 to disable delay interrupt.
>> @@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local
>> *lp)
>>  	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);
>> 
>>  	/* Start updating the Tx channel control register */
>> -	lp->tx_dma_cr = (lp->coalesce_count_tx <<
>> XAXIDMA_COALESCE_SHIFT) |
>> +	lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
>> +				   min(lp->coalesce_count_tx,
>> +
>> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>>  			XAXIDMA_IRQ_IOC_MASK |
>> XAXIDMA_IRQ_ERROR_MASK;
>>  	/* Only set interrupt delay timer if not generating an interrupt on
>>  	 * the first TX packet. Otherwise leave at 0 to disable delay interrupt.
>> --
>> 2.35.1.1320.gc452695387.dirty
> 

RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Pandey, Radhey Shyam
> Sent: Thursday, September 12, 2024 8:05 PM
> To: Sean Anderson <sean.anderson@linux.dev>; David S . Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org;
> Gupta, Suraj <Suraj.Gupta2@amd.com>; Katakam, Harini
> <harini.katakam@amd.com>
> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>; Daniel
> Borkmann <daniel@iogearbox.net>; linux-arm-kernel@lists.infradead.org; Simek,
> Michal <michal.simek@amd.com>
> Subject: RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
> overflow
> 
> > -----Original Message-----
> > From: Sean Anderson <sean.anderson@linux.dev>
> > Sent: Thursday, September 12, 2024 8:01 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S . Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski
> > <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> netdev@vger.kernel.org;
> > Gupta, Suraj <Suraj.Gupta2@amd.com>; Katakam, Harini
> > <harini.katakam@amd.com>
> > Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
> > Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>;
> Daniel
> > Borkmann <daniel@iogearbox.net>; linux-arm-kernel@lists.infradead.org; Simek,
> > Michal <michal.simek@amd.com>
> > Subject: Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
> > overflow
> >
> > On 9/11/24 03:01, Pandey, Radhey Shyam wrote:
> > >> -----Original Message-----
> > >> From: Sean Anderson <sean.anderson@linux.dev>
> > >> Sent: Tuesday, September 10, 2024 4:39 AM
> > >> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S .
> > >> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> > >> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> > >> netdev@vger.kernel.org
> > >> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
> > >> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>;
> > >> Daniel Borkmann <daniel@iogearbox.net>; linux-arm-
> > >> kernel@lists.infradead.org; Simek, Michal <michal.simek@amd.com>; Sean
> > >> Anderson <sean.anderson@linux.dev>
> > >> Subject: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
> > >> overflow
> > >>
> > >> If coalece_count is greater than 255 it will not fit in the register and
> > >> will overflow. This can be reproduced by running
> > >>
> > >>     # ethtool -C ethX rx-frames 256
> > >>
> > >> which will result in a timeout of 0us instead. Fix this by clamping the
> > >> counts to the maximum value.
> > > After this fix - what is o/p we get on rx-frames read? I think silent clamping is not
> a
> > great
> > > idea and user won't know about it.  One alternative is to add check in
> set_coalesc
> > > count for valid range? (Similar to axienet_ethtools_set_ringparam so that user is
> > notified
> > > for incorrect range)
> >
> > The value reported will be unclamped. In [1] I improve the driver to
> > return the actual (clamped) value.
> >
> > Remember that without this commit, we have silent wraparound instead. I
> > think clamping is much friendlier, since you at least get something
> > close to the rx-frames value, instead of zero!
> >
> > This commit is just a fix for the overflow issue. To ensure it is
> > appropriate for backporting I have omitted any other
> > changes/improvements.
> 
> But the point is the fix also can be to avoid setting coalesce count
> to invalid (or not supported range) value - like done in existing
> axienet_ethtools_set_ringparam() implementation.

Sean: I think above comment got missed out, so I am bringing it again
to discuss and close on it.

> 
> And we don't clamp on every dma_start().
> 
> >
> > --Sean
> >
> > [1] https://lore.kernel.org/netdev/20240909235208.1331065-6-
> > sean.anderson@linux.dev/
> >
> > >>
> > >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> > >> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet
> > >> driver")
> > >> ---
> > >>
> > >> Changes in v2:
> > >> - Use FIELD_MAX to extract the max value from the mask
> > >> - Expand the commit message with an example on how to reproduce this
> > >>   issue
> > >>
> > >>  drivers/net/ethernet/xilinx/xilinx_axienet.h      | 5 ++---
> > >>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++--
> > >>  2 files changed, 8 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > >> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > >> index 1223fcc1a8da..54db69893565 100644
> > >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > >> @@ -109,11 +109,10 @@
> > >>  #define XAXIDMA_BD_CTRL_TXEOF_MASK	0x04000000 /* Last tx packet
> > >> */
> > >>  #define XAXIDMA_BD_CTRL_ALL_MASK	0x0C000000 /* All control bits
> > >> */
> > >>
> > >> -#define XAXIDMA_DELAY_MASK		0xFF000000 /* Delay timeout
> > >> counter */
> > >> -#define XAXIDMA_COALESCE_MASK		0x00FF0000 /* Coalesce
> > >> counter */
> > >> +#define XAXIDMA_DELAY_MASK		((u32)0xFF000000) /* Delay
> > >> timeout counter */
> > >
> > > Adding typecast here looks odd. Any reason for it?
> > > If needed we do it in specific case where it is required.
> > >
> > >> +#define XAXIDMA_COALESCE_MASK		((u32)0x00FF0000) /*
> > >> Coalesce counter */
> > >>
> > >>  #define XAXIDMA_DELAY_SHIFT		24
> > >> -#define XAXIDMA_COALESCE_SHIFT		16
> > >>
> > >>  #define XAXIDMA_IRQ_IOC_MASK		0x00001000 /* Completion
> > >> intr */
> > >>  #define XAXIDMA_IRQ_DELAY_MASK		0x00002000 /* Delay
> > >> interrupt */
> > >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > >> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > >> index 9eb300fc3590..89b63695293d 100644
> > >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > >> @@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local
> > >> *lp, u32 coalesce_usec)
> > >>  static void axienet_dma_start(struct axienet_local *lp)
> > >>  {
> > >>  	/* Start updating the Rx channel control register */
> > >> -	lp->rx_dma_cr = (lp->coalesce_count_rx <<
> > >> XAXIDMA_COALESCE_SHIFT) |
> > >> +	lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
> > >> +				   min(lp->coalesce_count_rx,
> > >> +
> > >> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
> > >>  			XAXIDMA_IRQ_IOC_MASK |
> > >> XAXIDMA_IRQ_ERROR_MASK;
> > >>  	/* Only set interrupt delay timer if not generating an interrupt on
> > >>  	 * the first RX packet. Otherwise leave at 0 to disable delay interrupt.
> > >> @@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local
> > >> *lp)
> > >>  	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);
> > >>
> > >>  	/* Start updating the Tx channel control register */
> > >> -	lp->tx_dma_cr = (lp->coalesce_count_tx <<
> > >> XAXIDMA_COALESCE_SHIFT) |
> > >> +	lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
> > >> +				   min(lp->coalesce_count_tx,
> > >> +
> > >> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
> > >>  			XAXIDMA_IRQ_IOC_MASK |
> > >> XAXIDMA_IRQ_ERROR_MASK;
> > >>  	/* Only set interrupt delay timer if not generating an interrupt on
> > >>  	 * the first TX packet. Otherwise leave at 0 to disable delay interrupt.
> > >> --
> > >> 2.35.1.1320.gc452695387.dirty
> > >

Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow

[Sean Anderson]

On 9/17/24 07:24, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Pandey, Radhey Shyam
>> Sent: Thursday, September 12, 2024 8:05 PM
>> To: Sean Anderson <sean.anderson@linux.dev>; David S . Miller
>> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
>> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org;
>> Gupta, Suraj <Suraj.Gupta2@amd.com>; Katakam, Harini
>> <harini.katakam@amd.com>
>> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
>> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>; Daniel
>> Borkmann <daniel@iogearbox.net>; linux-arm-kernel@lists.infradead.org; Simek,
>> Michal <michal.simek@amd.com>
>> Subject: RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
>> overflow
>> 
>> > -----Original Message-----
>> > From: Sean Anderson <sean.anderson@linux.dev>
>> > Sent: Thursday, September 12, 2024 8:01 PM
>> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S . Miller
>> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
>> Kicinski
>> > <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
>> netdev@vger.kernel.org;
>> > Gupta, Suraj <Suraj.Gupta2@amd.com>; Katakam, Harini
>> > <harini.katakam@amd.com>
>> > Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
>> > Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>;
>> Daniel
>> > Borkmann <daniel@iogearbox.net>; linux-arm-kernel@lists.infradead.org; Simek,
>> > Michal <michal.simek@amd.com>
>> > Subject: Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
>> > overflow
>> >
>> > On 9/11/24 03:01, Pandey, Radhey Shyam wrote:
>> > >> -----Original Message-----
>> > >> From: Sean Anderson <sean.anderson@linux.dev>
>> > >> Sent: Tuesday, September 10, 2024 4:39 AM
>> > >> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S .
>> > >> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
>> > >> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
>> > >> netdev@vger.kernel.org
>> > >> Cc: Andy Chiu <andy.chiu@sifive.com>; linux-kernel@vger.kernel.org; Simon
>> > >> Horman <horms@kernel.org>; Ariane Keller <ariane.keller@tik.ee.ethz.ch>;
>> > >> Daniel Borkmann <daniel@iogearbox.net>; linux-arm-
>> > >> kernel@lists.infradead.org; Simek, Michal <michal.simek@amd.com>; Sean
>> > >> Anderson <sean.anderson@linux.dev>
>> > >> Subject: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
>> > >> overflow
>> > >>
>> > >> If coalece_count is greater than 255 it will not fit in the register and
>> > >> will overflow. This can be reproduced by running
>> > >>
>> > >>     # ethtool -C ethX rx-frames 256
>> > >>
>> > >> which will result in a timeout of 0us instead. Fix this by clamping the
>> > >> counts to the maximum value.
>> > > After this fix - what is o/p we get on rx-frames read? I think silent clamping is not
>> a
>> > great
>> > > idea and user won't know about it.  One alternative is to add check in
>> set_coalesc
>> > > count for valid range? (Similar to axienet_ethtools_set_ringparam so that user is
>> > notified
>> > > for incorrect range)
>> >
>> > The value reported will be unclamped. In [1] I improve the driver to
>> > return the actual (clamped) value.
>> >
>> > Remember that without this commit, we have silent wraparound instead. I
>> > think clamping is much friendlier, since you at least get something
>> > close to the rx-frames value, instead of zero!
>> >
>> > This commit is just a fix for the overflow issue. To ensure it is
>> > appropriate for backporting I have omitted any other
>> > changes/improvements.
>> 
>> But the point is the fix also can be to avoid setting coalesce count
>> to invalid (or not supported range) value - like done in existing
>> axienet_ethtools_set_ringparam() implementation.
> 
> Sean: I think above comment got missed out, so I am bringing it again
> to discuss and close on it.

I am investigating whether this will work within the broader context of
the changes I want to make. I will reply when I have had a chance to work
on it.

--Sean

>> 
>> And we don't clamp on every dma_start().
>> 
>> >
>> > --Sean
>> >
>> > [1] https://lore.kernel.org/netdev/20240909235208.1331065-6-
>> > sean.anderson@linux.dev/
>> >
>> > >>
>> > >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> > >> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet
>> > >> driver")
>> > >> ---
>> > >>
>> > >> Changes in v2:
>> > >> - Use FIELD_MAX to extract the max value from the mask
>> > >> - Expand the commit message with an example on how to reproduce this
>> > >>   issue
>> > >>
>> > >>  drivers/net/ethernet/xilinx/xilinx_axienet.h      | 5 ++---
>> > >>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++--
>> > >>  2 files changed, 8 insertions(+), 5 deletions(-)
>> > >>
>> > >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> > >> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> > >> index 1223fcc1a8da..54db69893565 100644
>> > >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> > >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> > >> @@ -109,11 +109,10 @@
>> > >>  #define XAXIDMA_BD_CTRL_TXEOF_MASK	0x04000000 /* Last tx packet
>> > >> */
>> > >>  #define XAXIDMA_BD_CTRL_ALL_MASK	0x0C000000 /* All control bits
>> > >> */
>> > >>
>> > >> -#define XAXIDMA_DELAY_MASK		0xFF000000 /* Delay timeout
>> > >> counter */
>> > >> -#define XAXIDMA_COALESCE_MASK		0x00FF0000 /* Coalesce
>> > >> counter */
>> > >> +#define XAXIDMA_DELAY_MASK		((u32)0xFF000000) /* Delay
>> > >> timeout counter */
>> > >
>> > > Adding typecast here looks odd. Any reason for it?
>> > > If needed we do it in specific case where it is required.
>> > >
>> > >> +#define XAXIDMA_COALESCE_MASK		((u32)0x00FF0000) /*
>> > >> Coalesce counter */
>> > >>
>> > >>  #define XAXIDMA_DELAY_SHIFT		24
>> > >> -#define XAXIDMA_COALESCE_SHIFT		16
>> > >>
>> > >>  #define XAXIDMA_IRQ_IOC_MASK		0x00001000 /* Completion
>> > >> intr */
>> > >>  #define XAXIDMA_IRQ_DELAY_MASK		0x00002000 /* Delay
>> > >> interrupt */
>> > >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> > >> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> > >> index 9eb300fc3590..89b63695293d 100644
>> > >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> > >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> > >> @@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local
>> > >> *lp, u32 coalesce_usec)
>> > >>  static void axienet_dma_start(struct axienet_local *lp)
>> > >>  {
>> > >>  	/* Start updating the Rx channel control register */
>> > >> -	lp->rx_dma_cr = (lp->coalesce_count_rx <<
>> > >> XAXIDMA_COALESCE_SHIFT) |
>> > >> +	lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
>> > >> +				   min(lp->coalesce_count_rx,
>> > >> +
>> > >> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>> > >>  			XAXIDMA_IRQ_IOC_MASK |
>> > >> XAXIDMA_IRQ_ERROR_MASK;
>> > >>  	/* Only set interrupt delay timer if not generating an interrupt on
>> > >>  	 * the first RX packet. Otherwise leave at 0 to disable delay interrupt.
>> > >> @@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local
>> > >> *lp)
>> > >>  	axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr);
>> > >>
>> > >>  	/* Start updating the Tx channel control register */
>> > >> -	lp->tx_dma_cr = (lp->coalesce_count_tx <<
>> > >> XAXIDMA_COALESCE_SHIFT) |
>> > >> +	lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK,
>> > >> +				   min(lp->coalesce_count_tx,
>> > >> +
>> > >> FIELD_MAX(XAXIDMA_COALESCE_MASK))) |
>> > >>  			XAXIDMA_IRQ_IOC_MASK |
>> > >> XAXIDMA_IRQ_ERROR_MASK;
>> > >>  	/* Only set interrupt delay timer if not generating an interrupt on
>> > >>  	 * the first TX packet. Otherwise leave at 0 to disable delay interrupt.
>> > >> --
>> > >> 2.35.1.1320.gc452695387.dirty
>> > >