| Message ID | 20210701005117.3846179-1-jaegeuk@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: ufs: add missing host_lock in setup_xfer_req | expand |
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.
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>
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.
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.
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.
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
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,
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(-)