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

[Sean Anderson <sean.anderson@linux.dev>]

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