On Wed, Jul 9, 2025 at 12:54 PM Dragos Tatulea [off-list ref] wrote:

On Wed, Jul 09, 2025 at 12:29:22PM -0700, Mina Almasry wrote:

quoted

On Wed, Jul 9, 2025 at 5:46 AM Dragos Tatulea [off-list ref] wrote:

quoted

For zerocopy (io_uring, devmem), there is an assumption that the
parent device can do DMA. However that is not always the case:
ScalableFunction devices have the DMA device in the grandparent.

This patch adds a helper for getting the DMA device for a netdev from
its parent or grandparent if necessary. The NULL case is handled in the
callers.

devmem and io_uring are updated accordingly to use this helper instead
of directly using the parent.

Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Fixes: 170aafe35cb9 ("netdev: support binding dma-buf to netdevice")

nit: This doesn't seem like a fix? The current code supports all
devices that are not SF well enough, right? And in the case of SF
devices, I expect net_devmem_bind_dmabuf() to fail gracefully as the
dma mapping of a device that doesn't support it, I think, would fail
gracefully. So to me this seems like an improvement rather than a bug
fix.

dma_buf_map_attachment_unlocked() will return a sg_table with 0 nents.
That is graceful. However this will result in page_pools that will
always be returning errors further down the line which is very confusing
regarding the motives that caused it.

I am also fine to not make it a fix btw. Especially since the mlx5
devmem code was just accepted.

If you submit another version I'd rather it be a non-fix, especially
since applying the io_uring hunk will be challenging when backporting
this patch, but I assume hunk can be dropped while backporting, so I'm
fine either way.

quoted

quoted

Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
Changes in v1:
- Upgraded from RFC status.
- Dropped driver specific bits for generic solution.
- Implemented single patch as a fix as requested in RFC.
- Handling of multi-PF netdevs will be handled in a subsequent patch
  series.

RFC: https://lore.kernel.org/all/20250702172433.1738947-2-dtatulea@nvidia.com/ (local)
---
 include/linux/netdevice.h | 14 ++++++++++++++
 io_uring/zcrx.c           |  2 +-
 net/core/devmem.c         | 10 +++++++++-
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5847c20994d3..1cbde7193c4d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -5560,4 +5560,18 @@ extern struct net_device *blackhole_netdev;
                atomic_long_add((VAL), &(DEV)->stats.__##FIELD)
 #define DEV_STATS_READ(DEV, FIELD) atomic_long_read(&(DEV)->stats.__##FIELD)

+static inline struct device *netdev_get_dma_dev(const struct net_device *dev)
+{
+       struct device *dma_dev = dev->dev.parent;
+
+       if (!dma_dev)
+               return NULL;
+
+       /* Some devices (e.g. SFs) have the dma device as a grandparent. */
+       if (!dma_dev->dma_mask)

I was able to confirm that !dev->dma_mask means "this device doesn't
support dma". Multiple existing places in the code seem to use this
check.

Ack. That was my understanding as well.

quoted

quoted

+               dma_dev = dma_dev->parent;
+
+       return (dma_dev && dma_dev->dma_mask) ? dma_dev : NULL;

This may be a noob question, but are we sure that !dma_dev->dma_mask
&& dma_dev->parent->dma_mask != NULL means that the parent is the
dma-device that we should use? I understand SF devices work that way
but it's not immediately obvious to me that this is generically true.

This is what I gathered from Parav's answer.

quoted

For example pavel came up with the case where for veth,
netdev->dev.parent == NULL , I wonder if there are weird devices in
the wild where netdev->dev.parent->dma_mask == NULL but that doesn't
necessarily mean that the grandparent is the dma-device that we should
use.

Yep.

quoted

I guess to keep my long question short: what makes you think this is
generically safe to do? Or is it not, but we think most devices behave
this way and we're going to handle more edge cases in follow up
patches?

It is just what we know so far about SFs. See end of mail.

I see. OK, even though this is 'just what we know so far', I'm still
in favor of this simple approach, but I would say it would be good to
communicate in the comments that this is a best-effort dma-device
finding and doesn't handle every case under the sun. Something like
(untested):

static inline struct device *netdev_get_dma_dev(const struct net_device *dev)
{
       struct device *parent = dev->dev.parent;

       if (!parent)
               return NULL;

       /* For most netdevs, the parent supports dma and is the correct
        * dma-device
        */
       if (parent->dma_mask)
               return parent;

       /* For SF devices, the parent doesn't support dma, but the grandparent
        * does, and is the correct dma-device to use (link to docs that explain
        * this if any).
        */
       if (parent->parent && parent->parent->dma_mask)
               return parent->parent;

       /* If neither the parent nor grandparent support dma, then we're not
        * sure what dma-device to use. Error out. Special handling for new
        * netdevs may need to be added in the future.
        */
       return NULL;
}

With some comments explaining the logic a bit, you can add:

Reviewed-by: Mina Almasry <redacted>

And let's see if Jakub likes this. If not, we can always do the future
proof approach with the queue API giving the driver the ability to
tell us what exactly is the dma-device to use (or whatever approach he
prefers).

-- 
Thanks,
Mina