| Message ID | 20240329-ufs-reset-ensure-effect-before-delay-v5-0-181252004586@redhat.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: Remove overzealous memory barriers | expand |
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Fri, Mar 29, 2024 at 03:46:45PM -0500, Andrew Halaney wrote: > Currently, the testbus configuration is written and completed with an > mb(). > > mb() ensure that the write completes, but completion doesn't mean > that it isn't stored in a buffer somewhere. The recommendation for > ensuring this bit has taken effect on the device is to perform a read > back to force it to make it all the way to the device. This is > documented in device-io.rst and a talk by Will Deacon on this can > be seen over here: > > https://youtu.be/i6DayghhA8Q?si=MiyxB5cKJXSaoc01&t=1678 > > But, there's really no reason to even ensure completion before > continuing. The only requirement here is that this write is ordered to > this endpoint (which readl()/writel() guarantees already). For that > reason the mb() can be dropped altogether without anything forcing > completion. > > Fixes: 9c46b8676271 ("scsi: ufs-qcom: dump additional testbus registers") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/ufs/host/ufs-qcom.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 804dc8153e7b..649fada24345 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1445,11 +1445,6 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host) > (u32)host->testbus.select_minor << offset, > reg); > ufs_qcom_enable_test_bus(host); > - /* > - * Make sure the test bus configuration is > - * committed before returning. > - */ > - mb(); > > return 0; > } > > -- > 2.44.0 > >
On Fri, Mar 29, 2024 at 03:46:53PM -0500, Andrew Halaney wrote: > Currently a wmb() is used to ensure that writes to the > UTP_TASK_REQ_LIST_BASE* regs are completed prior to following writes to > the run/stop registers. > > wmb() ensure that the write completes, but completion doesn't mean that > it isn't stored in a buffer somewhere. The recommendation for > ensuring the bits have taken effect on the device is to perform a read > back to force it to make it all the way to the device. This is > documented in device-io.rst and a talk by Will Deacon on this can > be seen over here: > > https://youtu.be/i6DayghhA8Q?si=MiyxB5cKJXSaoc01&t=1678 > > But, none of that is necessary here. All of the writel()/readl()'s here > are to the same endpoint, so they will be ordered. There's no subsequent > delay() etc that requires it to have taken effect already, so no > readback is necessary here. > > For that reason just drop the wmb() altogether. > > Fixes: 897efe628d7e ("scsi: ufs: add missing memory barriers") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/ufs/core/ufshcd.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index a2f2941450fd..cf6a24e550f0 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4769,12 +4769,6 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba) > ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr), > REG_UTP_TASK_REQ_LIST_BASE_H); > > - /* > - * Make sure base address and interrupt setup are updated before > - * enabling the run/stop registers below. > - */ > - wmb(); > - > /* > * UCRDY, UTMRLDY and UTRLRDY bits must be 1 > */ > > -- > 2.44.0 > >
On Fri, 29 Mar 2024 15:46:42 -0500, Andrew Halaney wrote: > Please review with care as I'm not all that confident in this subject. > UFS has a lot of mb() variants used, most with comments saying "ensure this > takes effect before continuing". mb()'s aren't really the way to > guarantee that, a read back is the best method. > > Some of these though I think could go a step further and remove the mb() > variant without a read back. As far as I can tell there's no real reason > to ensure it takes effect in most cases (there's no delay() or anything > afterwards, and eventually another readl()/writel() happens which is by > definition ordered). Some of the patches in this series do that if I was > confident it was safe (or a reviewer pointed out prior that they thought > it was safe to do so). > > [...] Applied to 6.10/scsi-queue, thanks! [01/11] scsi: ufs: qcom: Perform read back after writing reset bit https://git.kernel.org/mkp/scsi/c/c4d28e06b0c9 [02/11] scsi: ufs: qcom: Perform read back after writing REG_UFS_SYS1CLK_1US https://git.kernel.org/mkp/scsi/c/a862fafa263a [03/11] scsi: ufs: qcom: Remove unnecessary mb() after writing testbus config https://git.kernel.org/mkp/scsi/c/95d26dda90df [04/11] scsi: ufs: qcom: Perform read back after writing unipro mode https://git.kernel.org/mkp/scsi/c/823150ecf04f [05/11] scsi: ufs: qcom: Perform read back after writing CGC enable https://git.kernel.org/mkp/scsi/c/d9488511b3ac [06/11] scsi: ufs: cdns-pltfrm: Perform read back after writing HCLKDIV https://git.kernel.org/mkp/scsi/c/b715c55daf59 [07/11] scsi: ufs: core: Perform read back after writing UTP_TASK_REQ_LIST_BASE_H https://git.kernel.org/mkp/scsi/c/408e28086f1c [08/11] scsi: ufs: core: Perform read back after disabling interrupts https://git.kernel.org/mkp/scsi/c/e4a628877119 [09/11] scsi: ufs: core: Perform read back after disabling UIC_COMMAND_COMPL https://git.kernel.org/mkp/scsi/c/4bf3855497b6 [10/11] scsi: ufs: core: Remove unnecessary wmb() after ringing doorbell https://git.kernel.org/mkp/scsi/c/d3fb9a24a602 [11/11] scsi: ufs: core: Remove unnecessary wmb() prior to writing run/stop regs https://git.kernel.org/mkp/scsi/c/356a8ce7cd50
Please review with care as I'm not all that confident in this subject. UFS has a lot of mb() variants used, most with comments saying "ensure this takes effect before continuing". mb()'s aren't really the way to guarantee that, a read back is the best method. Some of these though I think could go a step further and remove the mb() variant without a read back. As far as I can tell there's no real reason to ensure it takes effect in most cases (there's no delay() or anything afterwards, and eventually another readl()/writel() happens which is by definition ordered). Some of the patches in this series do that if I was confident it was safe (or a reviewer pointed out prior that they thought it was safe to do so). Thanks in advance for the help, Andrew To: Andy Gross <agross@kernel.org> To: Bjorn Andersson <andersson@kernel.org> To: Konrad Dybcio <konrad.dybcio@linaro.org> To: Manivannan Sadhasivam <mani@kernel.org> To: James E.J. Bottomley <jejb@linux.ibm.com> To: Martin K. Petersen <martin.petersen@oracle.com> To: Hannes Reinecke <hare@suse.de> To: Janek Kotas <jank@cadence.com> To: Alim Akhtar <alim.akhtar@samsung.com> To: Avri Altman <avri.altman@wdc.com> To: Bart Van Assche <bvanassche@acm.org> To: Can Guo <quic_cang@quicinc.com> To: Anjana Hari <quic_ahari@quicinc.com> Cc: Will Deacon <will@kernel.org> Cc: linux-arm-msm@vger.kernel.org Cc: linux-scsi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Changes in v5: - Rebased on top of next-20240327 - v4 changes need review, so please pay attention to that bit still :) - Link to v4: https://lore.kernel.org/r/20240122-ufs-reset-ensure-effect-before-delay-v4-0-6c48432151cc@redhat.com Changes in v4: - Collected Reviewed-by tags - Changed patches 3, 4, 10, and 11 to drop the read back && mb(): - Please note all of those patches got reviewed-by tags by either Can, Mani, or Bart, but one of the three pointed out that they thought it could be dropped altogether (some of Mani's comments are on my foobar'ed v2). After some consideration I agree. Therefore I'd appreciate re-review on those patches by you three to make sure that's appropriate - Link to v3: https://lore.kernel.org/r/20231221-ufs-reset-ensure-effect-before-delay-v3-0-2195a1b66d2e@redhat.com Changes in v3: - Nothing changed, I just failed to send with b4 (resulting in 2 half sent v2 series on list) - Link to v2: https://lore.kernel.org/r/pnwsdz3i2liivjxvtfwq6tijotgh5adyqipjjb5wdvo4jpu7yv@j6fkshm5ipue Changes in v2: - Added review tags for original patch - Added new patches to address all other memory barriers used - Link to v1: https://lore.kernel.org/r/20231208-ufs-reset-ensure-effect-before-delay-v1-1-8a0f82d7a09e@redhat.com --- Andrew Halaney (11): scsi: ufs: qcom: Perform read back after writing reset bit scsi: ufs: qcom: Perform read back after writing REG_UFS_SYS1CLK_1US scsi: ufs: qcom: Remove unnecessary mb() after writing testbus config scsi: ufs: qcom: Perform read back after writing unipro mode scsi: ufs: qcom: Perform read back after writing CGC enable scsi: ufs: cdns-pltfrm: Perform read back after writing HCLKDIV scsi: ufs: core: Perform read back after writing UTP_TASK_REQ_LIST_BASE_H scsi: ufs: core: Perform read back after disabling interrupts scsi: ufs: core: Perform read back after disabling UIC_COMMAND_COMPL scsi: ufs: core: Remove unnecessary wmb() after ringing doorbell scsi: ufs: core: Remove unnecessary wmb() prior to writing run/stop regs drivers/ufs/core/ufshcd.c | 15 +++------------ drivers/ufs/host/cdns-pltfrm.c | 2 +- drivers/ufs/host/ufs-qcom.c | 12 ++---------- drivers/ufs/host/ufs-qcom.h | 12 ++++++------ 4 files changed, 12 insertions(+), 29 deletions(-) --- base-commit: 26074e1be23143b2388cacb36166766c235feb7c change-id: 20231208-ufs-reset-ensure-effect-before-delay-6e06899d5419 Best regards,