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

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.

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

+               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.

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.

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.

quoted

+}
+
 #endif /* _LINUX_NETDEVICE_H */

diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
index 797247a34cb7..93462e5b2207 100644
--- a/io_uring/zcrx.c
+++ b/io_uring/zcrx.c

@@ -584,7 +584,7 @@ int io_register_zcrx_ifq(struct io_ring_ctx *ctx,
                goto err;
        }

-       ifq->dev = ifq->netdev->dev.parent;
+       ifq->dev = netdev_get_dma_dev(ifq->netdev);

nit: this hunk will not apply when backporting this to trees that only
have the Fixes commit... which makes it more weird that this is
considered a fix for that, but I'm fine either way.

Ouch, indeed. Should have thought of that.

Big picture view:
Maybe after all it is more generic to have queue ops that
can provide this info. For zcrx it is trivial (see below hunk).

For devmem I was thinking of calling netdev_queue_get_dma_dev()
for every bound queue (before the mapping) and return an error
only when we find different . It will make netdev_nl_bind_rx_doit()
a bit icky, but the idea is not complicated. What do you think?

---

diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
index 6e835972abd1..04c69f39558d 100644
--- a/include/net/netdev_queues.h
+++ b/include/net/netdev_queues.h

@@ -127,6 +127,9 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  * @ndo_queue_stop:    Stop the RX queue at the specified index. The stopped
  *                     queue's memory is written at the specified address.
  *
+ * @ndo_queue_get_dma_dev: When set, the driver can provide the DMA device to
+ *                        be used for the given queue.
+ *
  * Note that @ndo_queue_mem_alloc and @ndo_queue_mem_free may be called while
  * the interface is closed. @ndo_queue_start and @ndo_queue_stop will only
  * be called for an interface which is open.

@@ -144,6 +147,8 @@ struct netdev_queue_mgmt_ops {
        int                     (*ndo_queue_stop)(struct net_device *dev,
                                                  void *per_queue_mem,
                                                  int idx);
+       struct device *         (*ndo_queue_get_dma_dev)(const struct net_device *dev,
+                                                        int idx);
 };
 
 /**

@@ -321,4 +326,15 @@ static inline void netif_subqueue_sent(const struct net_device *dev,
                                         get_desc, start_thrs);         \
        })
 
+static inline struct device *netdev_queue_get_dma_dev(const struct net_device *dev,
+                                                     int idx)
+{
+       const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
+
+       if (qops && qops->ndo_queue_get_dma_dev)
+               return qops->ndo_queue_get_dma_dev(dev, idx);
+
+       return netdev_get_dma_dev(dev);
+}
+
 #endif

diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
index 93462e5b2207..478693a6d325 100644
--- a/io_uring/zcrx.c
+++ b/io_uring/zcrx.c

@@ -12,6 +12,7 @@
 #include <net/page_pool/helpers.h>
 #include <net/page_pool/memory_provider.h>
 #include <net/netlink.h>
+#include <net/netdev_queues.h>
 #include <net/netdev_rx_queue.h>
 #include <net/tcp.h>
 #include <net/rps.h>

@@ -584,7 +585,7 @@ int io_register_zcrx_ifq(struct io_ring_ctx *ctx,
                goto err;
        }
 
-       ifq->dev = netdev_get_dma_dev(ifq->netdev);
+       ifq->dev = netdev_queue_get_dma_dev(ifq->netdev, reg.if_rxq);
        if (!ifq->dev) {
                ret = -EOPNOTSUPP;
                goto err;
---

Thanks,
Dragos