* Re: [PATCH net] ethernet: ionic: Fix DMA mapping test in `ionic_xdp_post_frame()`
2025-06-17 9:18 [PATCH net] ethernet: ionic: Fix DMA mapping test in `ionic_xdp_post_frame()` Thomas Fourier
@ 2025-06-17 14:35 ` Michal Swiatkowski
2025-06-17 15:01 ` Alexander Lobakin
2025-06-17 16:56 ` Brett Creeley
2 siblings, 0 replies; 5+ messages in thread
From: Michal Swiatkowski @ 2025-06-17 14:35 UTC (permalink / raw)
To: Thomas Fourier
Cc: Shannon Nelson, Brett Creeley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Vladimir Oltean, Caleb Sander Mateos,
Taehee Yoo, netdev, linux-kernel, bpf
On Tue, Jun 17, 2025 at 11:18:36AM +0200, Thomas Fourier wrote:
> The `ionic_tx_map_frag()` wrapper function is used which returns 0 or a
> valid DMA address. Testing that pointer with `dma_mapping_error()`could
> be eroneous since the error value exptected by `dma_mapping_error()` is
> not 0 but `DMA_MAPPING_ERROR` which is often ~0.
>
> Fixes: ac8813c0ab7d ("ionic: convert Rx queue buffers to use page_pool")
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> ---
> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 2ac59564ded1..beefdc43013e 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -357,7 +357,7 @@ static int ionic_xdp_post_frame(struct ionic_queue *q, struct xdp_frame *frame,
> } else {
> dma_addr = ionic_tx_map_frag(q, frag, 0,
> skb_frag_size(frag));
> - if (dma_mapping_error(q->dev, dma_addr)) {
> + if (!dma_addr) {
> ionic_tx_desc_unmap_bufs(q, desc_info);
> return -EIO;
Right, in ionic_tx_map_skb() it is used correctly (like here now).
Thanks for fixing.
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> }
> --
> 2.43.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] ethernet: ionic: Fix DMA mapping test in `ionic_xdp_post_frame()`
2025-06-17 9:18 [PATCH net] ethernet: ionic: Fix DMA mapping test in `ionic_xdp_post_frame()` Thomas Fourier
2025-06-17 14:35 ` Michal Swiatkowski
@ 2025-06-17 15:01 ` Alexander Lobakin
2025-06-17 16:56 ` Brett Creeley
2 siblings, 0 replies; 5+ messages in thread
From: Alexander Lobakin @ 2025-06-17 15:01 UTC (permalink / raw)
To: Thomas Fourier
Cc: Shannon Nelson, Brett Creeley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Vladimir Oltean, Caleb Sander Mateos,
Taehee Yoo, netdev, linux-kernel, bpf
From: Thomas Fourier <fourier.thomas@gmail.com>
Date: Tue, 17 Jun 2025 11:18:36 +0200
> The `ionic_tx_map_frag()` wrapper function is used which returns 0 or a
> valid DMA address. Testing that pointer with `dma_mapping_error()`could
> be eroneous since the error value exptected by `dma_mapping_error()` is
> not 0 but `DMA_MAPPING_ERROR` which is often ~0.
BTW I remember clearly that Chris wrote that dma_addr == 0 is a
perfectly valid DMA address, that's why ~0 was picked for error checking.
So maybe it's better to fix the driver. I realize that in the real
world, it's almost impossible to get dma_addr == 0 from dma_map_*(), but
still.
>
> Fixes: ac8813c0ab7d ("ionic: convert Rx queue buffers to use page_pool")
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
Thanks,
Olek
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] ethernet: ionic: Fix DMA mapping test in `ionic_xdp_post_frame()`
2025-06-17 9:18 [PATCH net] ethernet: ionic: Fix DMA mapping test in `ionic_xdp_post_frame()` Thomas Fourier
2025-06-17 14:35 ` Michal Swiatkowski
2025-06-17 15:01 ` Alexander Lobakin
@ 2025-06-17 16:56 ` Brett Creeley
2025-06-18 17:05 ` Alexander Lobakin
2 siblings, 1 reply; 5+ messages in thread
From: Brett Creeley @ 2025-06-17 16:56 UTC (permalink / raw)
To: Thomas Fourier
Cc: Shannon Nelson, Brett Creeley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Vladimir Oltean, Caleb Sander Mateos,
Taehee Yoo, netdev, linux-kernel, bpf
On 6/17/2025 2:18 AM, Thomas Fourier wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> The `ionic_tx_map_frag()` wrapper function is used which returns 0 or a
> valid DMA address. Testing that pointer with `dma_mapping_error()`could
> be eroneous since the error value exptected by `dma_mapping_error()` is
> not 0 but `DMA_MAPPING_ERROR` which is often ~0.
>
> Fixes: ac8813c0ab7d ("ionic: convert Rx queue buffers to use page_pool")
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> ---
> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 2ac59564ded1..beefdc43013e 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -357,7 +357,7 @@ static int ionic_xdp_post_frame(struct ionic_queue *q, struct xdp_frame *frame,
> } else {
> dma_addr = ionic_tx_map_frag(q, frag, 0,
> skb_frag_size(frag));
> - if (dma_mapping_error(q->dev, dma_addr)) {
> + if (!dma_addr) {
Thanks for the fix.
After looking at this and Olek's comment, I think it makes the most
sense to return DMA_MAPPING_ERROR from ionic_tx_map_frag() and
ionic_tx_map_single() instead of 0 on failures.
Then any callers would do the following check:
if (unlikely(dma_addr == DMA_MAPPING_ERROR))
/* failure path */
Another option is always returning dma_addr regardless of
success/failure from the ionic_tx_map* functions, but then I'd be
inclined to use dma_mapping_error() again in the caller. This approach
seems wrong to me, especially when CONFIG_DMA_API_DEBUG is enabled.
Thanks,
Brett
> ionic_tx_desc_unmap_bufs(q, desc_info);
> return -EIO;
> }
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] ethernet: ionic: Fix DMA mapping test in `ionic_xdp_post_frame()`
2025-06-17 16:56 ` Brett Creeley
@ 2025-06-18 17:05 ` Alexander Lobakin
0 siblings, 0 replies; 5+ messages in thread
From: Alexander Lobakin @ 2025-06-18 17:05 UTC (permalink / raw)
To: Brett Creeley, Thomas Fourier
Cc: Shannon Nelson, Brett Creeley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Vladimir Oltean, Caleb Sander Mateos,
Taehee Yoo, netdev, linux-kernel, bpf
From: Brett Creeley <bcreeley@amd.com>
Date: Tue, 17 Jun 2025 09:56:55 -0700
> On 6/17/2025 2:18 AM, Thomas Fourier wrote:
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> The `ionic_tx_map_frag()` wrapper function is used which returns 0 or a
>> valid DMA address. Testing that pointer with `dma_mapping_error()`could
>> be eroneous since the error value exptected by `dma_mapping_error()` is
>> not 0 but `DMA_MAPPING_ERROR` which is often ~0.
>>
>> Fixes: ac8813c0ab7d ("ionic: convert Rx queue buffers to use page_pool")
>> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
>> ---
>> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/
>> drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> index 2ac59564ded1..beefdc43013e 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> @@ -357,7 +357,7 @@ static int ionic_xdp_post_frame(struct ionic_queue
>> *q, struct xdp_frame *frame,
>> } else {
>> dma_addr = ionic_tx_map_frag(q, frag, 0,
>>
>> skb_frag_size(frag));
>> - if (dma_mapping_error(q->dev,
>> dma_addr)) {
>> + if (!dma_addr) {
>
> Thanks for the fix.
>
> After looking at this and Olek's comment, I think it makes the most
> sense to return DMA_MAPPING_ERROR from ionic_tx_map_frag() and
> ionic_tx_map_single() instead of 0 on failures.
>
> Then any callers would do the following check:
>
> if (unlikely(dma_addr == DMA_MAPPING_ERROR))
> /* failure path */
>
> Another option is always returning dma_addr regardless of success/
> failure from the ionic_tx_map* functions, but then I'd be inclined to
> use dma_mapping_error() again in the caller. This approach seems wrong
> to me, especially when CONFIG_DMA_API_DEBUG is enabled.
Yup, I'd agree that dma_mapping_error() should be used only for checking
return values of the generic DMA API functions and only once per one
mapping. Otherwise, it's fine to pass DMA_MAPPING_ERROR and test against
it directly (after dma_mapping_error() was called already).
>
> Thanks,
>
> Brett
Thanks,
Olek
^ permalink raw reply [flat|nested] 5+ messages in thread