| Message ID | 20231221-ufs-reset-ensure-effect-before-delay-v2-0-75af2a9bae51@redhat.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: Remove overzealous memory barriers | expand |
On Thu, Dec 21, 2023 at 12:25:20PM -0600, 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 > > Let's do that to ensure the bit hits the device. Because the mb()'s > purpose wasn't to add extra ordering (on top of the ordering guaranteed > by writel()/readl()), it can safely be removed. > > Fixes: 9c46b8676271 ("scsi: ufs-qcom: dump additional testbus registers") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > drivers/ufs/host/ufs-qcom.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 4c15c8a1d058..6df2ab3b6f23 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1332,6 +1332,9 @@ static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host) > ufshcd_rmwl(host->hba, UFS_REG_TEST_BUS_EN, > UFS_REG_TEST_BUS_EN, REG_UFS_CFG1); > ufshcd_rmwl(host->hba, TEST_BUS_EN, TEST_BUS_EN, REG_UFS_CFG1); > + > + /* dummy read to ensure this has been enabled prior to returning */ > + ufshcd_readl(host->hba, REG_UFS_CFG1); In this case, I do not see the necessity to do a read back itself since there is no delay afterwards nor any dependent operation in an altogether different domain. So removing the mb() should be sufficient. - Mani > } > > static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host) > @@ -1429,11 +1432,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.43.0 >
On Thu, Dec 21, 2023 at 12:25:21PM -0600, Andrew Halaney wrote: > Currently, the QUNIPRO_SEL bit is written to and then an mb() is used to > ensure that completes before continuing. > > 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 > > Let's do that to ensure the bit hits the device. Because the mb()'s > purpose wasn't to add extra ordering (on top of the ordering guaranteed > by writel()/readl()), it can safely be removed. > > Fixes: f06fcc7155dc ("scsi: ufs-qcom: add QUniPro hardware support and power optimizations") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > drivers/ufs/host/ufs-qcom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 6df2ab3b6f23..ab1ff7432d11 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -280,7 +280,7 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host) > ufshcd_rmwl(host->hba, QUNIPRO_G4_SEL, 0, REG_UFS_CFG0); > > /* make sure above configuration is applied before we return */ > - mb(); > + ufshcd_readl(host->hba, REG_UFS_CFG1); Same comment as patch 03/11. - Mani > } > > /* > > -- > 2.43.0 >
On Thu, Dec 21, 2023 at 12:25:25PM -0600, Andrew Halaney wrote: > Currently, interrupts are cleared and disabled prior to registering the > interrupt. An mb() is used to complete the clear/disable writes before > the interrupt is registered. > > mb() ensure that the write completes, but completion doesn't mean that > it isn't stored in a buffer somewhere. The recommendation for > ensuring these 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 these bits hit the device. Because the mb()'s > purpose wasn't to add extra ordering (on top of the ordering guaranteed > by writel()/readl()), it can safely be removed. > > Fixes: 199ef13cac7d ("scsi: ufs: avoid spurious UFS host controller interrupts") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Same comment as patch 07/11. 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 7bfb556e5b8e..bb603769b029 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -10568,7 +10568,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) > * Make sure that UFS interrupts are disabled and any pending interrupt > * status is cleared before registering UFS interrupt handler. > */ > - mb(); > + ufshcd_readl(hba, REG_INTERRUPT_ENABLE); > > /* IRQ registration */ > err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba); > > -- > 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 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,