Re: [PATCH net-next V2 12/13] net/mlx5e: Disable data direct on data direct device unbind

[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 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 2
> 
> 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 suspend teardown and the notifier teardown leave the
>   data_direct private flag in different states.
> - [Medium] mlx5e_data_direct_event() can run against a netdev that
>   mlx5e_nic_disable() or mlx5e_nic_enable() is in the middle of…
> - [Low] Socket-direct gap: both new paths only look at priv->mdev's data-
>   direct device, while mlx5e_queue_get_dma_dev() picks the DMA device…
> - [Low] Notifier payload contract: mlx5_data_direct_do_unbind() now
>   passes reg->dd.dev as the UNBIND payload.
> 
> --
> 
> net/mlx5e: Disable data direct on data direct device unbind
> 
> When the data direct device is unbound, the notifier handler now stops the
> netdev, releases the providers bound to that DMA device and clears the
> data_direct private flag. The suspend path also releases those providers,
> because the notifier is not registered while the device is suspended.
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/data_direct.c b/drivers/net/ethernet/mellanox/mlx5/core/data_direct.c
>> index 7db94b8a5b79c..246a03f44afd3 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/data_direct.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/data_direct.c
>> @@ -198,9 +198,11 @@ static void mlx5_data_direct_bind(struct mlx5_data_direct_registration *reg,
>>  static void
>>  mlx5_data_direct_do_unbind(struct mlx5_data_direct_registration *reg)
>>  {
>> +	struct mlx5_data_direct_dev *dev = reg->dd.dev;
>> +
>>  	WRITE_ONCE(reg->dd.dev, NULL);
>>  	blocking_notifier_call_chain(&reg->users, MLX5_DATA_DIRECT_UNBIND,
>> -				     NULL);
>> +				     dev);
>>  }
> 
> [Severity: Low]
> This isn't a bug, but the UNBIND payload is now an implicit contract. It is
> not documented next to enum mlx5_data_direct_event in
> include/linux/mlx5/data_direct.h.
> 
> reg->dd.dev can be NULL here when mlx5_data_direct_cleanup() calls this for
> a registration that was never bound. mlx5e_data_direct_event() dereferences
> dd_dev->device without a NULL check.
>
This can't happen. So I won't touch this.

>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index ebd41f24330d0..63d91ef0dda80 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -6170,9 +6170,53 @@ static int mlx5e_data_direct_event(struct notifier_block *nb,
> 
> [ ... ]
> 
>> +	if (action != MLX5_DATA_DIRECT_UNBIND)
>> +		return NOTIFY_DONE;
>> +
>> +	rtnl_lock();
>> +	netdev_lock(netdev);
>> +
>> +	mutex_lock(&priv->state_lock);
>> +	enabled = MLX5E_GET_PFLAG(&priv->channels.params,
>> +				  MLX5E_PFLAG_DATA_DIRECT);
>> +	mutex_unlock(&priv->state_lock);
>> +	if (!enabled)
>> +		goto out;
>> +
>> +	reopen = netif_running(netdev);
>> +	netif_close(netdev);
> 
> [Severity: Medium]
> Can this handler run against a netdev that mlx5e_nic_disable() has already
> detached? Nothing checks netif_device_present() before netif_close().
> 
> On the disable side, mlx5_data_direct_do_unbind() sets dd.dev to NULL. The
> handler then blocks in rtnl_lock() while mlx5e_nic_disable() holds rtnl and
> runs:
> 
> 	if (netif_running(priv->netdev))
> 		mlx5e_close(priv->netdev);
> 	netif_device_detach(priv->netdev);
> 	...
> 	mlx5e_data_direct_unbind_dev(priv);
> 	netdev_unlock(priv->netdev);
> 	rtnl_unlock();
> 
> 	mlx5e_nic_set_rx_mode(priv);
> 
> 	mlx5_data_direct_unregister(mdev, &priv->data_direct_nb);
> 
> mlx5e_data_direct_unbind_dev() finds mlx5_data_direct_get_dev() == NULL and
> returns without doing anything. mlx5e_close() is the ndo-level call, so
> IFF_UP and __LINK_STATE_START stay set.
> 
> Once rtnl is dropped, the handler continues with the pflag still set and
> reopen = true. netif_close()->__dev_close_many() clears IFF_UP and
> __LINK_STATE_START and sends NETDEV_GOING_DOWN/NETDEV_DOWN. Then
> netif_open()->__dev_open() fails with -ENODEV because the device is
> detached.
> 
> The profile can be enabled again later, on resume or for the uplink rep
> profile after an eswitch mode change. At that point mlx5e_nic_enable()
> checks:
> 
> 	if (netif_running(netdev))
> 		mlx5e_open(netdev);
> 
> That check is now false. Does the interface then stay administratively
> down?
> 
> The enable side has a similar window. mlx5e_nic_enable() registers the
> notifier before it takes rtnl and before netif_device_attach(). An unbind
> handled in that window would do the same close and failed open on a netdev
> that is still detached.
> 
> Is a netif_device_present() check needed here, or some other handling for
> the detached case?
> 
Since this is low risk, I accepted this as a potential issue that the
user could fix by bringing the interface up again.

But after the necessary suspend/resume fix there is an easy way to fix this.

>> +
>> +	err = netdev_unbind_dma_dev(netdev, dd_dev->device);
>> +	if (WARN_ON_ONCE(err)) {
>> +		netdev_err(netdev, "Failed to unbind dma device, won't reopen netdev, err=%d\n",
>> +			   err);
>> +		goto out;
>> +	}
>> +
>> +	mutex_lock(&priv->state_lock);
>> +	new_params = priv->channels.params;
>> +	MLX5E_SET_PFLAG(&new_params, MLX5E_PFLAG_DATA_DIRECT, false);
>> +	mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, false);
>> +	mutex_unlock(&priv->state_lock);
>> +
>> +	netdev_info(netdev, "data_direct device unbound, disabled data_direct\n");
>>  
>> -	if (action == MLX5_DATA_DIRECT_UNBIND)
>> -		mlx5_core_dbg(priv->mdev, "data direct device unbound\n");
>> +	if (reopen) {
>> +		err = netif_open(netdev, NULL);
>> +		if (err)
>> +			netdev_err(netdev, "Failed to reopen after data_direct unbind, err=%d\n",
>> +				   err);
>> +	}
> 
> [ ... ]
> 
>> @@ -6407,6 +6451,24 @@ static int mlx5e_nic_enable(struct mlx5e_priv *priv)
>>  	return err;
>>  }
>>  
>> +static void mlx5e_data_direct_unbind_dev(struct mlx5e_priv *priv)
>> +{
>> +	struct mlx5_data_direct_dev *dd_dev;
>> +	bool enabled;
>> +
>> +	dd_dev = mlx5_data_direct_get_dev(priv->mdev->data_direct);
> 
> [Severity: Low]
> This helper and the notifier only look at priv->mdev's data direct device.
> mlx5e_queue_get_dma_dev() picks the DMA device per channel:
> 
> 	dma_dev = mlx5e_get_dma_dev(ch->mdev, dd);
> 
> Here ch->mdev comes from mlx5_sd_ch_ix_get_dev(). With socket direct, would
> providers and dmabuf bindings mapped to a secondary PF's data direct device
> stay in place?
> 
> This appears to be resolved later in the series. The patch "net/mlx5e:
> Enable the data direct netdev feature" makes set_pflag_data_direct() return
> -EOPNOTSUPP when mlx5_get_sd(mdev) is non-NULL, and at this commit the flag
> can't be enabled at all.
> 
Right. So no need to change anything here.

>> +	if (!dd_dev)
>> +		return;
>> +
>> +	mutex_lock(&priv->state_lock);
>> +	enabled = MLX5E_GET_PFLAG(&priv->channels.params,
>> +				  MLX5E_PFLAG_DATA_DIRECT);
>> +	mutex_unlock(&priv->state_lock);
>> +	if (enabled)
>> +		WARN_ON_ONCE(netdev_unbind_dma_dev(priv->netdev,
>> +						   dd_dev->device));
>> +}
> 
> [Severity: Medium]
> mlx5e_data_direct_event() clears the flag, but this helper only releases the
> providers. MLX5E_PFLAG_DATA_DIRECT stays set in priv->channels.params. Can
> that leave a stale flag after resume?
> 
> On PCI PM suspend the path is:
> 
> mlx5_suspend()
>   mlx5_unload_one(dev, true)
>     mlx5e_suspend()
>       mlx5e_detach_netdev()
>         mlx5e_nic_disable()
>           mlx5e_data_direct_unbind_dev()
> 
> While the device is suspended, mlx5_unload()->mlx5_data_direct_cleanup()
> frees the registration, and the mlx5e notifier is unregistered.
> 
> The data direct PCI function could go away in that window. The commit
> message says such an unbind "would go unnoticed". In that case
> mlx5_data_direct_init() finds no matching device on resume, and dd.dev
> stays NULL.
> 
> mlx5e_attach_netdev() and mlx5e_nic_enable() keep the old params.
> mlx5e_nic_enable() registers the notifier again without checking the flag
> against mlx5_data_direct_get_dev().
> 
> That has three effects:
> 
> - ethtool reports data_direct as on with no device behind it.
> - mlx5e_queue_get_dma_dev()->mlx5e_get_dma_dev() returns NULL for every
>   queue. Devmem and io_uring zcrx binds then fail instead of falling back
>   to the PF DMA device.
> - mlx5e_alloc_txqsq() keeps creating SQs with MLX5E_SQ_STATE_DATA_DIRECT
>   and the crossing mkey.
> 
> The commit message says "To prevent further use disable the private flag",
> but the flag is not cleared on this path. Should the suspend path clear it
> too, or should mlx5e_nic_enable() check it again?
> 
Although there is a small possibility that the private flag remains set,
data-direct won't be enabled because the channel open code also checks
for the presence of the data-direct device.

Thanks,
Dragos