Re: [PATCH net-next V2 09/13] net/mlx5e: Use the correct DMA dev when data_direct is enabled

[Dragos Tatulea <dtatulea@nvidia.com>]

On 12.11.26 10:40, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] This patch makes the data-direct PCI device (dd_dev->device) the
>   DMA device for the page_pool of data-direct RQs (mlx5e_alloc_rq,…
> - [Low] mlx5e_get_dma_dev() (en_main.c:965-976) reads
>   mdev->data_direct->dev with READ_ONCE() through…
> 
> --
> 
> net/mlx5e: Use the correct DMA dev when data_direct is enabled
> 
> This commit adds mlx5e_get_dma_dev(). When data_direct is enabled it
> returns the data direct device instead of the PF PCI device. The helper
> is used for the RQ page_pool DMA device and for .ndo_queue_get_dma_dev,
> and the latter now refuses to return a device in data-direct mode when
> RX queues are leased.
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index 5b99099e4ed0c..ebd41f24330d0 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -962,6 +962,19 @@ static void mlx5e_rq_free_shampo(struct mlx5e_rq *rq)
>>  	kvfree(shampo);
>>  }
>>  
>> +static struct device *mlx5e_get_dma_dev(struct mlx5_core_dev *mdev,
>> +					bool data_direct)
>> +{
>> +	struct mlx5_data_direct_dev *dd_dev;
>> +
>> +	if (!data_direct)
>> +		return mlx5_core_dma_dev(mdev);
>> +
>> +	dd_dev = mlx5_data_direct_get_dev(mdev->data_direct);
>> +
>> +	return dd_dev ? dd_dev->device : NULL;
> 
> [Severity: Low]
> Is anything keeping dd_dev alive between the READ_ONCE() in
> mlx5_data_direct_get_dev() and the dd_dev->device dereference here?
> 
> The unbind side clears the pointer, runs the notifier chain, and then
> frees the device:
> 
> mlx5_data_direct_remove()
>     mlx5_data_direct_dev_unreg(dev)
>         ...
>         mlx5_data_direct_do_unbind()
>             WRITE_ONCE(reg->dd.dev, NULL);
>             blocking_notifier_call_chain(&reg->users,
>                                          MLX5_DATA_DIRECT_UNBIND, NULL);
>     pci_disable_device(pdev);
>     kfree(dev->vuid);
>     kfree(dev);
> 
> At this commit, mlx5e_data_direct_event() only logs on
> MLX5_DATA_DIRECT_UNBIND. It takes none of the locks the readers hold,
> which is priv->state_lock in mlx5e_queue_get_dma_dev() and in the channel
> open path that reaches mlx5e_alloc_rq().
> 
> If a reader has already loaded a non-NULL dd_dev, can it dereference it
> after kfree(dev)? The returned struct device pointer is also used
> without taking a reference.
> 
> This does not look reachable at this commit. set_pflag_data_direct()
> still returns -EOPNOTSUPP, so data_direct is never true here.
> 
> Later in the series, "net/mlx5e: Disable data direct on data direct
> device unbind" makes the notifier take rtnl_lock(), netdev_lock() and
> priv->state_lock before it returns. That serializes it against these
> readers before kfree(dev) runs.
> 
You said it yourself. So nothing to change in this patch.

>> +}
>> +
>>  static int mlx5e_alloc_rq(struct mlx5e_params *params,
>>  			  struct mlx5e_rq_param *rq_param,
>>  			  struct mlx5e_rq_opt_param *rqo,
>> @@ -1105,7 +1118,11 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
>>  		pp_params.flags     = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
>>  		pp_params.pool_size = pool_size;
>>  		pp_params.nid       = node;
>> -		pp_params.dev       = rq->pdev;
>> +		pp_params.dev       = mlx5e_get_dma_dev(mdev, data_direct);
> 
> [Severity: Low]
> With this change, the page_pool of a data direct RQ is created against
> dd_dev->device. mlx5e_queue_get_dma_dev() also hands that device to
> memory providers (devmem, io_uring zcrx), which map their buffers
> against it.
> 
> What tears these down when the data direct device is unbound? At this
> commit, this is the only mlx5e consumer of MLX5_DATA_DIRECT_UNBIND:
> 
> mlx5e_data_direct_event() {
>     ...
> 	if (action == MLX5_DATA_DIRECT_UNBIND)
> 		mlx5_core_dbg(priv->mdev, "data direct device unbound\n");
> 
> 	return NOTIFY_OK;
> }
> 
> It doesn't close the data direct RQs, destroy their page pools, or
> unbind the memory provider mappings. mlx5_data_direct_remove() then
> runs pci_disable_device() and kfree(dev) right after the notifier
> returns.
> 
> Could the page_pool and dmabuf mappings outlive the device they were
> mapped against?
> 
> This also doesn't appear reachable at this commit, since
> set_pflag_data_direct() in en_ethtool.c still returns -EOPNOTSUPP.
> 
> Later in the series, "net/mlx5e: Disable data direct on data direct
> device unbind" rewrites mlx5e_data_direct_event(). The new version calls
> netif_close() and netdev_unbind_dma_dev(), then clears
> MLX5E_PFLAG_DATA_DIRECT. The flag only becomes settable later, in
> "net/mlx5e: Enable the data direct netdev feature".
>
Same.
 
Thanks,
Dragos