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;
Date:   Sun, 13 Aug 2023 19:54:49 +0300
From:   Leon Romanovsky <leon@kernel.org>
To:     Petr Pavlu <petr.pavlu@suse.com>
Cc:     tariqt@nvidia.com, yishaih@nvidia.com, 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
Subject: Re: [PATCH net-next v2 03/10] mlx4: Replace the mlx4_interface.event
 callback with a notifier
Message-ID: <20230813165449.GL7707@unreal>
References: <20230813145127.10653-1-petr.pavlu@suse.com>
 <20230813145127.10653-4-petr.pavlu@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230813145127.10653-4-petr.pavlu@suse.com>
Precedence: bulk

On Sun, Aug 13, 2023 at 04:51:20PM +0200, Petr Pavlu wrote:
> 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 <leonro@nvidia.com>
> Acked-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>  drivers/infiniband/hw/mlx4/main.c            | 41 +++++++++++++-------
>  drivers/infiniband/hw/mlx4/mlx4_ib.h         |  2 +
>  drivers/net/ethernet/mellanox/mlx4/en_main.c | 27 +++++++++----
>  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, 77 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 7dd70d778b6b..0761c465120b 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,
> +			 void *ptr)
>  {
> +	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;

You don't need this assignment here as later, you will cast param again,
in your next patches:

  3227         if (event == MLX4_DEV_EVENT_PORT_MGMT_CHANGE)
  3228                 eqe = (struct mlx4_eqe *)param;
  3229         else
  3230                 p = (int) param;

so use ptr directly:

         if (event == MLX4_DEV_EVENT_PORT_MGMT_CHANGE)
                 eqe = param;
         else
                 p = *(int *) param;

Thanks