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 <[email protected]>
To: Bjorn Andersson <[email protected]>
To: Konrad Dybcio <[email protected]>
To: Manivannan Sadhasivam <[email protected]>
To: James E.J. Bottomley <[email protected]>
To: Martin K. Petersen <[email protected]>
To: Hannes Reinecke <[email protected]>
To: Janek Kotas <[email protected]>
To: Alim Akhtar <[email protected]>
To: Avri Altman <[email protected]>
To: Bart Van Assche <[email protected]>
To: Can Guo <[email protected]>
To: Anjana Hari <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Andrew Halaney <[email protected]>
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,
--
Andrew Halaney <[email protected]>
Currently, the reset bit for the UFS provided reset controller (used by
its phy) is written to, and then a mb() happens to try and ensure that
hit the device. Immediately afterwards a usleep_range() occurs.
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. By doing so and
guaranteeing the ordering against the immediately following
usleep_range(), the mb() can safely be removed.
Fixes: 81c0fc51b7a7 ("ufs-qcom: add support for Qualcomm Technologies Inc platforms")
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Can Guo <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
drivers/ufs/host/ufs-qcom.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 9dd9a391ebb7..b9de170983c9 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -151,10 +151,10 @@ static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
/*
- * Make sure assertion of ufs phy reset is written to
- * register before returning
+ * Dummy read to ensure the write takes effect before doing any sort
+ * of delay
*/
- mb();
+ ufshcd_readl(hba, REG_UFS_CFG1);
}
static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
@@ -162,10 +162,10 @@ static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, 0, REG_UFS_CFG1);
/*
- * Make sure de-assertion of ufs phy reset is written to
- * register before returning
+ * Dummy read to ensure the write takes effect before doing any sort
+ * of delay
*/
- mb();
+ ufshcd_readl(hba, REG_UFS_CFG1);
}
/* Host controller hardware version: major.minor.step */
--
2.44.0
Currently after writing to REG_UFS_SYS1CLK_1US a mb() is used to ensure
that write has gone through to the device.
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")
Reviewed-by: Can Guo <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
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 06859e17b67b..804dc8153e7b 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -501,7 +501,7 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
* make sure above write gets applied before we return from
* this function.
*/
- mb();
+ ufshcd_readl(hba, REG_UFS_SYS1CLK_1US);
}
return 0;
--
2.44.0
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 <[email protected]>
---
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
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
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: f06fcc7155dc ("scsi: ufs-qcom: add QUniPro hardware support and power optimizations")
Signed-off-by: Andrew Halaney <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 649fada24345..66a6c95f5d72 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -278,9 +278,6 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
if (host->hw_ver.major >= 0x05)
ufshcd_rmwl(host->hba, QUNIPRO_G4_SEL, 0, REG_UFS_CFG0);
-
- /* make sure above configuration is applied before we return */
- mb();
}
/*
--
2.44.0
Currently, the CGC enable bit is written 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.
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Can Guo <[email protected]>
Fixes: 81c0fc51b7a7 ("ufs-qcom: add support for Qualcomm Technologies Inc platforms")
Signed-off-by: Andrew Halaney <[email protected]>
---
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 66a6c95f5d72..1439c1df0481 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -406,7 +406,7 @@ static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba)
REG_UFS_CFG2);
/* Ensure that HW clock gating is enabled before next operations */
- mb();
+ ufshcd_readl(hba, REG_UFS_CFG2);
}
static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
--
2.44.0
Currently, HCLKDIV is written to and then 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: d90996dae8e4 ("scsi: ufs: Add UFS platform driver for Cadence UFS")
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
drivers/ufs/host/cdns-pltfrm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ufs/host/cdns-pltfrm.c b/drivers/ufs/host/cdns-pltfrm.c
index bb30267da471..66811d8d1929 100644
--- a/drivers/ufs/host/cdns-pltfrm.c
+++ b/drivers/ufs/host/cdns-pltfrm.c
@@ -136,7 +136,7 @@ static int cdns_ufs_set_hclkdiv(struct ufs_hba *hba)
* Make sure the register was updated,
* UniPro layer will not work with an incorrect value.
*/
- mb();
+ ufshcd_readl(hba, CDNS_UFS_REG_HCLKDIV);
return 0;
}
--
2.44.0
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")
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Reviewed-by: Can Guo <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
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 a89887878d98..268fcfebd7bd 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10616,7 +10616,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.44.0
Currently, the UTP_TASK_REQ_LIST_BASE_L/UTP_TASK_REQ_LIST_BASE_H regs
are written to and then 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 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 the 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: 88441a8d355d ("scsi: ufs: core: Add hibernation callbacks")
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Reviewed-by: Can Guo <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
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 e30fd125988d..a89887878d98 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10395,7 +10395,7 @@ int ufshcd_system_restore(struct device *dev)
* are updated with the latest queue addresses. Only after
* updating these addresses, we can queue the new commands.
*/
- mb();
+ ufshcd_readl(hba, REG_UTP_TASK_REQ_LIST_BASE_H);
/* Resuming from hibernate, assume that link was OFF */
ufshcd_set_link_off(hba);
--
2.44.0
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")
Reviewed-by: Bart Van Assche <[email protected]>
Reviewed-by: Can Guo <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
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 268fcfebd7bd..dfa4f827766a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4287,7 +4287,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.44.0
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 <[email protected]>
---
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
Currently, the doorbell is written to and a wmb() is used to commit it
immediately.
wmb() ensures that the write completes before following writes occur,
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, completion and taking effect aren't necessary to guarantee here.
There's already other examples of the doorbell being rung that don't do
this. The writel() of the doorbell guarantees prior writes by this
thread (to the request being setup for example) complete prior to the
ringing of the doorbell, and the following
wait_for_completion_io_timeout() doesn't require any special memory
barriers either.
With that in mind, just remove the wmb() altogether here.
Fixes: ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the doorbell")
Signed-off-by: Andrew Halaney <[email protected]>
---
drivers/ufs/core/ufshcd.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index dfa4f827766a..a2f2941450fd 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7090,10 +7090,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
/* send command to the controller */
__set_bit(task_tag, &hba->outstanding_tasks);
-
ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
- /* Make sure that doorbell is committed immediately */
- wmb();
spin_unlock_irqrestore(host->host_lock, flags);
--
2.44.0
Reviewed-by: Bart Van Assche <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
On Fri, Mar 29, 2024 at 03:46:44PM -0500, Andrew Halaney wrote:
> Currently after writing to REG_UFS_SYS1CLK_1US a mb() is used to ensure
> that write has gone through to the device.
>
> 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")
> Reviewed-by: Can Guo <[email protected]>
> Signed-off-by: Andrew Halaney <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
> 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 06859e17b67b..804dc8153e7b 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -501,7 +501,7 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
> * make sure above write gets applied before we return from
> * this function.
> */
> - mb();
> + ufshcd_readl(hba, REG_UFS_SYS1CLK_1US);
> }
>
> return 0;
>
> --
> 2.44.0
>
>
--
மணிவண்ணன் சதாசிவம்
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 <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- 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:46PM -0500, 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
>
> 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: f06fcc7155dc ("scsi: ufs-qcom: add QUniPro hardware support and power optimizations")
> Signed-off-by: Andrew Halaney <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
> drivers/ufs/host/ufs-qcom.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 649fada24345..66a6c95f5d72 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -278,9 +278,6 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
>
> if (host->hw_ver.major >= 0x05)
> ufshcd_rmwl(host->hba, QUNIPRO_G4_SEL, 0, REG_UFS_CFG0);
> -
> - /* make sure above configuration is applied before we return */
> - mb();
> }
>
> /*
>
> --
> 2.44.0
>
>
--
மணிவண்ணன் சதாசிவம்
On Fri, Mar 29, 2024 at 03:46:52PM -0500, Andrew Halaney wrote:
> Currently, the doorbell is written to and a wmb() is used to commit it
> immediately.
>
> wmb() ensures that the write completes before following writes occur,
> 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, completion and taking effect aren't necessary to guarantee here.
>
> There's already other examples of the doorbell being rung that don't do
> this. The writel() of the doorbell guarantees prior writes by this
> thread (to the request being setup for example) complete prior to the
> ringing of the doorbell, and the following
> wait_for_completion_io_timeout() doesn't require any special memory
> barriers either.
>
> With that in mind, just remove the wmb() altogether here.
>
> Fixes: ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the doorbell")
> Signed-off-by: Andrew Halaney <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
> drivers/ufs/core/ufshcd.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index dfa4f827766a..a2f2941450fd 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7090,10 +7090,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>
> /* send command to the controller */
> __set_bit(task_tag, &hba->outstanding_tasks);
> -
> ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
> - /* Make sure that doorbell is committed immediately */
> - wmb();
>
> spin_unlock_irqrestore(host->host_lock, flags);
>
>
> --
> 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 <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- 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
>
>
--
மணிவண்ணன் சதாசிவம்
Andrew,
> 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.
Applied to 6.10/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering
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
--
Martin K. Petersen Oracle Linux Engineering