All of lore.kernel.org
 help / color / mirror / Atom feed
From: Leon Romanovsky <leon@kernel.org>
To: sashiko-reviews@lists.linux.dev
Cc: Tariq Toukan <tariqt@nvidia.com>, linux-rdma@vger.kernel.org
Subject: Re: [PATCH mlx5-next 10/10] mlx5: Move data direct implementation to mlx5_core
Date: Mon, 28 Sep 2026 14:34:05 +0300	[thread overview]
Message-ID: <20260928113405.GU563127@unreal> (raw)
In-Reply-To: <20260918111634.4B8BD1F00893@smtp.kernel.org>

On Fri, Sep 18, 2026 at 11:16:33AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mlx5: fatal device probe failure on optional data direct init error
> --
> 
> commit 99d3c5a833c6ca3bd3a1b016b15bbb8a8b597a79
> Author: Dragos Tatulea <dtatulea@nvidia.com>
> 
> mlx5: Move data direct implementation to mlx5_core
> 
> This commit moves the data direct implementation from mlx5_ib to mlx5_core
> so it can be used by netdev devices. The data direct initialization is moved
> to mlx5_load() and resource creation is managed by the core driver.
> 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/data_direct.c b/drivers/net/ethernet/mellanox/mlx5/core/data_direct.c
> > rename from drivers/infiniband/hw/mlx5/data_direct.c
> > rename to drivers/net/ethernet/mellanox/mlx5/core/data_direct.c
> > index 3878c4aebb2c6..67717110feab0 100644
> > --- a/drivers/infiniband/hw/mlx5/data_direct.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/data_direct.c
> 
> [ ... ]
> 
> > -	err = mlx5_data_direct_query_vuid(ibdev->mdev, reg->vuid);
> > +	err = mlx5_data_direct_query_vuid(mdev, reg->vuid);
> >  	if (err) {
> > -		mlx5_ib_warn(ibdev, "Failed to query VUID, disabling data direct, err=%d\n",
> > -			     err);
> > +		mlx5_core_warn(mdev,
> > +			       "Failed to query VUID, disabling data direct, err=%d\n",
> > +			       err);
> >  		kfree(reg);
> > -		return err;
> > +		return -EINVAL;
> >  	}
> 
> [Severity: High]
> In mlx5_data_direct_init(), does this code inadvertently cause a fatal probe
> failure? The warning claims the optional feature is being disabled, but
> returning -EINVAL bubbles up and aborts the entire core driver probe. If the
> intent is to gracefully disable the feature when the VUID query fails,
> shouldn't it return 0 instead?

There is a need to update the comment, the failure to enable data direct
is fatal, as we are enabling based on FW capability for very specific
devices which support data-direct mode.

> 
> [ ... ]
> 
> > -int mlx5_data_direct_register(struct mlx5_ib_dev *ibdev,
> > +int mlx5_data_direct_register(struct mlx5_core_dev *mdev,
> >  			      struct notifier_block *nb)
> >  {
> >  	struct mlx5_data_direct_registration *reg;
> >  
> > -	if (!mlx5_data_direct_supported(ibdev->mdev))
> > +	if (!mlx5_data_direct_supported(mdev))
> >  		return 0;
> >  
> > -	reg = container_of(ibdev->data_direct,
> > -			    struct mlx5_data_direct_registration, dd);
> > +	reg = container_of(mdev->data_direct,
> > +			   struct mlx5_data_direct_registration, dd);
> 
> [Severity: High]
> If mlx5_data_direct_init() is modified to return 0 on failure to allow
> graceful degradation, will this code in mlx5_data_direct_register() trigger
> a NULL pointer dereference?
> 
> It appears mlx5_data_direct_supported() only checks hardware capabilities,
> blindly assuming mdev->data_direct is allocated. Should
> mlx5_data_direct_supported() also check if mdev->data_direct is valid, or
> should there be an explicit check here?

No, if capabilities exist, we should or enable direct mode or fail
driver probe.

> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index 643b4aac20336..34f0375b68513 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> 
> [ ... ]
> 
> > @@ -1339,8 +1340,14 @@ static int mlx5_load(struct mlx5_core_dev *dev)
> >  	if (err)
> >  		goto err_traps_reg;
> >  
> > +	err = mlx5_data_direct_init(dev);
> > +	if (err)
> > +		goto err_data_direct;
> > +
> >  	return 0;
> 
> [Severity: High]
> Does propagating the error from mlx5_data_direct_init() here in mlx5_load()
> unintentionally disable the entire NIC (both Ethernet and IB) when the
> optional data direct initialization fails?

Same, thanks

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

      reply	other threads:[~2026-09-28 11:34 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-17 11:14 [PATCH mlx5-next 00/10] mlx5: Move data direct infrastructure to mlx5_core Tariq Toukan
2026-09-17 11:14 ` [PATCH mlx5-next 01/10] RDMA/mlx5: Notify data direct users via a notifier chain on unbind Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-17 11:14 ` [PATCH mlx5-next 02/10] RDMA/mlx5: Add mlx5_data_direct_supported() helper Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-17 11:14 ` [PATCH mlx5-next 03/10] RDMA/mlx5: Make the data direct VUID query self-contained Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-17 11:14 ` [PATCH mlx5-next 04/10] RDMA/mlx5: Give the data direct PCI driver a proper name Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-17 11:14 ` [PATCH mlx5-next 05/10] RDMA/mlx5: Move and rename data direct resource functions Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-17 11:14 ` [PATCH mlx5-next 06/10] RDMA/mlx5: Add new registration stage for data direct users Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-17 11:14 ` [PATCH mlx5-next 07/10] RDMA/mlx5: Extract IB specific lock out of data direct Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-28 11:39     ` Leon Romanovsky
2026-09-17 11:14 ` [PATCH mlx5-next 08/10] RDMA/mlx5: Consolidate data direct state into one object Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-17 11:14 ` [PATCH mlx5-next 09/10] RDMA/mlx5: Pull data_direct resource creation in init phase Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-17 11:14 ` [PATCH mlx5-next 10/10] mlx5: Move data direct implementation to mlx5_core Tariq Toukan
2026-09-18 11:16   ` sashiko-bot
2026-09-28 11:34     ` Leon Romanovsky [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260928113405.GU563127@unreal \
    --to=leon@kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=tariqt@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.