[PATCH] scsi: ufs: add missing host_lock in setup_xfer_req

[PATCH] scsi: ufs: add missing host_lock in setup_xfer_req

[Jaegeuk Kim]

This patch adds a host_lock which existed before on ufshcd_vops_setup_xfer_req.

Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Fixes: a45f937110fa ("scsi: ufs: Optimize host lock on transfer requests send/compl paths")
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 drivers/scsi/ufs/ufshcd.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c98d540ac044..194755c9ddfe 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1229,8 +1229,13 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
 static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
 					bool is_scsi_cmd)
 {
-	if (hba->vops && hba->vops->setup_xfer_req)
-		return hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
+	if (hba->vops && hba->vops->setup_xfer_req) {
+		unsigned long flags;
+
+		spin_lock_irqsave(hba->host->host_lock, flags);
+		hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
+		spin_unlock_irqrestore(hba->host->host_lock, flags);
+	}
 }
 
 static inline void ufshcd_vops_setup_task_mgmt(struct ufs_hba *hba,
-- 
2.32.0.93.g670b81a890-goog

Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req

[Bart Van Assche]

On 6/30/21 5:51 PM, Jaegeuk Kim wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c98d540ac044..194755c9ddfe 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1229,8 +1229,13 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
>  static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
>  					bool is_scsi_cmd)
>  {
> -	if (hba->vops && hba->vops->setup_xfer_req)
> -		return hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> +	if (hba->vops && hba->vops->setup_xfer_req) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(hba->host->host_lock, flags);
> +		hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> +		spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	}
>  }

Since this function has only one caller, how about moving it into ufshcd.c?

Thanks,

Bart.

Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req

[Bart Van Assche]

On 7/1/21 8:23 AM, Bart Van Assche wrote:
> On 6/30/21 5:51 PM, Jaegeuk Kim wrote:
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index c98d540ac044..194755c9ddfe 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -1229,8 +1229,13 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
>>  static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
>>  					bool is_scsi_cmd)
>>  {
>> -	if (hba->vops && hba->vops->setup_xfer_req)
>> -		return hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
>> +	if (hba->vops && hba->vops->setup_xfer_req) {
>> +		unsigned long flags;
>> +
>> +		spin_lock_irqsave(hba->host->host_lock, flags);
>> +		hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
>> +		spin_unlock_irqrestore(hba->host->host_lock, flags);
>> +	}
>>  }
> 
> Since this function has only one caller, how about moving it into ufshcd.c?

(replying to my own email)

Since I just noticed that there are many other similar function
definitions in ufshcd.h, let's postpone moving these definitions until a
later time.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req

[Bart Van Assche]

On 6/30/21 5:51 PM, Jaegeuk Kim wrote:
> This patch adds a host_lock which existed before on ufshcd_vops_setup_xfer_req.
> 
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Fixes: a45f937110fa ("scsi: ufs: Optimize host lock on transfer requests send/compl paths")
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>   drivers/scsi/ufs/ufshcd.h | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c98d540ac044..194755c9ddfe 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1229,8 +1229,13 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
>   static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
>   					bool is_scsi_cmd)
>   {
> -	if (hba->vops && hba->vops->setup_xfer_req)
> -		return hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> +	if (hba->vops && hba->vops->setup_xfer_req) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(hba->host->host_lock, flags);
> +		hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> +		spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	}
>   }
>   
>   static inline void ufshcd_vops_setup_task_mgmt(struct ufs_hba *hba,

Can anyone help with reviewing this patch?

Thanks,

Bart.

Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req

[Bean Huo]

On Tue, 2021-07-13 at 12:06 -0700, Bart Van Assche wrote:
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -1229,8 +1229,13 @@ static inline int
> > ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
> >    static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba
> > *hba, int tag,
> >                                        bool is_scsi_cmd)
> >    {
> > -     if (hba->vops && hba->vops->setup_xfer_req)
> > -             return hba->vops->setup_xfer_req(hba, tag,
> > is_scsi_cmd);
> > +     if (hba->vops && hba->vops->setup_xfer_req) {
> > +             unsigned long flags;
> > +
> > +             spin_lock_irqsave(hba->host->host_lock, flags);
> > +             hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> > +             spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +     }
> >    }
> >    
> >    static inline void ufshcd_vops_setup_task_mgmt(struct ufs_hba
> > *hba,
> 
> 
> Can anyone help with reviewing this patch?
> 
> 
> 
> Thanks,
> 
> 
> 
Hi Bart,
This change only impacts on the Samsung exynos platform. and Can's
optimization patch is to optimise the host_lock,, and removed
host_lock, now add back in this function makes sense to me.
but I am thinking how about hba->host_sem?

Bean

Bean

> Bart.

Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req

[Bart Van Assche]

On 7/13/21 12:45 PM, Bean Huo wrote:
> This change only impacts on the Samsung exynos platform. and Can's
> optimization patch is to optimise the host_lock,, and removed
> host_lock, now add back in this function makes sense to me.
> but I am thinking how about hba->host_sem?

Hi Bean,

Calls of exynos_ufs_specify_nexus_t_xfer_req() must be serialized, hence 
Jaegeuks' patch. This function is called from the I/O submission path so 
performance matters. My understanding is that spinlocks have less 
overhead than semaphores. Hence the choice for a spinlock.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req

[Bean Huo]

On Wed, 2021-07-14 at 11:09 -0700, Bart Van Assche wrote:
> On 7/13/21 12:45 PM, Bean Huo wrote:
> 
> > This change only impacts on the Samsung exynos platform. and Can's
> > optimization patch is to optimise the host_lock,, and removed
> > host_lock, now add back in this function makes sense to me.
> > but I am thinking how about hba->host_sem?
> 
> 
> Hi Bean,
> 
> 
> 
> Calls of exynos_ufs_specify_nexus_t_xfer_req() must be serialized,
> hence 
> 
> Jaegeuks' patch. This function is called from the I/O submission path
> so 
> 
> performance matters. My understanding is that spinlocks have less 
> 
> overhead than semaphores. Hence the choice for a spinlock.
> 
> 
> 
> Thanks,

> 
Bart,

After adding spin_lock/unlock_irqsave()
in ufshcd_vops_setup_xfer_req(), there will be 4 times of call of
host_lock lock/unlock in ufshcd_send_command(). Reduce the code scope
of protection, but increase the number of calls to
spin_lock/unlock_irqsave().  Almost each sub-funciton
in ufshcd_send_command() calls spin_lock/unlock_irqsave(). why not
directly take spin_lock/unlock_irqsave() away from each sub-fun, and
increase the scope in ufshcd_send_command()?

Bean







> Bart