[PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Suraj Gupta]

In DMAengine flow, AXI DMA driver invokes callback before freeing BD in
irq handling path.
In Rx callback (axienet_dma_rx_cb()), axienet driver tries to allocate
new BD after processing skb.
This will be problematic if both AXI-DMA and AXI ethernet have same
BD count as all Rx BDs will be allocated initially and it won't be
able to allocate new one after Rx irq. Incrementing head pointer w/o
checking for BD allocation will result in garbage values in skb BD and
cause the below kernel crash:

Unable to handle kernel paging request at virtual address fffffffffffffffa
<snip>
Internal error: Oops: 0000000096000006 [#1]  SMP
pc : axienet_dma_rx_cb+0x78/0x150
lr : axienet_dma_rx_cb+0x78/0x150
 Call trace:
  axienet_dma_rx_cb+0x78/0x150 (P)
  xilinx_dma_do_tasklet+0xdc/0x290
  tasklet_action_common+0x12c/0x178
  tasklet_action+0x30/0x3c
  handle_softirqs+0xf8/0x230
<snip>

Fixes: 6a91b846af85 ("net: axienet: Introduce dmaengine support")
Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 6011d7eae0c7..acd5be60afec 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1457,7 +1457,6 @@ static void axienet_rx_submit_desc(struct net_device *ndev)
 	if (!skbuf_dma)
 		return;
 
-	lp->rx_ring_head++;
 	skb = netdev_alloc_skb(ndev, lp->max_frm_size);
 	if (!skb)
 		return;
@@ -1482,6 +1481,7 @@ static void axienet_rx_submit_desc(struct net_device *ndev)
 	skbuf_dma->desc = dma_rx_desc;
 	dma_rx_desc->callback_param = lp;
 	dma_rx_desc->callback_result = axienet_dma_rx_cb;
+	lp->rx_ring_head++;
 	dmaengine_submit(dma_rx_desc);
 
 	return;
-- 
2.25.1

Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Sean Anderson]

On 8/5/25 15:19, Suraj Gupta wrote:
> In DMAengine flow, AXI DMA driver invokes callback before freeing BD in
> irq handling path.
> In Rx callback (axienet_dma_rx_cb()), axienet driver tries to allocate
> new BD after processing skb.
> This will be problematic if both AXI-DMA and AXI ethernet have same
> BD count as all Rx BDs will be allocated initially and it won't be
> able to allocate new one after Rx irq. Incrementing head pointer w/o
> checking for BD allocation will result in garbage values in skb BD and
> cause the below kernel crash:
> 
> Unable to handle kernel paging request at virtual address fffffffffffffffa
> <snip>
> Internal error: Oops: 0000000096000006 [#1]  SMP
> pc : axienet_dma_rx_cb+0x78/0x150
> lr : axienet_dma_rx_cb+0x78/0x150
>  Call trace:
>   axienet_dma_rx_cb+0x78/0x150 (P)
>   xilinx_dma_do_tasklet+0xdc/0x290
>   tasklet_action_common+0x12c/0x178
>   tasklet_action+0x30/0x3c
>   handle_softirqs+0xf8/0x230
> <snip>
> 
> Fixes: 6a91b846af85 ("net: axienet: Introduce dmaengine support")
> Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 6011d7eae0c7..acd5be60afec 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1457,7 +1457,6 @@ static void axienet_rx_submit_desc(struct net_device *ndev)
>  	if (!skbuf_dma)
>  		return;
>  
> -	lp->rx_ring_head++;
>  	skb = netdev_alloc_skb(ndev, lp->max_frm_size);
>  	if (!skb)
>  		return;
> @@ -1482,6 +1481,7 @@ static void axienet_rx_submit_desc(struct net_device *ndev)
>  	skbuf_dma->desc = dma_rx_desc;
>  	dma_rx_desc->callback_param = lp;
>  	dma_rx_desc->callback_result = axienet_dma_rx_cb;
> +	lp->rx_ring_head++;
>  	dmaengine_submit(dma_rx_desc);
>  
>  	return;

Reviewed-by: Sean Anderson <sean.anderson@linux.dev>

RE: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Pandey, Radhey Shyam]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Suraj Gupta <suraj.gupta2@amd.com>
> Sent: Wednesday, August 6, 2025 12:50 AM
> To: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; Simek, Michal <michal.simek@amd.com>;
> sean.anderson@linux.dev; Pandey, Radhey Shyam
> <radhey.shyam.pandey@amd.com>; horms@kernel.org
> Cc: netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Katakam, Harini <harini.katakam@amd.com>
> Subject: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after
> BD is successfully allocated in dmaengine flow
>
> In DMAengine flow, AXI DMA driver invokes callback before freeing BD in
> irq handling path.
> In Rx callback (axienet_dma_rx_cb()), axienet driver tries to allocate
> new BD after processing skb.
> This will be problematic if both AXI-DMA and AXI ethernet have same
> BD count as all Rx BDs will be allocated initially and it won't be
> able to allocate new one after Rx irq. Incrementing head pointer w/o
> checking for BD allocation will result in garbage values in skb BD and
> cause the below kernel crash:
>
> Unable to handle kernel paging request at virtual address fffffffffffffffa
> <snip>
> Internal error: Oops: 0000000096000006 [#1]  SMP
> pc : axienet_dma_rx_cb+0x78/0x150
> lr : axienet_dma_rx_cb+0x78/0x150
>  Call trace:
>   axienet_dma_rx_cb+0x78/0x150 (P)
>   xilinx_dma_do_tasklet+0xdc/0x290
>   tasklet_action_common+0x12c/0x178
>   tasklet_action+0x30/0x3c
>   handle_softirqs+0xf8/0x230
> <snip>
>
> Fixes: 6a91b846af85 ("net: axienet: Introduce dmaengine support")
> Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com>

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>

> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 6011d7eae0c7..acd5be60afec 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1457,7 +1457,6 @@ static void axienet_rx_submit_desc(struct net_device
> *ndev)
>       if (!skbuf_dma)
>               return;
>
> -     lp->rx_ring_head++;
>       skb = netdev_alloc_skb(ndev, lp->max_frm_size);
>       if (!skb)
>               return;
> @@ -1482,6 +1481,7 @@ static void axienet_rx_submit_desc(struct net_device
> *ndev)
>       skbuf_dma->desc = dma_rx_desc;
>       dma_rx_desc->callback_param = lp;
>       dma_rx_desc->callback_result = axienet_dma_rx_cb;
> +     lp->rx_ring_head++;
>       dmaengine_submit(dma_rx_desc);
>
>       return;
> --
> 2.25.1

Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Jakub Kicinski]

On Wed, 6 Aug 2025 00:49:58 +0530 Suraj Gupta wrote:
> In DMAengine flow, AXI DMA driver invokes callback before freeing BD in
> irq handling path.
> In Rx callback (axienet_dma_rx_cb()), axienet driver tries to allocate
> new BD after processing skb.
> This will be problematic if both AXI-DMA and AXI ethernet have same
> BD count as all Rx BDs will be allocated initially and it won't be
> able to allocate new one after Rx irq. Incrementing head pointer w/o
> checking for BD allocation will result in garbage values in skb BD and
> cause the below kernel crash:
> 
> Unable to handle kernel paging request at virtual address fffffffffffffffa
> <snip>
> Internal error: Oops: 0000000096000006 [#1]  SMP
> pc : axienet_dma_rx_cb+0x78/0x150
> lr : axienet_dma_rx_cb+0x78/0x150
>  Call trace:
>   axienet_dma_rx_cb+0x78/0x150 (P)
>   xilinx_dma_do_tasklet+0xdc/0x290
>   tasklet_action_common+0x12c/0x178
>   tasklet_action+0x30/0x3c
>   handle_softirqs+0xf8/0x230
> <snip>

Do you mean that we're incrementing lp->rx_ring_head before we know
that the submission will succeed? Potentially leaving an uninitialized
entry (say at index n), next attempt will try to use the next entry 
(n + 1) but the completion will not know about the skip so it will
try to complete entry n ?

This is really not coming thru in your explanation.

The fix itself seems incomplete. Even if we correctly skip the increment
we will never try to catch up with the allocations, the ring will have
fewer outstanding Rx skbs until reset, right? Worst case we drop all
the skbs and the ring will be empty, no Rx will happen until reset.
The shutdown path seems to be checking for skb = NULL so I guess it's
correct but good to double check..
-- 
pw-bot: cr

RE: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Gupta, Suraj]

[Public]

Hi Jakub,
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Saturday, August 9, 2025 12:36 AM
> To: Gupta, Suraj <Suraj.Gupta2@amd.com>
> Cc: andrew+netdev@lunn.ch; davem@davemloft.net;
> edumazet@google.com; pabeni@redhat.com; Simek, Michal
> <michal.simek@amd.com>; sean.anderson@linux.dev; Pandey, Radhey
> Shyam <radhey.shyam.pandey@amd.com>; horms@kernel.org;
> netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Katakam, Harini <harini.katakam@amd.com>
> Subject: Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head
> pointer after BD is successfully allocated in dmaengine flow
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On Wed, 6 Aug 2025 00:49:58 +0530 Suraj Gupta wrote:
> > In DMAengine flow, AXI DMA driver invokes callback before freeing BD
> > in irq handling path.
> > In Rx callback (axienet_dma_rx_cb()), axienet driver tries to allocate
> > new BD after processing skb.
> > This will be problematic if both AXI-DMA and AXI ethernet have same BD
> > count as all Rx BDs will be allocated initially and it won't be able
> > to allocate new one after Rx irq. Incrementing head pointer w/o
> > checking for BD allocation will result in garbage values in skb BD and
> > cause the below kernel crash:
> >
> > Unable to handle kernel paging request at virtual address
> > fffffffffffffffa <snip> Internal error: Oops: 0000000096000006 [#1]
> > SMP pc : axienet_dma_rx_cb+0x78/0x150 lr :
> > axienet_dma_rx_cb+0x78/0x150  Call trace:
> >   axienet_dma_rx_cb+0x78/0x150 (P)
> >   xilinx_dma_do_tasklet+0xdc/0x290
> >   tasklet_action_common+0x12c/0x178
> >   tasklet_action+0x30/0x3c
> >   handle_softirqs+0xf8/0x230
> > <snip>
>
> Do you mean that we're incrementing lp->rx_ring_head before we know that
> the submission will succeed? Potentially leaving an uninitialized entry (say at
> index n), next attempt will try to use the next entry (n + 1) but the completion
> will not know about the skip so it will try to complete entry n ?
>
> This is really not coming thru in your explanation.
>
You're right, I only explained the issue I faced while running perf test with same BD count in axienet and AXI DMA. Above is more generic explanation of the situation here. I'll modify the description.

> The fix itself seems incomplete. Even if we correctly skip the increment we
> will never try to catch up with the allocations, the ring will have fewer
> outstanding Rx skbs until reset, right? Worst case we drop all the skbs and the
> ring will be empty, no Rx will happen until reset.
> The shutdown path seems to be checking for skb = NULL so I guess it's correct
> but good to double check..
> --
> pw-bot: cr

I agree that Rx ring will have fewer outstanding skbs. But I think that difference won't exceed one anytime as descriptors submission will fail only once due to insufficient space in AXIDMA BD ring. Rest of the time we already will have an extra entry in AXIDMA BD ring. Also, invoking callback (where Rx skb ring hp is filled in axienet)and freeing AXIDMA BD are part of same tasklet in AXIDMA driver so next callback will only be called after freeing a BD.
I tested running stress tests (Both UPD and TCP netperf).
Please let me know your thoughts if I'm missing something.

Thanks,
Suraj

Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Jakub Kicinski]

On Sat, 9 Aug 2025 20:31:40 +0000 Gupta, Suraj wrote:
> > The fix itself seems incomplete. Even if we correctly skip the increment we
> > will never try to catch up with the allocations, the ring will have fewer
> > outstanding Rx skbs until reset, right? Worst case we drop all the skbs and the
> > ring will be empty, no Rx will happen until reset.
> > The shutdown path seems to be checking for skb = NULL so I guess it's correct
> > but good to double check..
> 
> I agree that Rx ring will have fewer outstanding skbs. But I think
> that difference won't exceed one anytime as descriptors submission
> will fail only once due to insufficient space in AXIDMA BD ring. Rest
> of the time we already will have an extra entry in AXIDMA BD ring.
> Also, invoking callback (where Rx skb ring hp is filled in
> axienet)and freeing AXIDMA BD are part of same tasklet in AXIDMA
> driver so next callback will only be called after freeing a BD. I
> tested running stress tests (Both UPD and TCP netperf). Please let me
> know your thoughts if I'm missing something.

That wasn't my reading, maybe I misinterpreted the code.

From what I could tell the driver tries to give one new buffer for each
buffer completed. So it never tries to "catch up" on previously missed
allocations. IOW say we have a queue with 16 indexes, after 16 failures
(which may be spread out over time) the ring will be empty.

RE: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Pandey, Radhey Shyam]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, August 11, 2025 9:08 PM
> To: Gupta, Suraj <Suraj.Gupta2@amd.com>
> Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> pabeni@redhat.com; Simek, Michal <michal.simek@amd.com>;
> sean.anderson@linux.dev; Pandey, Radhey Shyam
> <radhey.shyam.pandey@amd.com>; horms@kernel.org; netdev@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Katakam, Harini
> <harini.katakam@amd.com>
> Subject: Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer
> after BD is successfully allocated in dmaengine flow
>
> On Sat, 9 Aug 2025 20:31:40 +0000 Gupta, Suraj wrote:
> > > The fix itself seems incomplete. Even if we correctly skip the
> > > increment we will never try to catch up with the allocations, the
> > > ring will have fewer outstanding Rx skbs until reset, right? Worst
> > > case we drop all the skbs and the ring will be empty, no Rx will happen until
> reset.
> > > The shutdown path seems to be checking for skb = NULL so I guess
> > > it's correct but good to double check..
> >
> > I agree that Rx ring will have fewer outstanding skbs. But I think
> > that difference won't exceed one anytime as descriptors submission
> > will fail only once due to insufficient space in AXIDMA BD ring. Rest
> > of the time we already will have an extra entry in AXIDMA BD ring.
> > Also, invoking callback (where Rx skb ring hp is filled in axienet)and
> > freeing AXIDMA BD are part of same tasklet in AXIDMA driver so next
> > callback will only be called after freeing a BD. I tested running
> > stress tests (Both UPD and TCP netperf). Please let me know your
> > thoughts if I'm missing something.
>
> That wasn't my reading, maybe I misinterpreted the code.
>
> From what I could tell the driver tries to give one new buffer for each buffer
> completed. So it never tries to "catch up" on previously missed allocations. IOW say
> we have a queue with 16 indexes, after 16 failures (which may be spread out over
> time) the ring will be empty.

Yes, IIRC there is 1:1 mapping for RX DMA callback and
axienet_rx_submit_desc(). In case there are failure in
axienet_rx_submit_desc() it is not able to reattempt
in current implementation. Theoretically there could
be other error in rx_submit_desc() (like dma_mapping/netdev
allocation)

One thought is to have some flag/index to tell that it should
be reattempted in subsequent axienet_rx_submit_desc() ?

Thanks,
Radhey

Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Jakub Kicinski]

On Mon, 11 Aug 2025 15:55:02 +0000 Pandey, Radhey Shyam wrote:
> > That wasn't my reading, maybe I misinterpreted the code.
> >
> > From what I could tell the driver tries to give one new buffer for each buffer
> > completed. So it never tries to "catch up" on previously missed allocations. IOW say
> > we have a queue with 16 indexes, after 16 failures (which may be spread out over
> > time) the ring will be empty.  
> 
> Yes, IIRC there is 1:1 mapping for RX DMA callback and
> axienet_rx_submit_desc(). In case there are failure in
> axienet_rx_submit_desc() it is not able to reattempt
> in current implementation. Theoretically there could
> be other error in rx_submit_desc() (like dma_mapping/netdev
> allocation)
> 
> One thought is to have some flag/index to tell that it should
> be reattempted in subsequent axienet_rx_submit_desc() ?

Yes, some kind of counter of buffer that need to be allocated.
The other problem to solve is when the buffers are completely
depleted there will be no callback so no opportunity to refill.
For drivers which refill from NAPI this is usually solved by
periodically scheduling NAPI.

RE: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

[Gupta, Suraj]

[Public]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, August 11, 2025 10:13 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: Gupta, Suraj <Suraj.Gupta2@amd.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; Simek,
> Michal <michal.simek@amd.com>; sean.anderson@linux.dev;
> horms@kernel.org; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Katakam, Harini
> <harini.katakam@amd.com>
> Subject: Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head
> pointer after BD is successfully allocated in dmaengine flow
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On Mon, 11 Aug 2025 15:55:02 +0000 Pandey, Radhey Shyam wrote:
> > > That wasn't my reading, maybe I misinterpreted the code.
> > >
> > > From what I could tell the driver tries to give one new buffer for
> > > each buffer completed. So it never tries to "catch up" on previously
> > > missed allocations. IOW say we have a queue with 16 indexes, after
> > > 16 failures (which may be spread out over
> > > time) the ring will be empty.
> >
> > Yes, IIRC there is 1:1 mapping for RX DMA callback and
> > axienet_rx_submit_desc(). In case there are failure in
> > axienet_rx_submit_desc() it is not able to reattempt in current
> > implementation. Theoretically there could be other error in
> > rx_submit_desc() (like dma_mapping/netdev
> > allocation)
> >
> > One thought is to have some flag/index to tell that it should be
> > reattempted in subsequent axienet_rx_submit_desc() ?
>
> Yes, some kind of counter of buffer that need to be allocated.
> The other problem to solve is when the buffers are completely depleted there
> will be no callback so no opportunity to refill.
> For drivers which refill from NAPI this is usually solved by periodically
> scheduling NAPI.
I think even after this same problem will exist in dmaengine flow because main problem here is axidma descriptor is freed after invoking callback. Callback (axienet_rx_cb()) will try to request new descriptor in rx_submit_desc(), but it won't be free till that time.
One possible solution I can think of is together with NAPI, having some DMAengine interface invoked by AXIDMA (similar to callback, say post_cb()) after freeing descriptor, which requests new descriptor from AXIDMA?