Re: [PATCH 2/3] ib_srp: Protect the target list with a mutex instead of a spinlock

[Leon Romanovsky <leonro@mellanox.com>]

On Tue, Feb 15, 2022 at 10:26:49AM -0800, Bart Van Assche wrote:
> This patch makes the next patch in this series easier to read.

It is not readability change, but move to lock that can sleep (mutex),
which is always good thing.

Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> 
> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c | 18 +++++++++---------
>  drivers/infiniband/ulp/srp/ib_srp.h |  4 ++--
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index e174e853f8a4..2db7429b42e1 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1062,9 +1062,9 @@ static void srp_remove_target(struct srp_target_port *target)
>  	kfree(target->ch);
>  	target->ch = NULL;
>  
> -	spin_lock(&target->srp_host->target_lock);
> +	mutex_lock(&target->srp_host->target_mutex);
>  	list_del(&target->list);
> -	spin_unlock(&target->srp_host->target_lock);
> +	mutex_unlock(&target->srp_host->target_mutex);
>  
>  	scsi_host_put(target->scsi_host);
>  }
> @@ -3146,9 +3146,9 @@ static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
>  	rport->lld_data = target;
>  	target->rport = rport;
>  
> -	spin_lock(&host->target_lock);
> +	mutex_lock(&host->target_mutex);
>  	list_add_tail(&target->list, &host->target_list);
> -	spin_unlock(&host->target_lock);
> +	mutex_unlock(&host->target_mutex);
>  
>  	scsi_scan_target(&target->scsi_host->shost_gendev,
>  			 0, target->scsi_id, SCAN_WILD_CARD, SCSI_SCAN_INITIAL);
> @@ -3203,7 +3203,7 @@ static bool srp_conn_unique(struct srp_host *host,
>  
>  	ret = true;
>  
> -	spin_lock(&host->target_lock);
> +	mutex_lock(&host->target_mutex);
>  	list_for_each_entry(t, &host->target_list, list) {
>  		if (t != target &&
>  		    target->id_ext == t->id_ext &&
> @@ -3213,7 +3213,7 @@ static bool srp_conn_unique(struct srp_host *host,
>  			break;
>  		}
>  	}
> -	spin_unlock(&host->target_lock);
> +	mutex_unlock(&host->target_mutex);
>  
>  out:
>  	return ret;
> @@ -3898,7 +3898,7 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port)
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&host->target_list);
> -	spin_lock_init(&host->target_lock);
> +	mutex_init(&host->target_mutex);
>  	init_completion(&host->released);
>  	mutex_init(&host->add_target_mutex);
>  	host->srp_dev = device;
> @@ -4041,10 +4041,10 @@ static void srp_remove_one(struct ib_device *device, void *client_data)
>  		/*
>  		 * Remove all target ports.
>  		 */
> -		spin_lock(&host->target_lock);
> +		mutex_lock(&host->target_mutex);
>  		list_for_each_entry(target, &host->target_list, list)
>  			srp_queue_remove_work(target);
> -		spin_unlock(&host->target_lock);
> +		mutex_unlock(&host->target_mutex);
>  
>  		/*
>  		 * Wait for tl_err and target port removal tasks.
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
> index 55a575e2cace..2af968277994 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.h
> +++ b/drivers/infiniband/ulp/srp/ib_srp.h
> @@ -116,14 +116,14 @@ struct srp_device {
>   * One port of an RDMA adapter in the initiator system.
>   *
>   * @target_list: List of connected target ports (struct srp_target_port).
> - * @target_lock: Protects @target_list.
> + * @target_mutex: Protects @target_list.
>   */
>  struct srp_host {
>  	struct srp_device      *srp_dev;
>  	u8			port;
>  	struct device		dev;
>  	struct list_head	target_list;
> -	spinlock_t		target_lock;
> +	struct mutex		target_mutex;
>  	struct completion	released;
>  	struct list_head	list;
>  	struct mutex		add_target_mutex;