* [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req
@ 2021-07-01 0:51 Jaegeuk Kim
2021-07-01 15:23 ` Bart Van Assche
2021-07-13 19:06 ` Bart Van Assche
0 siblings, 2 replies; 7+ messages in thread
From: Jaegeuk Kim @ 2021-07-01 0:51 UTC (permalink / raw)
To: linux-kernel, linux-scsi
Cc: Jaegeuk Kim, Stanley Chu, Can Guo, Bean Huo, Bart Van Assche,
Asutosh Das
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req
2021-07-01 0:51 [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req Jaegeuk Kim
@ 2021-07-01 15:23 ` Bart Van Assche
2021-07-01 17:00 ` Bart Van Assche
2021-07-13 19:06 ` Bart Van Assche
1 sibling, 1 reply; 7+ messages in thread
From: Bart Van Assche @ 2021-07-01 15:23 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-scsi
Cc: Stanley Chu, Can Guo, Bean Huo, Asutosh Das
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req
2021-07-01 15:23 ` Bart Van Assche
@ 2021-07-01 17:00 ` Bart Van Assche
0 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2021-07-01 17:00 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-scsi
Cc: Stanley Chu, Can Guo, Bean Huo, Asutosh Das
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req
2021-07-01 0:51 [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req Jaegeuk Kim
2021-07-01 15:23 ` Bart Van Assche
@ 2021-07-13 19:06 ` Bart Van Assche
2021-07-13 19:45 ` Bean Huo
1 sibling, 1 reply; 7+ messages in thread
From: Bart Van Assche @ 2021-07-13 19:06 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-scsi
Cc: Stanley Chu, Can Guo, Bean Huo, Asutosh Das
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req
2021-07-13 19:06 ` Bart Van Assche
@ 2021-07-13 19:45 ` Bean Huo
2021-07-14 18:09 ` Bart Van Assche
0 siblings, 1 reply; 7+ messages in thread
From: Bean Huo @ 2021-07-13 19:45 UTC (permalink / raw)
To: Bart Van Assche, Jaegeuk Kim, linux-kernel, linux-scsi
Cc: Stanley Chu, Can Guo, Bean Huo, Asutosh Das
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req
2021-07-13 19:45 ` Bean Huo
@ 2021-07-14 18:09 ` Bart Van Assche
2021-07-14 21:22 ` Bean Huo
0 siblings, 1 reply; 7+ messages in thread
From: Bart Van Assche @ 2021-07-14 18:09 UTC (permalink / raw)
To: Bean Huo, Jaegeuk Kim, linux-kernel, linux-scsi
Cc: Stanley Chu, Can Guo, Bean Huo, Asutosh Das
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req
2021-07-14 18:09 ` Bart Van Assche
@ 2021-07-14 21:22 ` Bean Huo
0 siblings, 0 replies; 7+ messages in thread
From: Bean Huo @ 2021-07-14 21:22 UTC (permalink / raw)
To: Bart Van Assche, Jaegeuk Kim, linux-kernel, linux-scsi
Cc: Stanley Chu, Can Guo, Bean Huo, Asutosh Das
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-14 21:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-01 0:51 [PATCH] scsi: ufs: add missing host_lock in setup_xfer_req Jaegeuk Kim
2021-07-01 15:23 ` Bart Van Assche
2021-07-01 17:00 ` Bart Van Assche
2021-07-13 19:06 ` Bart Van Assche
2021-07-13 19:45 ` Bean Huo
2021-07-14 18:09 ` Bart Van Assche
2021-07-14 21:22 ` Bean Huo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).