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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net/mlx5e: stale MLX5E_PFLAG_DATA_DIRECT after unbind during
  suspend
- [Medium] net/mlx5e: mlx5e_data_direct_event() closes detached netdev
  on race
--

commit 25c3357461eb765e71d4e416eff4cb0e4cfec6a7
Author: Dragos Tatulea <dtatulea@nvidia.com>

net/mlx5e: Disable data direct on data direct device unbind

This commit stops the netdev, unbinds the data direct DMA device, and
disables the data_direct private flag when the data direct device is
unbound, and also unbinds the DMA device during mlx5e_nic_disable().

> 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,
>  {
>  	struct mlx5e_priv *priv = container_of(nb, struct mlx5e_priv,
>  					       data_direct_nb);
> +	struct mlx5_data_direct_dev *dd_dev = data;
> +	struct net_device *netdev = priv->netdev;
> +	struct mlx5e_params new_params;
> +	bool enabled, reopen;
> +	int err;
> +
> +	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);
> +
> +	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);
> +	}

[Severity: Medium]
Can mlx5e_data_direct_event() clear IFF_UP and leave netdev down if a data
direct unbind races with mlx5e_nic_disable() or mlx5e_nic_enable()?

CPU0                                  CPU1
mlx5e_nic_disable()
  rtnl_lock()
  netdev_lock(netdev)
  mlx5e_close(netdev)
  netif_device_detach(netdev)
                                      mlx5_data_direct_do_unbind()
                                        WRITE_ONCE(reg->dd.dev, NULL)
                                        blocking_notifier_call_chain()
                                          mlx5e_data_direct_event()
                                            rtnl_lock()  [blocks]
  mlx5e_data_direct_unbind_dev()
    dd_dev = NULL -> returns
  netdev_unlock(netdev)
  rtnl_unlock()
                                            netdev_lock(netdev)
                                            reopen = netif_running(netdev)
                                            netif_close(netdev)
                                            ...
                                            netif_open(netdev, NULL)
                                              __dev_open() -> -ENODEV
  mlx5_data_direct_unregister()

mlx5e_nic_disable() detaches the netdev while keeping __LINK_STATE_START
(netif_running(netdev)) and IFF_UP set so that mlx5e_nic_enable() can
reopen it on resume.  The same window exists in mlx5e_nic_enable() between
mlx5_data_direct_register() and netif_device_attach().  When
mlx5e_data_direct_event() runs while !netif_device_present(netdev),
netif_close(netdev) clears IFF_UP and __LINK_STATE_START, and
netif_open(netdev, NULL) then fails with -ENODEV, leaving the interface
down after resume.

> @@ -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);
> +	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]
Does MLX5E_PFLAG_DATA_DIRECT stay set after resume if the data direct
device is unbound while the NIC is suspended?

During suspend, mlx5e_nic_disable() calls mlx5e_data_direct_unbind_dev()
to unbind the DMA device, then unregisters priv->data_direct_nb.  If the
data direct device is unbound before mlx5e_nic_enable() re-registers the
notifier on resume, mlx5e_data_direct_event() never runs and
mdev->data_direct->dev remains NULL, but MLX5E_PFLAG_DATA_DIRECT is still
set in priv->channels.params.

When mlx5e_nic_enable() reopens the netdev, TX queues are allocated with
MLX5E_SQ_STATE_DATA_DIRECT set without a bound data direct device, and
mlx5e_queue_get_dma_dev() passes dd = true to mlx5e_get_dma_dev(),
returning NULL for every queue instead of the PF DMA device and breaking
subsequent memory provider and dmabuf bindings.  Should mlx5e_nic_enable()
clear MLX5E_PFLAG_DATA_DIRECT if mlx5_data_direct_get_dev() is NULL after
registering the notifier?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110183826.2927274-1-tariqt@nvidia.com?part=12