Re: [PATCH net-next v2 03/10] mlx4: Replace the mlx4_interface.event callback with a notifier

[Petr Pavlu]

On 8/13/23 18:54, Leon Romanovsky wrote:
> 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@xxxxxxxx>
>> Tested-by: Leon Romanovsky <leonro@xxxxxxxxxx>
>> Acked-by: Tariq Toukan <tariqt@xxxxxxxxxx>
>> ---
>>  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;

Function mlx4_dispatch_event() currently takes an 'unsigned long' as its
event parameter. The patch updates the function to use
atomic_notifier_call_chain() which however expects 'void *' as the
'param' value. To solve the mismatch, mlx4_dispatch_event() passes to
atomic_notifier_call_chain() an address of the original 'param'. This
creates one additional level of indirection which the handlers, such as
mlx4_ib_event(), need to deal with. Line
'unsigned long param = *(unsigned long *)ptr;' is added for that.

I think the best way to avoid this complexity would be for
mlx4_dispatch_event() to take 'void *' as its 'param' in the first
place. I would add the following patch before this one in v3 of the
series.