Re: [PATCH rdma-rc 1/3] RDMA/mlx5: Use xa_lock_irqsave when access to SRQ table

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Jul 07, 2020 at 02:06:10PM +0300, Leon Romanovsky wrote:
> Fixes: b02a29eb8841 ("mlx5: Convert mlx5_srq_table to XArray")

This didn't introduce the bug, when things were converted to xarray it
already had the wrong spinlock type.

I'm surprised this is only been found now since it has been wrong for
years. Did something else change?

> Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>  drivers/infiniband/hw/mlx5/srq_cmd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx5/srq_cmd.c b/drivers/infiniband/hw/mlx5/srq_cmd.c
> index 6f5eadc4d183..be0e5469dad0 100644
> +++ b/drivers/infiniband/hw/mlx5/srq_cmd.c
> @@ -82,12 +82,13 @@ struct mlx5_core_srq *mlx5_cmd_get_srq(struct mlx5_ib_dev *dev, u32 srqn)
>  {
>  	struct mlx5_srq_table *table = &dev->srq_table;
>  	struct mlx5_core_srq *srq;
> +	unsigned long flags;
>  
> -	xa_lock(&table->array);
> +	xa_lock_irqsave(&table->array, flags);
>  	srq = xa_load(&table->array, srqn);
>  	if (srq)
>  		refcount_inc(&srq->common.refcount);
> -	xa_unlock(&table->array);
> +	xa_unlock_irqrestore(&table->array, flags);

This and other places can just be xa_lock_irq as we are not in an atomic
context here.
  
>  	return srq;
>  }
> @@ -644,6 +645,7 @@ static int srq_event_notifier(struct notifier_block *nb,
>  	struct mlx5_srq_table *table;
>  	struct mlx5_core_srq *srq;
>  	struct mlx5_eqe *eqe;
> +	unsigned long flags;
>  	u32 srqn;
>  
>  	if (type != MLX5_EVENT_TYPE_SRQ_CATAS_ERROR &&
> @@ -655,11 +657,11 @@ static int srq_event_notifier(struct notifier_block *nb,
>  	eqe = data;
>  	srqn = be32_to_cpu(eqe->data.qp_srq.qp_srq_n) & 0xffffff;
>  
> -	xa_lock(&table->array);
> +	xa_lock_irqsave(&table->array, flags);
>  	srq = xa_load(&table->array, srqn);
>  	if (srq)
>  		refcount_inc(&srq->common.refcount);
> -	xa_unlock(&table->array);
> +	xa_unlock_irqrestore(&table->array, flags);

This change isn't needed, the notifier is always called from an IRQ
context

Jason