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

From: Sean Anderson <[email-masked]>
Date: Tue, 17 Sep 2024 10:40:00 -0400

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 <[email-masked]>; David S . Miller
>> <[email-masked]>; Eric Dumazet <[email-masked]>; Jakub Kicinski
>> <[email-masked]>; Paolo Abeni <[email-masked]>; [email-masked];
>> Gupta, Suraj <[email-masked]>; Katakam, Harini
>> <[email-masked]>
>> Cc: Andy Chiu <[email-masked]>; [email-masked]; Simon
>> Horman <[email-masked]>; Ariane Keller <[email-masked]>; Daniel
>> Borkmann <[email-masked]>; [email-masked]; Simek,
>> Michal <[email-masked]>
>> Subject: RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count
>> overflow
>> 
>> > -----Original Message-----
>> > From: Sean Anderson <[email-masked]>
>> > Sent: Thursday, September 12, 2024 8:01 PM
>> > To: Pandey, Radhey Shyam <[email-masked]>; David S . Miller
>> > <[email-masked]>; Eric Dumazet <[email-masked]>; Jakub
>> Kicinski
>> > <[email-masked]>; Paolo Abeni <[email-masked]>;
>> [email-masked];
>> > Gupta, Suraj <[email-masked]>; Katakam, Harini
>> > <[email-masked]>
>> > Cc: Andy Chiu <[email-masked]>; [email-masked]; Simon
>> > Horman <[email-masked]>; Ariane Keller <[email-masked]>;
>> Daniel
>> > Borkmann <[email-masked]>; [email-masked]; Simek,
>> > Michal <[email-masked]>
>> > 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 <[email-masked]>
>> > >> Sent: Tuesday, September 10, 2024 4:39 AM
>> > >> To: Pandey, Radhey Shyam <[email-masked]>; David S .
>> > >> Miller <[email-masked]>; Eric Dumazet <[email-masked]>;
>> > >> Jakub Kicinski <[email-masked]>; Paolo Abeni <[email-masked]>;
>> > >> [email-masked]
>> > >> Cc: Andy Chiu <[email-masked]>; [email-masked]; Simon
>> > >> Horman <[email-masked]>; Ariane Keller <[email-masked]>;
>> > >> Daniel Borkmann <[email-masked]>; linux-arm-
>> > >> [email-masked]; Simek, Michal <[email-masked]>; Sean
>> > >> Anderson <[email-masked]>
>> > >> 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-
>> > [email-masked]/
>> >
>> > >>
>> > >> Signed-off-by: Sean Anderson <[email-masked]>
>> > >> 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
>> > >