Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <72a98e55-46b0-66d6-b4b3-4f6d14e1fd7b@linux.dev>
Date:   Sat, 5 Aug 2023 22:29:29 +0800
MIME-Version: 1.0
Subject: Re: [PATCH net-next 03/10] mlx4: Replace the mlx4_interface.event
 callback with a notifier
To:     Petr Pavlu <petr.pavlu@suse.com>, tariqt@nvidia.com,
        yishaih@nvidia.com, leon@kernel.org
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, jgg@ziepe.ca, netdev@vger.kernel.org,
        linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230804150527.6117-1-petr.pavlu@suse.com>
 <20230804150527.6117-4-petr.pavlu@suse.com>
From:   Zhu Yanjun <yanjun.zhu@linux.dev>
In-Reply-To: <20230804150527.6117-4-petr.pavlu@suse.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

在 2023/8/4 23:05, Petr Pavlu 写道:
> Use a notifier to implement mlx4_dispatch_event() in preparation to
> switch mlx4_en and mlx4_ib to be an auxiliary device.
> 
> A problem is that if the mlx4_interface.event callback was replaced with
> something as mlx4_adrv.event then the implementation of
> mlx4_dispatch_event() would need to acquire a lock on a given device
> before executing this callback. That is necessary because otherwise
> there is no guarantee that the associated driver cannot get unbound when
> the callback is running. However, taking this lock is not possible
> because mlx4_dispatch_event() can be invoked from the hardirq context.
> Using an atomic notifier allows the driver to accurately record when it
> wants to receive these events and solves this problem.
> 
> A handler registration is done by both mlx4_en and mlx4_ib at the end of
> their mlx4_interface.add callback. This matches the current situation
> when mlx4_add_device() would enable events for a given device
> immediately after this callback, by adding the device on the
> mlx4_priv.list.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> Tested-by: Leon Romanovsky <leon@kernel.org>
> ---
>   drivers/infiniband/hw/mlx4/main.c            | 41 +++++++++++++-------
>   drivers/infiniband/hw/mlx4/mlx4_ib.h         |  2 +
>   drivers/net/ethernet/mellanox/mlx4/en_main.c | 25 ++++++++----
>   drivers/net/ethernet/mellanox/mlx4/intf.c    | 24 ++++++++----
>   drivers/net/ethernet/mellanox/mlx4/main.c    |  2 +
>   drivers/net/ethernet/mellanox/mlx4/mlx4.h    |  2 +
>   drivers/net/ethernet/mellanox/mlx4/mlx4_en.h |  2 +
>   include/linux/mlx4/driver.h                  |  8 +++-
>   8 files changed, 76 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 7dd70d778b6b..458b4b11dffa 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -82,6 +82,8 @@ static const char mlx4_ib_version[] =
>   static void do_slave_init(struct mlx4_ib_dev *ibdev, int slave, int do_init);
>   static enum rdma_link_layer mlx4_ib_port_link_layer(struct ib_device *device,
>   						    u32 port_num);
> +static int mlx4_ib_event(struct notifier_block *this, unsigned long event,
> +			 void *ptr);
>   
>   static struct workqueue_struct *wq;
>   
> @@ -2836,6 +2838,12 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
>   				do_slave_init(ibdev, j, 1);
>   		}
>   	}
> +
> +	/* register mlx4 core notifier */
> +	ibdev->mlx_nb.notifier_call = mlx4_ib_event;
> +	err = mlx4_register_event_notifier(dev, &ibdev->mlx_nb);
> +	WARN(err, "failed to register mlx4 event notifier (%d)", err);
> +
>   	return ibdev;
>   
>   err_notif:
> @@ -2953,6 +2961,8 @@ static void mlx4_ib_remove(struct mlx4_dev *dev, void *ibdev_ptr)
>   	int p;
>   	int i;
>   
> +	mlx4_unregister_event_notifier(dev, &ibdev->mlx_nb);
> +
>   	mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_IB)
>   		devlink_port_type_clear(mlx4_get_devlink_port(dev, i));
>   	ibdev->ib_active = false;
> @@ -3173,11 +3183,14 @@ void mlx4_sched_ib_sl2vl_update_work(struct mlx4_ib_dev *ibdev,
>   	}
>   }
>   
> -static void mlx4_ib_event(struct mlx4_dev *dev, void *ibdev_ptr,
> -			  enum mlx4_dev_event event, unsigned long param)
> +static int mlx4_ib_event(struct notifier_block *this,
> +			 unsigned long event /*mlx4_dev_event*/, void *ptr)

/*mlx4_dev_event*/ should be removed?

Zhu Yanjun

>   {
> +	struct mlx4_ib_dev *ibdev =
> +		container_of(this, struct mlx4_ib_dev, mlx_nb);
> +	struct mlx4_dev *dev = ibdev->dev;
> +	unsigned long param = *(unsigned long *)ptr;
>   	struct ib_event ibev;
> -	struct mlx4_ib_dev *ibdev = to_mdev((struct ib_device *) ibdev_ptr);
>   	struct mlx4_eqe *eqe = NULL;
>   	struct ib_event_work *ew;
>   	int p = 0;
> @@ -3187,11 +3200,11 @@ static void mlx4_ib_event(struct mlx4_dev *dev, void *ibdev_ptr,
>   	    (event == MLX4_DEV_EVENT_PORT_DOWN))) {
>   		ew = kmalloc(sizeof(*ew), GFP_ATOMIC);
>   		if (!ew)
> -			return;
> +			return NOTIFY_DONE;
>   		INIT_WORK(&ew->work, handle_bonded_port_state_event);
>   		ew->ib_dev = ibdev;
>   		queue_work(wq, &ew->work);
> -		return;
> +		return NOTIFY_DONE;
>   	}
>   
>   	if (event == MLX4_DEV_EVENT_PORT_MGMT_CHANGE)
> @@ -3202,7 +3215,7 @@ static void mlx4_ib_event(struct mlx4_dev *dev, void *ibdev_ptr,
>   	switch (event) {
>   	case MLX4_DEV_EVENT_PORT_UP:
>   		if (p > ibdev->num_ports)
> -			return;
> +			return NOTIFY_DONE;
>   		if (!mlx4_is_slave(dev) &&
>   		    rdma_port_get_link_layer(&ibdev->ib_dev, p) ==
>   			IB_LINK_LAYER_INFINIBAND) {
> @@ -3217,7 +3230,7 @@ static void mlx4_ib_event(struct mlx4_dev *dev, void *ibdev_ptr,
>   
>   	case MLX4_DEV_EVENT_PORT_DOWN:
>   		if (p > ibdev->num_ports)
> -			return;
> +			return NOTIFY_DONE;
>   		ibev.event = IB_EVENT_PORT_ERR;
>   		break;
>   
> @@ -3230,7 +3243,7 @@ static void mlx4_ib_event(struct mlx4_dev *dev, void *ibdev_ptr,
>   	case MLX4_DEV_EVENT_PORT_MGMT_CHANGE:
>   		ew = kmalloc(sizeof *ew, GFP_ATOMIC);
>   		if (!ew)
> -			return;
> +			return NOTIFY_DONE;
>   
>   		INIT_WORK(&ew->work, handle_port_mgmt_change_event);
>   		memcpy(&ew->ib_eqe, eqe, sizeof *eqe);
> @@ -3240,7 +3253,7 @@ static void mlx4_ib_event(struct mlx4_dev *dev, void *ibdev_ptr,
>   			queue_work(wq, &ew->work);
>   		else
>   			handle_port_mgmt_change_event(&ew->work);
> -		return;
> +		return NOTIFY_DONE;
>   
>   	case MLX4_DEV_EVENT_SLAVE_INIT:
>   		/* here, p is the slave id */
> @@ -3256,7 +3269,7 @@ static void mlx4_ib_event(struct mlx4_dev *dev, void *ibdev_ptr,
>   								       1);
>   			}
>   		}
> -		return;
> +		return NOTIFY_DONE;
>   
>   	case MLX4_DEV_EVENT_SLAVE_SHUTDOWN:
>   		if (mlx4_is_master(dev)) {
> @@ -3272,22 +3285,22 @@ static void mlx4_ib_event(struct mlx4_dev *dev, void *ibdev_ptr,
>   		}
>   		/* here, p is the slave id */
>   		do_slave_init(ibdev, p, 0);
> -		return;
> +		return NOTIFY_DONE;
>   
>   	default:
> -		return;
> +		return NOTIFY_DONE;
>   	}
>   
> -	ibev.device	      = ibdev_ptr;
> +	ibev.device	      = &ibdev->ib_dev;
>   	ibev.element.port_num = mlx4_is_bonded(ibdev->dev) ? 1 : (u8)p;
>   
>   	ib_dispatch_event(&ibev);
> +	return NOTIFY_DONE;
>   }
>   
>   static struct mlx4_interface mlx4_ib_interface = {
>   	.add		= mlx4_ib_add,
>   	.remove		= mlx4_ib_remove,
> -	.event		= mlx4_ib_event,
>   	.protocol	= MLX4_PROT_IB_IPV6,
>   	.flags		= MLX4_INTFF_BONDING
>   };
> diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
> index 17fee1e73a45..41ca1114a995 100644
> --- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
> +++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
> @@ -38,6 +38,7 @@
>   #include <linux/list.h>
>   #include <linux/mutex.h>
>   #include <linux/idr.h>
> +#include <linux/notifier.h>
>   
>   #include <rdma/ib_verbs.h>
>   #include <rdma/ib_umem.h>
> @@ -644,6 +645,7 @@ struct mlx4_ib_dev {
>   	spinlock_t		reset_flow_resource_lock;
>   	struct list_head		qp_list;
>   	struct mlx4_ib_diag_counters diag_counters[MLX4_DIAG_COUNTERS_TYPES];
> +	struct notifier_block	mlx_nb;
>   };
>   
>   struct ib_event_work {
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_main.c b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> index be8ba34c9025..8384bff5c37d 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> @@ -183,17 +183,20 @@ static void mlx4_en_get_profile(struct mlx4_en_dev *mdev)
>   	}
>   }
>   
> -static void mlx4_en_event(struct mlx4_dev *dev, void *endev_ptr,
> -			  enum mlx4_dev_event event, unsigned long port)
> +static int mlx4_en_event(struct notifier_block *this,
> +			 unsigned long event /*mlx4_dev_event*/, void *ptr)
>   {
> -	struct mlx4_en_dev *mdev = (struct mlx4_en_dev *) endev_ptr;
> +	struct mlx4_en_dev *mdev =
> +		container_of(this, struct mlx4_en_dev, mlx_nb);
> +	struct mlx4_dev *dev = mdev->dev;
> +	unsigned long port = *(unsigned long *)ptr;
>   	struct mlx4_en_priv *priv;
>   
>   	switch (event) {
>   	case MLX4_DEV_EVENT_PORT_UP:
>   	case MLX4_DEV_EVENT_PORT_DOWN:
>   		if (!mdev->pndev[port])
> -			return;
> +			return NOTIFY_DONE;
>   		priv = netdev_priv(mdev->pndev[port]);
>   		/* To prevent races, we poll the link state in a separate
>   		  task rather than changing it here */
> @@ -211,10 +214,12 @@ static void mlx4_en_event(struct mlx4_dev *dev, void *endev_ptr,
>   	default:
>   		if (port < 1 || port > dev->caps.num_ports ||
>   		    !mdev->pndev[port])
> -			return;
> -		mlx4_warn(mdev, "Unhandled event %d for port %d\n", event,
> +			return NOTIFY_DONE;
> +		mlx4_warn(mdev, "Unhandled event %d for port %d\n", (int) event,
>   			  (int) port);
>   	}
> +
> +	return NOTIFY_DONE;
>   }
>   
>   static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
> @@ -222,6 +227,8 @@ static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
>   	struct mlx4_en_dev *mdev = endev_ptr;
>   	int i;
>   
> +	mlx4_unregister_event_notifier(dev, &mdev->mlx_nb);
> +
>   	mutex_lock(&mdev->state_lock);
>   	mdev->device_up = false;
>   	mutex_unlock(&mdev->state_lock);
> @@ -326,6 +333,11 @@ static void *mlx4_en_add(struct mlx4_dev *dev)
>   	mutex_init(&mdev->state_lock);
>   	mdev->device_up = true;
>   
> +	/* register mlx4 core notifier */
> +	mdev->mlx_nb.notifier_call = mlx4_en_event;
> +	err = mlx4_register_event_notifier(dev, &mdev->mlx_nb);
> +	WARN(err, "failed to register mlx4 event notifier (%d)", err);
> +
>   	return mdev;
>   
>   err_mr:
> @@ -346,7 +358,6 @@ static void *mlx4_en_add(struct mlx4_dev *dev)
>   static struct mlx4_interface mlx4_en_interface = {
>   	.add		= mlx4_en_add,
>   	.remove		= mlx4_en_remove,
> -	.event		= mlx4_en_event,
>   	.protocol	= MLX4_PROT_ETH,
>   	.activate	= mlx4_en_activate,
>   };
> diff --git a/drivers/net/ethernet/mellanox/mlx4/intf.c b/drivers/net/ethernet/mellanox/mlx4/intf.c
> index 28d7da925d36..a7c3e2efa464 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/intf.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/intf.c
> @@ -183,17 +183,27 @@ void mlx4_dispatch_event(struct mlx4_dev *dev, enum mlx4_dev_event type,
>   			 unsigned long param)
>   {
>   	struct mlx4_priv *priv = mlx4_priv(dev);
> -	struct mlx4_device_context *dev_ctx;
> -	unsigned long flags;
>   
> -	spin_lock_irqsave(&priv->ctx_lock, flags);
> +	atomic_notifier_call_chain(&priv->event_nh, type, &param);
> +}
>   
> -	list_for_each_entry(dev_ctx, &priv->ctx_list, list)
> -		if (dev_ctx->intf->event)
> -			dev_ctx->intf->event(dev, dev_ctx->context, type, param);
> +int mlx4_register_event_notifier(struct mlx4_dev *dev,
> +				 struct notifier_block *nb)
> +{
> +	struct mlx4_priv *priv = mlx4_priv(dev);
>   
> -	spin_unlock_irqrestore(&priv->ctx_lock, flags);
> +	return atomic_notifier_chain_register(&priv->event_nh, nb);
> +}
> +EXPORT_SYMBOL(mlx4_register_event_notifier);
> +
> +int mlx4_unregister_event_notifier(struct mlx4_dev *dev,
> +				   struct notifier_block *nb)
> +{
> +	struct mlx4_priv *priv = mlx4_priv(dev);
> +
> +	return atomic_notifier_chain_unregister(&priv->event_nh, nb);
>   }
> +EXPORT_SYMBOL(mlx4_unregister_event_notifier);
>   
>   int mlx4_register_device(struct mlx4_dev *dev)
>   {
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index 8a5409b00530..5f3ba8385e23 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -3378,6 +3378,8 @@ static int mlx4_load_one(struct pci_dev *pdev, int pci_dev_data,
>   	INIT_LIST_HEAD(&priv->ctx_list);
>   	spin_lock_init(&priv->ctx_lock);
>   
> +	ATOMIC_INIT_NOTIFIER_HEAD(&priv->event_nh);
> +
>   	mutex_init(&priv->port_mutex);
>   	mutex_init(&priv->bond_mutex);
>   
> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
> index 6ccf340660d9..10f12e4992f1 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h
> +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
> @@ -47,6 +47,7 @@
>   #include <linux/spinlock.h>
>   #include <net/devlink.h>
>   #include <linux/rwsem.h>
> +#include <linux/notifier.h>
>   
>   #include <linux/mlx4/device.h>
>   #include <linux/mlx4/driver.h>
> @@ -878,6 +879,7 @@ struct mlx4_priv {
>   	struct list_head	dev_list;
>   	struct list_head	ctx_list;
>   	spinlock_t		ctx_lock;
> +	struct atomic_notifier_head event_nh;
>   
>   	int			pci_dev_data;
>   	int                     removed;
> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> index 72a3fea36702..efe3f97b874f 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> @@ -49,6 +49,7 @@
>   #include <linux/ptp_clock_kernel.h>
>   #include <linux/irq.h>
>   #include <net/xdp.h>
> +#include <linux/notifier.h>
>   
>   #include <linux/mlx4/device.h>
>   #include <linux/mlx4/qp.h>
> @@ -433,6 +434,7 @@ struct mlx4_en_dev {
>   	struct ptp_clock	*ptp_clock;
>   	struct ptp_clock_info	ptp_clock_info;
>   	struct notifier_block	netdev_nb;
> +	struct notifier_block	mlx_nb;
>   };
>   
>   
> diff --git a/include/linux/mlx4/driver.h b/include/linux/mlx4/driver.h
> index 923951e19300..228da8ed7e75 100644
> --- a/include/linux/mlx4/driver.h
> +++ b/include/linux/mlx4/driver.h
> @@ -34,6 +34,7 @@
>   #define MLX4_DRIVER_H
>   
>   #include <net/devlink.h>
> +#include <linux/notifier.h>
>   #include <linux/mlx4/device.h>
>   
>   struct mlx4_dev;
> @@ -57,8 +58,6 @@ enum {
>   struct mlx4_interface {
>   	void *			(*add)	 (struct mlx4_dev *dev);
>   	void			(*remove)(struct mlx4_dev *dev, void *context);
> -	void			(*event) (struct mlx4_dev *dev, void *context,
> -					  enum mlx4_dev_event event, unsigned long param);
>   	void			(*activate)(struct mlx4_dev *dev, void *context);
>   	struct list_head	list;
>   	enum mlx4_protocol	protocol;
> @@ -87,6 +86,11 @@ struct mlx4_port_map {
>   
>   int mlx4_port_map_set(struct mlx4_dev *dev, struct mlx4_port_map *v2p);
>   
> +int mlx4_register_event_notifier(struct mlx4_dev *dev,
> +				 struct notifier_block *nb);
> +int mlx4_unregister_event_notifier(struct mlx4_dev *dev,
> +				   struct notifier_block *nb);
> +
>   struct devlink_port *mlx4_get_devlink_port(struct mlx4_dev *dev, int port);
>   
>   #endif /* MLX4_DRIVER_H */