| Message ID | 20231221-ufs-reset-ensure-effect-before-delay-v3-0-2195a1b66d2e@redhat.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: Remove overzealous memory barriers | expand |
On 12/22/2023 3:09 AM, 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 > > Let's do that to ensure the bits hit the device. Because the wmb()'s > purpose wasn't to add extra ordering (on top of the ordering guaranteed > by writel()/readl()), it can safely be removed. > > Fixes: 897efe628d7e ("scsi: ufs: add missing memory barriers") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > drivers/ufs/core/ufshcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index caebd589e08c..7c1975a1181f 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4726,7 +4726,7 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba) > * Make sure base address and interrupt setup are updated before > * enabling the run/stop registers below. > */ > - wmb(); > + ufshcd_readl(hba, REG_UTP_TASK_REQ_LIST_BASE_H); > > /* > * UCRDY, UTMRLDY and UTRLRDY bits must be 1 > Reviewed-by: Can Guo <quic_cang@quicinc.com>
On Thu, Dec 21, 2023 at 01:09:55PM -0600, Andrew Halaney wrote: > Currently, the UIC_COMMAND_COMPL interrupt is disabled and a wmb() is > used to complete the register write before any following writes. > > wmb() ensures the writes complete in that order, 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 > > Let's do that to ensure the bit hits the device. Because the wmb()'s > purpose wasn't to add extra ordering (on top of the ordering guaranteed > by writel()/readl()), it can safely be removed. > > Fixes: d75f7fe495cf ("scsi: ufs: reduce the interrupts for power mode change requests") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/ufs/core/ufshcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index bb603769b029..75a03ee9a1ba 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4240,7 +4240,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd) > * Make sure UIC command completion interrupt is disabled before > * issuing UIC command. > */ > - wmb(); > + ufshcd_readl(hba, REG_INTERRUPT_ENABLE); > reenable_intr = true; > } > spin_unlock_irqrestore(hba->host->host_lock, flags); > > -- > 2.43.0 >
On Thu, Dec 21, 2023 at 01:09:57PM -0600, 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 > > Let's do that to ensure the bits hit the device. Because the wmb()'s > purpose wasn't to add extra ordering (on top of the ordering guaranteed > by writel()/readl()), it can safely be removed. > > Fixes: 897efe628d7e ("scsi: ufs: add missing memory barriers") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > drivers/ufs/core/ufshcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index caebd589e08c..7c1975a1181f 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4726,7 +4726,7 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba) > * Make sure base address and interrupt setup are updated before > * enabling the run/stop registers below. > */ > - wmb(); > + ufshcd_readl(hba, REG_UTP_TASK_REQ_LIST_BASE_H); I don't think the readback is really needed here. Because, the dependency is with UTP registers and both should be in the same domain. So there is no way the following write can happen before prior UTP write completion. - Mani > > /* > * UCRDY, UTMRLDY and UTRLRDY bits must be 1 > > -- > 2.43.0 >
This is an RFC because I'm not all the confident in this topic. 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). In this current series I don't do that as I wasn't totally convinced, but it should be considered when reviewing. Hopefully this series gets enough interest where we can confidently merge the final result after review helps improve it. I'll be travelling a good bit the next 2ish weeks, so expect little response until my return. Thanks in advance for the help, Andrew Signed-off-by: Andrew Halaney <ahalaney@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: Perform read back 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: Perform read back to commit doorbell scsi: ufs: core: Perform read back before writing run/stop regs drivers/ufs/core/ufshcd.c | 10 +++++----- drivers/ufs/host/cdns-pltfrm.c | 2 +- drivers/ufs/host/ufs-qcom.c | 14 ++++++-------- drivers/ufs/host/ufs-qcom.h | 12 ++++++------ 4 files changed, 18 insertions(+), 20 deletions(-) --- base-commit: 20d857259d7d10cd0d5e8b60608455986167cfad change-id: 20231208-ufs-reset-ensure-effect-before-delay-6e06899d5419 Best regards,