* [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow
@ 2024-09-09 23:09 Sean Anderson
2024-09-10 0:51 ` Nelson, Shannon
2024-09-11 7:01 ` Pandey, Radhey Shyam
0 siblings, 2 replies; 8+ messages in thread
From: Sean Anderson @ 2024-09-09 23:09 UTC (permalink / raw)
To: Radhey Shyam Pandey, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
Cc: Andy Chiu, linux-kernel, Simon Horman, Ariane Keller,
Daniel Borkmann, linux-arm-kernel, Michal Simek, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow 2024-09-09 23:09 [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow Sean Anderson @ 2024-09-10 0:51 ` Nelson, Shannon 2024-09-11 7:01 ` Pandey, Radhey Shyam 1 sibling, 0 replies; 8+ messages in thread From: Nelson, Shannon @ 2024-09-10 0:51 UTC (permalink / raw) To: Sean Anderson, Radhey Shyam Pandey, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev Cc: Andy Chiu, linux-kernel, Simon Horman, Ariane Keller, Daniel Borkmann, linux-arm-kernel, Michal Simek 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow 2024-09-09 23:09 [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow Sean Anderson 2024-09-10 0:51 ` Nelson, Shannon @ 2024-09-11 7:01 ` Pandey, Radhey Shyam 2024-09-12 14:30 ` Sean Anderson 2024-09-13 16:51 ` Sean Anderson 1 sibling, 2 replies; 8+ messages in thread From: Pandey, Radhey Shyam @ 2024-09-11 7:01 UTC (permalink / raw) To: Sean Anderson, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org, Gupta, Suraj, Katakam, Harini Cc: Andy Chiu, linux-kernel@vger.kernel.org, Simon Horman, Ariane Keller, Daniel Borkmann, linux-arm-kernel@lists.infradead.org, Simek, Michal > -----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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow 2024-09-11 7:01 ` Pandey, Radhey Shyam @ 2024-09-12 14:30 ` Sean Anderson 2024-09-12 14:35 ` Pandey, Radhey Shyam 2024-09-13 16:51 ` Sean Anderson 1 sibling, 1 reply; 8+ messages in thread From: Sean Anderson @ 2024-09-12 14:30 UTC (permalink / raw) To: Pandey, Radhey Shyam, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org, Gupta, Suraj, Katakam, Harini Cc: Andy Chiu, linux-kernel@vger.kernel.org, Simon Horman, Ariane Keller, Daniel Borkmann, linux-arm-kernel@lists.infradead.org, Simek, Michal 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow 2024-09-12 14:30 ` Sean Anderson @ 2024-09-12 14:35 ` Pandey, Radhey Shyam 2024-09-17 11:24 ` Pandey, Radhey Shyam 0 siblings, 1 reply; 8+ messages in thread From: Pandey, Radhey Shyam @ 2024-09-12 14:35 UTC (permalink / raw) To: Sean Anderson, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org, Gupta, Suraj, Katakam, Harini Cc: Andy Chiu, linux-kernel@vger.kernel.org, Simon Horman, Ariane Keller, Daniel Borkmann, linux-arm-kernel@lists.infradead.org, Simek, Michal > -----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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow 2024-09-12 14:35 ` Pandey, Radhey Shyam @ 2024-09-17 11:24 ` Pandey, Radhey Shyam 2024-09-17 14:40 ` Sean Anderson 0 siblings, 1 reply; 8+ messages in thread From: Pandey, Radhey Shyam @ 2024-09-17 11:24 UTC (permalink / raw) To: Sean Anderson, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org, Gupta, Suraj, Katakam, Harini Cc: Andy Chiu, linux-kernel@vger.kernel.org, Simon Horman, Ariane Keller, Daniel Borkmann, linux-arm-kernel@lists.infradead.org, Simek, Michal > -----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 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow 2024-09-17 11:24 ` Pandey, Radhey Shyam @ 2024-09-17 14:40 ` Sean Anderson 0 siblings, 0 replies; 8+ messages in thread From: Sean Anderson @ 2024-09-17 14:40 UTC (permalink / raw) To: Pandey, Radhey Shyam, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org, Gupta, Suraj, Katakam, Harini Cc: Andy Chiu, linux-kernel@vger.kernel.org, Simon Horman, Ariane Keller, Daniel Borkmann, linux-arm-kernel@lists.infradead.org, Simek, Michal 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 >> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow 2024-09-11 7:01 ` Pandey, Radhey Shyam 2024-09-12 14:30 ` Sean Anderson @ 2024-09-13 16:51 ` Sean Anderson 1 sibling, 0 replies; 8+ messages in thread From: Sean Anderson @ 2024-09-13 16:51 UTC (permalink / raw) To: Pandey, Radhey Shyam, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org, Gupta, Suraj, Katakam, Harini Cc: Andy Chiu, linux-kernel@vger.kernel.org, Simon Horman, Ariane Keller, Daniel Borkmann, linux-arm-kernel@lists.infradead.org, Simek, Michal 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-09-17 14:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-09 23:09 [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow Sean Anderson 2024-09-10 0:51 ` Nelson, Shannon 2024-09-11 7:01 ` Pandey, Radhey Shyam 2024-09-12 14:30 ` Sean Anderson 2024-09-12 14:35 ` Pandey, Radhey Shyam 2024-09-17 11:24 ` Pandey, Radhey Shyam 2024-09-17 14:40 ` Sean Anderson 2024-09-13 16:51 ` Sean Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).