| Message ID | 20210701211224.17070-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | UFS patches for kernel v5.15 | expand |
On 7/1/2021 2:12 PM, Bart Van Assche wrote: > From arch/arm/include/asm/io.h > > #define __iowmb() wmb() > [ ... ] > #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); }) > > From Documentation/memory-barriers.txt: "Note that, when using writel(), a > prior wmb() is not needed to guarantee that the cache coherent memory > writes have completed before writing to the MMIO region." > > In other words, calling wmb() before writel() is not necessary. Hence > remove the wmb() calls that precede a writel() call. Remove the wmb() > calls that precede a ufshcd_send_command() call since the latter function > uses writel(). Remove the wmb() call from ufshcd_wait_for_dev_cmd() > since the following chain of events guarantees that the CPU will see > up-to-date LRB values: > * UFS controller writes to host memory. > * UFS controller posts completion interrupt after the memory writes from > the previous step are visible to the CPU. > * complete(hba->dev_cmd.complete) is called from the UFS interrupt handler. > * The wait_for_completion(hba->dev_cmd.complete) call in > ufshcd_wait_for_dev_cmd() returns. > > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Stanley Chu <stanley.chu@mediatek.com> > Cc: Can Guo <cang@codeaurora.org> > Cc: Asutosh Das <asutoshd@codeaurora.org> > Cc: Avri Altman <avri.altman@wdc.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ufs/ufshcd.c | 11 ----------- > 1 file changed, 11 deletions(-) > Hi Bart, Did you verify this change? I think we've seen OCS errors which were fixed with the wmb() in queuecommand. > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 7091798e6245..25ab603acad1 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -2768,8 +2768,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) > ufshcd_release(hba); > goto out; > } > - /* Make sure descriptors are ready before ringing the doorbell */ > - wmb(); > > ufshcd_send_command(hba, tag); > out: > @@ -2879,8 +2877,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba, > time_left = wait_for_completion_timeout(hba->dev_cmd.complete, > msecs_to_jiffies(max_timeout)); > > - /* Make sure descriptors are ready before ringing the doorbell */ > - wmb(); > spin_lock_irqsave(hba->host->host_lock, flags); > hba->dev_cmd.complete = NULL; > if (likely(time_left)) { > @@ -2955,8 +2951,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba, > hba->dev_cmd.complete = &wait; > > ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr); > - /* Make sure descriptors are ready before ringing the doorbell */ > - wmb(); > > ufshcd_send_command(hba, tag); > err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout); > @@ -6514,9 +6508,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, > /* send command to the controller */ > __set_bit(task_tag, &hba->outstanding_tasks); > > - /* Make sure descriptors are ready before ringing the task doorbell */ > - wmb(); > - > ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL); > /* Make sure that doorbell is committed immediately */ > wmb(); > @@ -6687,8 +6678,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba, > hba->dev_cmd.complete = &wait; > > ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr); > - /* Make sure descriptors are ready before ringing the doorbell */ > - wmb(); > > ufshcd_send_command(hba, tag); > /* >
> The unit of the scsi_execute() timeout parameter is 1/HZ seconds instead of > one second, just like the timeouts used in the block layer. Fix the > documentation header above the definition of the scsi_execute() macro. > > Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> > Cc: Hannes Reinecke <hare@suse.de> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: John Garry <john.garry@huawei.com> > Fixes: "[SCSI] use scatter lists for all block pc requests and simplify hw > handlers" # v2.6.16.28 > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/scsi/scsi_lib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 7184f93dfe15..4b56e06faa5e 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -194,7 +194,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int > reason) > * @bufflen: len of buffer > * @sense: optional sense buffer > * @sshdr: optional decoded sense header > - * @timeout: request timeout in seconds > + * @timeout: request timeout in HZ > * @retries: number of times to retry request > * @flags: flags for ->cmd_flags > * @rq_flags: flags for ->rq_flags
> > This patch slightly reduces the UFS driver size if built with power > management support disabled. > > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Stanley Chu <stanley.chu@mediatek.com> > Cc: Can Guo <cang@codeaurora.org> > Cc: Asutosh Das <asutoshd@codeaurora.org> > Cc: Avri Altman <avri.altman@wdc.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ufs/ufshcd.c | 8 ++++++++ > drivers/scsi/ufs/ufshcd.h | 4 ++++ > 2 files changed, 12 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index a34aa6d486c7..37302a8b3937 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -8736,6 +8736,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba > *hba) > usleep_range(5000, 5100); > } > > +#ifdef CONFIG_PM Maybe move this few lines up to include ufshcd_vreg_set_lpm as well? Are there any ufs platforms that doesn't use pm management? Automotive platforms maybe? Thanks, Avri > static int ufshcd_vreg_set_hpm(struct ufs_hba *hba) > { > int ret = 0; > @@ -8763,6 +8764,7 @@ static int ufshcd_vreg_set_hpm(struct ufs_hba > *hba) > out: > return ret; > } > +#endif /* CONFIG_PM */ > > static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba) > { > @@ -9165,6 +9167,7 @@ static int ufshcd_suspend(struct ufs_hba *hba) > return ret; > } > > +#ifdef CONFIG_PM > /** > * ufshcd_resume - helper function for resume operations > * @hba: per adapter instance > @@ -9202,7 +9205,9 @@ static int ufshcd_resume(struct ufs_hba *hba) > ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret); > return ret; > } > +#endif /* CONFIG_PM */ > > +#ifdef CONFIG_PM_SLEEP > /** > * ufshcd_system_suspend - system suspend callback > * @dev: Device associated with the UFS controller. > @@ -9258,7 +9263,9 @@ int ufshcd_system_resume(struct device *dev) > return ret; > } > EXPORT_SYMBOL(ufshcd_system_resume); > +#endif /* CONFIG_PM_SLEEP */ > > +#ifdef CONFIG_PM > /** > * ufshcd_runtime_suspend - runtime suspend callback > * @dev: Device associated with the UFS controller. > @@ -9306,6 +9313,7 @@ int ufshcd_runtime_resume(struct device *dev) > return ret; > } > EXPORT_SYMBOL(ufshcd_runtime_resume); > +#endif /* CONFIG_PM */ > > /** > * ufshcd_shutdown - shutdown routine > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index dc75426c609f..79f6c261dfff 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -1009,10 +1009,14 @@ static inline u8 > ufshcd_wb_get_query_index(struct ufs_hba *hba) > return 0; > } > > +#ifdef CONFIG_PM > extern int ufshcd_runtime_suspend(struct device *dev); > extern int ufshcd_runtime_resume(struct device *dev); > +#endif > +#ifdef CONFIG_PM_SLEEP > extern int ufshcd_system_suspend(struct device *dev); > extern int ufshcd_system_resume(struct device *dev); > +#endif > extern int ufshcd_shutdown(struct ufs_hba *hba); > extern int ufshcd_dme_configure_adapt(struct ufs_hba *hba, > int agreed_gear,
> From Documentation/scheduler/completion.rst: "When a completion is > declared > as a local variable within a function, then the initialization should > always use DECLARE_COMPLETION_ONSTACK() explicitly, not just to make > lockdep happy, but also to make it clear that limited scope had been > considered and is intentional." > > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Stanley Chu <stanley.chu@mediatek.com> > Cc: Can Guo <cang@codeaurora.org> > Cc: Asutosh Das <asutoshd@codeaurora.org> > Cc: Avri Altman <avri.altman@wdc.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Instead of documenting the locking requirements of the UIC code as > comments, > use lockdep_assert_held() such that lockdep verifies the lockdep > requirements at runtime if lockdep is enabled. > > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Stanley Chu <stanley.chu@mediatek.com> > Cc: Can Guo <cang@codeaurora.org> > Cc: Asutosh Das <asutoshd@codeaurora.org> > Cc: Avri Altman <avri.altman@wdc.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Avri Altman <avri.altman@wdc.com>
On 7/4/21 11:33 PM, Avri Altman wrote: >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index a34aa6d486c7..37302a8b3937 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -8736,6 +8736,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba >> *hba) >> usleep_range(5000, 5100); >> } >> >> +#ifdef CONFIG_PM > Maybe move this few lines up to include ufshcd_vreg_set_lpm as well? The following call chain requires that ufshcd_vreg_set_lpm() is always available: ufshcd_shutdown() -> ufshcd_suspend() -> ufshcd_vreg_set_lpm(). > Are there any ufs platforms that doesn't use pm management? > Automotive platforms maybe? I'm not sure whether there is any platform on which UFS PM is disabled. My purpose with this patch is to document which code is not used if power management is disabled. Bart.
> > This patch slightly reduces the UFS driver size if built with power > management support disabled. > > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Stanley Chu <stanley.chu@mediatek.com> > Cc: Can Guo <cang@codeaurora.org> > Cc: Asutosh Das <asutoshd@codeaurora.org> > Cc: Avri Altman <avri.altman@wdc.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Avri Altman <avri.altman@wdc.com>
Hi Martin, Please consider the patches in this series for kernel v5.15. As one can see this patch series includes one ATA patch, four SCSI core patches and 16 UFS patches. Thanks, Bart. Bart Van Assche (21): Fix the documentation of the scsi_execute() time parameter libsas: Only abort commands from inside the error handler Clear host_eh_scheduled from inside the SCSI error handler libsas: Stop clearing host_eh_scheduled from the error handler ata: Stop clearing host_eh_scheduled from error handlers ufs: Reduce power management code duplication ufs: Only include power management code if necessary ufs: Rename the second ufshcd_probe_hba() argument ufs: Use DECLARE_COMPLETION_ONSTACK() where appropriate ufs: Remove ufshcd_valid_tag() ufs: Verify UIC locking requirements at runtime ufs: Improve static type checking for the host controller state ufs: Remove several wmb() calls ufs: Inline ufshcd_outstanding_req_clear() ufs: Rename __ufshcd_transfer_req_compl() ufs: Fix the SCSI abort handler ufs: Fix a race in the completion path ufs: Use the doorbell register instead of the UTRLCNR register ufs: Request sense data asynchronously ufs: Synchronize SCSI and UFS error handling ufs: Retry aborted SCSI commands instead of completing these successfully drivers/ata/libata-core.c | 2 - drivers/ata/libata-eh.c | 30 +- drivers/scsi/libsas/sas_scsi_host.c | 9 +- drivers/scsi/scsi_error.c | 7 + drivers/scsi/scsi_lib.c | 2 +- drivers/scsi/ufs/cdns-pltfrm.c | 7 +- drivers/scsi/ufs/tc-dwc-g210-pci.c | 32 +- drivers/scsi/ufs/tc-dwc-g210-pltfrm.c | 7 +- drivers/scsi/ufs/ufs-exynos.c | 7 +- drivers/scsi/ufs/ufs-hisi.c | 7 +- drivers/scsi/ufs/ufs-mediatek.c | 7 +- drivers/scsi/ufs/ufs-qcom.c | 7 +- drivers/scsi/ufs/ufshcd-pci.c | 48 +-- drivers/scsi/ufs/ufshcd-pltfrm.c | 47 --- drivers/scsi/ufs/ufshcd-pltfrm.h | 18 - drivers/scsi/ufs/ufshcd.c | 467 +++++++++++--------------- drivers/scsi/ufs/ufshcd.h | 48 ++- include/linux/libata.h | 1 - include/scsi/libsas.h | 1 + 19 files changed, 271 insertions(+), 483 deletions(-)