Multi-PF netdevs have queues belonging to different PFs which also means
different DMA devices. This means that the binding on the DMA buffer can
be done to the incorrect device.
This change allows devmem binding to multiple queues only when the
queues have the same DMA device. Otherwise an error is returned.
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
---
net/core/netdev-genl.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 0df9c159e515..a8c27f636453 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -906,6 +906,33 @@ static int netdev_nl_read_rxq_bitmap(struct genl_info *info,
return 0;
}
+static struct device *netdev_nl_get_dma_dev(struct net_device *netdev,
+ unsigned long *rxq_bitmap,
+ struct netlink_ext_ack *extack)
+{
+ struct device *dma_dev = NULL;
+ u32 rxq_idx, prev_rxq_idx;
+
+ for_each_set_bit(rxq_idx, rxq_bitmap, netdev->real_num_rx_queues) {
+ struct device *rxq_dma_dev;
+
+ rxq_dma_dev = netdev_queue_get_dma_dev(netdev, rxq_idx);
+ /* Multi-PF netdev queues can belong to different DMA devoces.
+ * Block this case.
+ */
+ if (dma_dev && rxq_dma_dev != dma_dev) {
+ NL_SET_ERR_MSG_FMT(extack, "Queue %u has a different dma device than queue %u",
+ rxq_idx, prev_rxq_idx);
+ return ERR_PTR(-EOPNOTSUPP);
+ }
+
+ dma_dev = rxq_dma_dev;
+ prev_rxq_idx = rxq_idx;
+ }
+
+ return dma_dev;
+}
+
int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
{
struct net_devmem_dmabuf_binding *binding;
@@ -969,7 +996,12 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
if (err)
goto err_rxq_bitmap;
- dma_dev = netdev_queue_get_dma_dev(netdev, 0);
+ dma_dev = netdev_nl_get_dma_dev(netdev, rxq_bitmap, info->extack);
+ if (IS_ERR(dma_dev)) {
+ err = PTR_ERR(dma_dev);
+ goto err_rxq_bitmap;
+ }
+
binding = net_devmem_bind_dmabuf(netdev, dma_dev, DMA_FROM_DEVICE,
dmabuf_fd, priv, info->extack);
if (IS_ERR(binding)) {
--
2.50.1On Wed, 20 Aug 2025 20:11:58 +0300 Dragos Tatulea wrote:
> +static struct device *netdev_nl_get_dma_dev(struct net_device *netdev,
> + unsigned long *rxq_bitmap,
> + struct netlink_ext_ack *extack)
break after type if it's long and multi line:
static struct device *
netdev_nl_get_dma_dev(struct net_device *netdev, unsigned long *rxq_bitmap,
struct netlink_ext_ack *extack)
> +{
> + struct device *dma_dev = NULL;
> + u32 rxq_idx, prev_rxq_idx;
> +
> + for_each_set_bit(rxq_idx, rxq_bitmap, netdev->real_num_rx_queues) {
> + struct device *rxq_dma_dev;
> +
> + rxq_dma_dev = netdev_queue_get_dma_dev(netdev, rxq_idx);
> + /* Multi-PF netdev queues can belong to different DMA devoces.
typo: devoces
> + * Block this case.
> + */
> + if (dma_dev && rxq_dma_dev != dma_dev) {
> + NL_SET_ERR_MSG_FMT(extack, "Queue %u has a different dma device than queue %u",
s/dma/DMA/
I think we may want to bubble up the Multi-PF thing from the comment to
the user. This could be quite confusing to people. How about:
"DMA device mismatch between queue %u and %u (multi-PF device?)"
On Wed, Aug 20, 2025 at 06:16:09PM -0700, Jakub Kicinski wrote:
> On Wed, 20 Aug 2025 20:11:58 +0300 Dragos Tatulea wrote:
> > +static struct device *netdev_nl_get_dma_dev(struct net_device *netdev,
> > + unsigned long *rxq_bitmap,
> > + struct netlink_ext_ack *extack)
>
> break after type if it's long and multi line:
>
> static struct device *
> netdev_nl_get_dma_dev(struct net_device *netdev, unsigned long *rxq_bitmap,
> struct netlink_ext_ack *extack)
>
Will fix. Hope to remember for next times as well.
> > +{
> > + struct device *dma_dev = NULL;
> > + u32 rxq_idx, prev_rxq_idx;
> > +
> > + for_each_set_bit(rxq_idx, rxq_bitmap, netdev->real_num_rx_queues) {
> > + struct device *rxq_dma_dev;
> > +
> > + rxq_dma_dev = netdev_queue_get_dma_dev(netdev, rxq_idx);
> > + /* Multi-PF netdev queues can belong to different DMA devoces.
>
> typo: devoces
>
Thanks!
> > + * Block this case.
> > + */
> > + if (dma_dev && rxq_dma_dev != dma_dev) {
> > + NL_SET_ERR_MSG_FMT(extack, "Queue %u has a different dma device than queue %u",
>
> s/dma/DMA/
> I think we may want to bubble up the Multi-PF thing from the comment to
> the user. This could be quite confusing to people. How about:
>
> "DMA device mismatch between queue %u and %u (multi-PF device?)"
Sounds good. Do we still need the comment? A similar remark is done in
the commit message as well.
Thanks,
Dragos
On Thu, 21 Aug 2025 11:10:57 +0000 Dragos Tatulea wrote: > > I think we may want to bubble up the Multi-PF thing from the comment to > > the user. This could be quite confusing to people. How about: > > > > "DMA device mismatch between queue %u and %u (multi-PF device?)" > Sounds good. Do we still need the comment? A similar remark is done in > the commit message as well. I think the comment can go away with the better user-facing message. The commit msg LGTM as is, doesn't count as extra LoC..
On Wed, Aug 20, 2025 at 10:14 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> Multi-PF netdevs have queues belonging to different PFs which also means
> different DMA devices. This means that the binding on the DMA buffer can
> be done to the incorrect device.
>
> This change allows devmem binding to multiple queues only when the
> queues have the same DMA device. Otherwise an error is returned.
>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> ---
> net/core/netdev-genl.c | 34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 0df9c159e515..a8c27f636453 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -906,6 +906,33 @@ static int netdev_nl_read_rxq_bitmap(struct genl_info *info,
> return 0;
> }
>
> +static struct device *netdev_nl_get_dma_dev(struct net_device *netdev,
> + unsigned long *rxq_bitmap,
> + struct netlink_ext_ack *extack)
> +{
> + struct device *dma_dev = NULL;
> + u32 rxq_idx, prev_rxq_idx;
> +
> + for_each_set_bit(rxq_idx, rxq_bitmap, netdev->real_num_rx_queues) {
> + struct device *rxq_dma_dev;
> +
> + rxq_dma_dev = netdev_queue_get_dma_dev(netdev, rxq_idx);
> + /* Multi-PF netdev queues can belong to different DMA devoces.
> + * Block this case.
> + */
> + if (dma_dev && rxq_dma_dev != dma_dev) {
> + NL_SET_ERR_MSG_FMT(extack, "Queue %u has a different dma device than queue %u",
> + rxq_idx, prev_rxq_idx);
> + return ERR_PTR(-EOPNOTSUPP);
> + }
> +
> + dma_dev = rxq_dma_dev;
> + prev_rxq_idx = rxq_idx;
> + }
> +
> + return dma_dev;
> +}
> +
> int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> {
> struct net_devmem_dmabuf_binding *binding;
> @@ -969,7 +996,12 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> if (err)
> goto err_rxq_bitmap;
>
> - dma_dev = netdev_queue_get_dma_dev(netdev, 0);
> + dma_dev = netdev_nl_get_dma_dev(netdev, rxq_bitmap, info->extack);
> + if (IS_ERR(dma_dev)) {
Does this need to be IS_ERR_OR_NULL? AFAICT if all the ndos return
NULL, then dma_dev will also be NULL, and NULL is not a valid value to
pass to bind_dmabuf below.
--
Thanks,
Mina
On Wed, Aug 20, 2025 at 03:57:32PM -0700, Mina Almasry wrote:
> On Wed, Aug 20, 2025 at 10:14 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >
> > Multi-PF netdevs have queues belonging to different PFs which also means
> > different DMA devices. This means that the binding on the DMA buffer can
> > be done to the incorrect device.
> >
> > This change allows devmem binding to multiple queues only when the
> > queues have the same DMA device. Otherwise an error is returned.
> >
> > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > ---
> > net/core/netdev-genl.c | 34 +++++++++++++++++++++++++++++++++-
> > 1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > index 0df9c159e515..a8c27f636453 100644
> > --- a/net/core/netdev-genl.c
> > +++ b/net/core/netdev-genl.c
> > @@ -906,6 +906,33 @@ static int netdev_nl_read_rxq_bitmap(struct genl_info *info,
> > return 0;
> > }
> >
> > +static struct device *netdev_nl_get_dma_dev(struct net_device *netdev,
> > + unsigned long *rxq_bitmap,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct device *dma_dev = NULL;
> > + u32 rxq_idx, prev_rxq_idx;
> > +
> > + for_each_set_bit(rxq_idx, rxq_bitmap, netdev->real_num_rx_queues) {
> > + struct device *rxq_dma_dev;
> > +
> > + rxq_dma_dev = netdev_queue_get_dma_dev(netdev, rxq_idx);
> > + /* Multi-PF netdev queues can belong to different DMA devoces.
> > + * Block this case.
> > + */
> > + if (dma_dev && rxq_dma_dev != dma_dev) {
> > + NL_SET_ERR_MSG_FMT(extack, "Queue %u has a different dma device than queue %u",
> > + rxq_idx, prev_rxq_idx);
> > + return ERR_PTR(-EOPNOTSUPP);
> > + }
> > +
> > + dma_dev = rxq_dma_dev;
> > + prev_rxq_idx = rxq_idx;
> > + }
> > +
> > + return dma_dev;
> > +}
> > +
> > int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > {
> > struct net_devmem_dmabuf_binding *binding;
> > @@ -969,7 +996,12 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > if (err)
> > goto err_rxq_bitmap;
> >
> > - dma_dev = netdev_queue_get_dma_dev(netdev, 0);
> > + dma_dev = netdev_nl_get_dma_dev(netdev, rxq_bitmap, info->extack);
> > + if (IS_ERR(dma_dev)) {
>
> Does this need to be IS_ERR_OR_NULL? AFAICT if all the ndos return
> NULL, then dma_dev will also be NULL, and NULL is not a valid value to
> pass to bind_dmabuf below.
>
netdev_nl_get_dma_dev() signals DMA device mismatch though EOPNOTSUPP.
That's why it is IS_ERR_OR_NULL. I find that doing this at the calling
site would be possible but less accurate.
Or did you mean something else?
Thannks,
Dragos
© 2016 - 2025 Red Hat, Inc.