Re: [PATCH net-next V2 10/13] net: devmem: add netdev_has_dmabuf_binding() helper

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The kernel-doc and commit message promise more than
  netdev_has_dmabuf_binding() actually checks.

--

net: devmem: add netdev_has_dmabuf_binding() helper

This adds an exported helper, netdev_has_dmabuf_binding(). It walks the
global devmem bindings xarray and reports whether a binding of a given
netdev is still mapped, with an optional filter on the DMA device. The
goal is to let drivers (data direct support in mlx5e) avoid swapping
the DMA device while in-flight skbs may still use dmabuf mappings.

> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index fb693bc1702a4..35bb6fa0f7194 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h

[ ... ]

> +/**
> + * netdev_has_dmabuf_binding - is there a dmabuf binding for a
> + * given @dev and @dma_dev
> + *
> + * @dev: netdev to check bindings
> + * @dma_dev: dma device to check, when NULL it will not be checked
> + *
> + * A binding is reported for as long as it is mapped, which can be long
> + * after the user unbound it: net_iovs of a TX binding can still be queued
> + * in flight, with DMA mappings that are only valid for the device the dmabuf
> + * was attached to.
> + *
> + * Return: true if a binding is found under @dev and possibly @dma_dev
> + */
> +bool netdev_has_dmabuf_binding(struct net_device *dev, struct device *dma_dev);

[ ... ]

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 5c7d9640e9d14..dd3987bbd9f6c 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> +bool netdev_has_dmabuf_binding(struct net_device *dev, struct device *dma_dev)
> +{
> +	struct net_devmem_dmabuf_binding *binding;
> +	unsigned long xa_idx;
> +	bool found = false;
> +
> +	netdev_assert_locked(dev);
> +
> +	/* A binding stays in the xarray until it is freed. binding->dev is
> +	 * only cleared under the instance lock of that netdev, and the free
> +	 * takes it before erasing the binding, so a binding found under @dev
> +	 * stays alive while the xarray lock is dropped.
> +	 */
> +	xa_lock(&net_devmem_dmabuf_bindings);
> +	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
> +		if (READ_ONCE(binding->dev) != dev)
> +			continue;

[Severity: Medium]
Does this check match what the kernel-doc promises, "A binding is
reported for as long as it is mapped"? The commit message also says the
helper will "report every binding of the netdev that is still mapped".

For leased RX queues, binding->dev may not be the device that holds the
mapping. netdev_nl_bind_rx_doit() passes the netdev the user named,
which can be a virtual leasee such as netkit:

net/core/netdev-genl.c:netdev_nl_bind_rx_doit() {
    ...
    binding = net_devmem_bind_dmabuf(netdev, NULL, dma_dev, DMA_FROM_DEVICE,
    ...
}

The dma_dev, though, comes from the physical queue the RX queue is
leased from:

net/core/netdev_queues.c:netdev_queue_get_dma_dev() {
    ...
	hw_rxq = __netif_get_rx_queue(dev, idx)->lease;

	netdev_lock(hw_rxq->dev);
	idx = get_netdev_rx_queue_index(hw_rxq);
	dma_dev = __netdev_queue_get_dma_dev(hw_rxq->dev, idx);
    ...
}

So binding->dev is the leasee, while binding->attachment->dev is the
DMA device of the physical NIC. That also seems to disagree with the
comment on struct net_devmem_dmabuf_binding::dev ("Physical NIC that
does the actual DMA for this binding"), which this helper relies on.

On top of that, both teardown paths skip the unmap in the lease case
but still clear binding->dev:

net/core/devmem.c:mp_dmabuf_devmem_uninstall() {
    ...
			if (!rxq->lease)
				net_devmem_dmabuf_binding_unmap(binding);
			ASSERT_EXCLUSIVE_WRITER(binding->dev);
			WRITE_ONCE(binding->dev, NULL);
    ...
}

net/core/devmem.c:net_devmem_dev_unregister() {
    ...
		if (!netif_is_queue_leasee(dev))
			net_devmem_dmabuf_binding_unmap(binding);
		xa_lock(&net_devmem_dmabuf_bindings);
		WRITE_ONCE(binding->dev, NULL);
    ...
}

After this, the binding stays in the xarray with attachment != NULL
until in-flight net_iovs release it. The check above can no longer
match it for any dev.

Could netdev_has_dmabuf_binding(phys_dev, phys_dma_dev) then return
false while a binding is still mapped against phys_dma_dev?

The only caller in this series, set_pflag_data_direct() in mlx5e,
already rejects installed RX memory providers and leased RX queues, so
it may not hit this today. Since the helper is exported, would it make
sense to either restrict the kernel-doc and commit message to TX
bindings (or bindings whose dev is still set), or to also match
bindings by attachment->dev against the DMA device(s) of @dev?

> +
> +		xa_unlock(&net_devmem_dmabuf_bindings);
> +		mutex_lock(&binding->lock);
> +		found = net_devmem_binding_mapped_to(binding, dma_dev);
> +		mutex_unlock(&binding->lock);
> +		xa_lock(&net_devmem_dmabuf_bindings);
> +		if (found)
> +			break;
> +	}
> +	xa_unlock(&net_devmem_dmabuf_bindings);
> +
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(netdev_has_dmabuf_binding);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110183826.2927274-1-tariqt%40nvidia.com