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 <redacted>
---
drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Sean Anderson <sean.anderson@linux.dev> Date: 2025-08-05 19:33:01
On 8/5/25 15:19, Suraj Gupta wrote:
quoted hunk
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 <redacted>
---
drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[AMD Official Use Only - AMD Internal Distribution Only]
-----Original Message-----
From: Suraj Gupta <redacted>
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 [off-list ref];
sean.anderson@linux.dev; Pandey, Radhey Shyam
[off-list ref]; horms@kernel.org
Cc: netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; Katakam, Harini [off-list ref]
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 <redacted>
From: Jakub Kicinski <kuba@kernel.org> Date: 2025-08-08 19:05:35
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
-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Saturday, August 9, 2025 12:36 AM
To: Gupta, Suraj <redacted>
Cc: andrew+netdev@lunn.ch; davem@davemloft.net;
edumazet@google.com; pabeni@redhat.com; Simek, Michal
[off-list ref]; sean.anderson@linux.dev; Pandey, Radhey
Shyam [off-list ref]; horms@kernel.org;
netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; Katakam, Harini [off-list ref]
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:
quoted
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2025-08-11 15:37:39
On Sat, 9 Aug 2025 20:31:40 +0000 Gupta, Suraj wrote:
quoted
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.
[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 <redacted>
Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
pabeni@redhat.com; Simek, Michal [off-list ref];
sean.anderson@linux.dev; Pandey, Radhey Shyam
[off-list ref]; horms@kernel.org; netdev@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Katakam, Harini
[off-list ref]
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:
quoted
quoted
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.
quoted
quoted
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2025-08-11 16:43:08
On Mon, 11 Aug 2025 15:55:02 +0000 Pandey, Radhey Shyam wrote:
quoted
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.
-----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 <redacted>; andrew+netdev@lunn.ch;
davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; Simek,
Michal [off-list ref]; sean.anderson@linux.dev;
horms@kernel.org; netdev@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Katakam, Harini
[off-list ref]
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:
quoted
quoted
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?