netdev - Re: [net-next V2 04/15] net/mlx5: Refactor LAG peer device lookout bus logic to mlx5 devcom

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ff42e84c-0859-4e7f-b8e7-1b4e1f7d1c8f@intel.com>
Date: Thu, 12 Oct 2023 14:26:24 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Saeed Mahameed <saeed@...nel.org>, "David S. Miller"
	<davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>
CC: Saeed Mahameed <saeedm@...dia.com>, <netdev@...r.kernel.org>, Tariq Toukan
	<tariqt@...dia.com>, Shay Drory <shayd@...dia.com>, Mark Bloch
	<mbloch@...dia.com>
Subject: Re: [net-next V2 04/15] net/mlx5: Refactor LAG peer device lookout
 bus logic to mlx5 devcom



On 10/12/2023 12:27 PM, Saeed Mahameed wrote:
> From: Shay Drory <shayd@...dia.com>
> 
> LAG peer device lookout bus logic required the usage of global lock,
> mlx5_intf_mutex.
> As part of the effort to remove this global lock, refactor LAG peer
> device lookout to use mlx5 devcom layer.
> 
> Signed-off-by: Shay Drory <shayd@...dia.com>
> Reviewed-by: Mark Bloch <mbloch@...dia.com>
> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/dev.c | 68 -------------------
>  .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 12 ++--
>  .../ethernet/mellanox/mlx5/core/lib/devcom.c  | 14 ++++
>  .../ethernet/mellanox/mlx5/core/lib/devcom.h  |  4 ++
>  .../net/ethernet/mellanox/mlx5/core/main.c    | 25 +++++++
>  .../ethernet/mellanox/mlx5/core/mlx5_core.h   |  1 -
>  include/linux/mlx5/driver.h                   |  1 +
>  7 files changed, 52 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dev.c b/drivers/net/ethernet/mellanox/mlx5/core/dev.c
> index 1fc03480c2ff..6e3a8c22881f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/dev.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/dev.c
> @@ -566,74 +566,6 @@ bool mlx5_same_hw_devs(struct mlx5_core_dev *dev, struct mlx5_core_dev *peer_dev
>  	return (fsystem_guid && psystem_guid && fsystem_guid == psystem_guid);
>  }
>  
> -static u32 mlx5_gen_pci_id(const struct mlx5_core_dev *dev)
> -{
> -	return (u32)((pci_domain_nr(dev->pdev->bus) << 16) |
> -		     (dev->pdev->bus->number << 8) |
> -		     PCI_SLOT(dev->pdev->devfn));
> -}
> -
> -static int _next_phys_dev(struct mlx5_core_dev *mdev,
> -			  const struct mlx5_core_dev *curr)
> -{
> -	if (!mlx5_core_is_pf(mdev))
> -		return 0;
> -
> -	if (mdev == curr)
> -		return 0;
> -
> -	if (!mlx5_same_hw_devs(mdev, (struct mlx5_core_dev *)curr) &&
> -	    mlx5_gen_pci_id(mdev) != mlx5_gen_pci_id(curr))
> -		return 0;
> -
> -	return 1;
> -}
> -
> -static void *pci_get_other_drvdata(struct device *this, struct device *other)
> -{
> -	if (this->driver != other->driver)
> -		return NULL;
> -
> -	return pci_get_drvdata(to_pci_dev(other));
> -}
> -
> -static int next_phys_dev_lag(struct device *dev, const void *data)
> -{
> -	struct mlx5_core_dev *mdev, *this = (struct mlx5_core_dev *)data;
> -
> -	mdev = pci_get_other_drvdata(this->device, dev);
> -	if (!mdev)
> -		return 0;
> -
> -	if (!mlx5_lag_is_supported(mdev))
> -		return 0;
> -
> -	return _next_phys_dev(mdev, data);
> -}
> -
> -static struct mlx5_core_dev *mlx5_get_next_dev(struct mlx5_core_dev *dev,
> -					       int (*match)(struct device *dev, const void *data))
> -{
> -	struct device *next;
> -
> -	if (!mlx5_core_is_pf(dev))
> -		return NULL;
> -
> -	next = bus_find_device(&pci_bus_type, NULL, dev, match);
> -	if (!next)
> -		return NULL;
> -
> -	put_device(next);
> -	return pci_get_drvdata(to_pci_dev(next));
> -}
> -
> -/* Must be called with intf_mutex held */
> -struct mlx5_core_dev *mlx5_get_next_phys_dev_lag(struct mlx5_core_dev *dev)
> -{
> -	lockdep_assert_held(&mlx5_intf_mutex);

The old flow had a lockdep_assert_held

> -	return mlx5_get_next_dev(dev, &next_phys_dev_lag);
> -}
> -
>  void mlx5_dev_list_lock(void)
>  {
>  	mutex_lock(&mlx5_intf_mutex);
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index af3fac090b82..f0b57f97739f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1212,13 +1212,14 @@ static void mlx5_ldev_remove_mdev(struct mlx5_lag *ldev,
>  	dev->priv.lag = NULL;
>  }
>  
> -/* Must be called with intf_mutex held */
> +/* Must be called with HCA devcom component lock held */
>  static int __mlx5_lag_dev_add_mdev(struct mlx5_core_dev *dev)
>  {
> +	struct mlx5_devcom_comp_dev *pos = NULL;
>  	struct mlx5_lag *ldev = NULL;
>  	struct mlx5_core_dev *tmp_dev;
>  
> -	tmp_dev = mlx5_get_next_phys_dev_lag(dev);
> +	tmp_dev = mlx5_devcom_get_next_peer_data(dev->priv.hca_devcom_comp, &pos);
>  	if (tmp_dev)
>  		ldev = mlx5_lag_dev(tmp_dev);
>  

But you didn't bother to add one here? Does
mlx5_devcom_get_next_peer_data already do that?

Not a big deal either way to me.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>