LinuxLists.cc - [PATCH RFC v3 00/11] scsi: ufs: Remove overzealous memory barriers

[permalink] [raw]

Subject: [PATCH RFC v3 01/11] scsi: ufs: qcom: Perform read back after writing reset bit

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]>
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.43.0

2023-12-21 19:11:27

[permalink] [raw]

Subject: [PATCH RFC v3 02/11] scsi: ufs: qcom: Perform read back after writing REG_UFS_SYS1CLK_1US

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")
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 480787048e75..4c15c8a1d058 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.43.0

2023-12-21 19:11:49

[permalink] [raw]

Subject: [PATCH RFC v3 04/11] scsi: ufs: qcom: Perform read back after writing unipro mode

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 <[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 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);
}

/*

--
2.43.0

2023-12-21 19:12:07

[permalink] [raw]

Subject: [PATCH RFC v3 05/11] scsi: ufs: qcom: Perform read back after writing CGC enable

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.

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 ab1ff7432d11..3db19591d008 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -409,7 +409,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.43.0

2023-12-21 19:12:22

[permalink] [raw]

Subject: [PATCH RFC v3 06/11] scsi: ufs: cdns-pltfrm: Perform read back after writing HCLKDIV

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")
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.43.0

2023-12-21 19:12:45

[permalink] [raw]

Subject: [PATCH RFC v3 07/11] scsi: ufs: core: Perform read back after writing UTP_TASK_REQ_LIST_BASE_H

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")
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 d1e33328ff3f..7bfb556e5b8e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10351,7 +10351,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.43.0

2023-12-21 19:12:50

[permalink] [raw]

Subject: [PATCH RFC v3 03/11] scsi: ufs: qcom: Perform read back after writing testbus config

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 <[email protected]>
---
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);
}

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

2023-12-21 19:13:18

[permalink] [raw]

Subject: [PATCH RFC v3 08/11] scsi: ufs: core: Perform read back after disabling interrupts

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 <[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 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

2023-12-21 19:13:36

[permalink] [raw]

Subject: [PATCH RFC v3 09/11] scsi: ufs: core: Perform read back after disabling UIC_COMMAND_COMPL

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 <[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 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

2023-12-21 19:13:55

[permalink] [raw]

Subject: [PATCH RFC v3 10/11] scsi: ufs: core: Perform read back to commit doorbell

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

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: ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the doorbell")
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 75a03ee9a1ba..caebd589e08c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7050,7 +7050,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,

ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
/* Make sure that doorbell is committed immediately */
- wmb();
+ ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);

spin_unlock_irqrestore(host->host_lock, flags);

--
2.43.0

2023-12-21 19:14:01

[permalink] [raw]

Subject: [PATCH RFC v3 11/11] scsi: ufs: core: Perform read back before writing run/stop regs

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 <[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 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

--
2.43.0

2023-12-21 19:16:55

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH RFC v3 00/11] scsi: ufs: Remove overzealous memory barriers

On 21.12.2023 20:09, Andrew Halaney wrote:
> 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).
If I understand this correctly - and I'm no expert - it's probably good
practice to read it back in critical places, so that if the code around
it changes, the most crucial writes arrive when expected.

Konrad

2023-12-21 19:26:06

[permalink] [raw]

Subject: Re: [PATCH RFC v3 11/11] scsi: ufs: core: Perform read back before writing run/stop regs

On 12/21/23 11:09, 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.

Reviewed-by: Bart Van Assche <[email protected]>

2023-12-21 19:29:00

[permalink] [raw]

Subject: Re: [PATCH RFC v3 10/11] scsi: ufs: core: Perform read back to commit doorbell

On 12/21/23 11:09, Andrew Halaney wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 75a03ee9a1ba..caebd589e08c 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7050,7 +7050,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>
> ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
> /* Make sure that doorbell is committed immediately */
> - wmb();
> + ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
>
> spin_unlock_irqrestore(host->host_lock, flags);

There is a wait_for_completion_io_timeout() call later in this function and
it is safe to write to the REG_UTP_TASK_REQ_DOOR_BELL register from multiple
threads concurrently so I think the above wmb() call can be left out entirely.

Thanks,

Bart.

2023-12-21 19:29:47

[permalink] [raw]

Subject: Re: [PATCH RFC v3 09/11] scsi: ufs: core: Perform read back after disabling UIC_COMMAND_COMPL

On 12/21/23 11:09, 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 <[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 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);

Reviewed-by: Bart Van Assche <[email protected]>

2023-12-21 19:30:25

[permalink] [raw]

Subject: Re: [PATCH RFC v3 08/11] scsi: ufs: core: Perform read back after disabling interrupts

On 12/21/23 11:09, 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 <[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 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);

Reviewed-by: Bart Van Assche <[email protected]>

2023-12-21 19:30:44

[permalink] [raw]

Subject: Re: [PATCH RFC v3 07/11] scsi: ufs: core: Perform read back after writing UTP_TASK_REQ_LIST_BASE_H

On 12/21/23 11:09, Andrew Halaney wrote:
> 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")
> 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 d1e33328ff3f..7bfb556e5b8e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10351,7 +10351,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);

Reviewed-by: Bart Van Assche <[email protected]>

2023-12-22 08:19:20

[permalink] [raw]

Subject: Re: [PATCH RFC v3 01/11] scsi: ufs: qcom: Perform read back after writing reset bit

On 12/22/2023 3:09 AM, Andrew Halaney wrote:
> 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]>
> 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 */
>
Reviewed-by: Can Guo <[email protected]>

2023-12-22 08:20:56

[permalink] [raw]

Subject: Re: [PATCH RFC v3 03/11] scsi: ufs: qcom: Perform read back after writing testbus config

On 12/22/2023 3:09 AM, 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 <[email protected]>
> ---
> 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);
> }
>
> 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;
> }
>
Reviewed-by: Can Guo <[email protected]>

2023-12-22 08:20:56

[permalink] [raw]

Subject: Re: [PATCH RFC v3 02/11] scsi: ufs: qcom: Perform read back after writing REG_UFS_SYS1CLK_1US

On 12/22/2023 3:09 AM, 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")
> 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 480787048e75..4c15c8a1d058 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;
>
Reviewed-by: Can Guo <[email protected]>

2023-12-22 08:24:16

[permalink] [raw]

Subject: Re: [PATCH RFC v3 04/11] scsi: ufs: qcom: Perform read back after writing unipro mode

On 12/22/2023 3:09 AM, 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 <[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 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);

nit: please also add the dummy read comment line to avoid confusions.

> }
>
> /*
>
With above addressed,

Reviewed-by: Can Guo <[email protected]>

Thanks,
Can Guo.

2023-12-22 08:26:52

[permalink] [raw]

Subject: Re: [PATCH RFC v3 05/11] scsi: ufs: qcom: Perform read back after writing CGC enable

On 12/22/2023 3:09 AM, Andrew Halaney wrote:
> 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.
>
> 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 ab1ff7432d11..3db19591d008 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -409,7 +409,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,
>
Reviewed-by: Can Guo <[email protected]>

2023-12-22 08:26:53

[permalink] [raw]

Subject: Re: [PATCH RFC v3 07/11] scsi: ufs: core: Perform read back after writing UTP_TASK_REQ_LIST_BASE_H

On 12/22/2023 3:09 AM, Andrew Halaney wrote:
> 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")
> 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 d1e33328ff3f..7bfb556e5b8e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10351,7 +10351,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);
>
Reviewed-by: Can Guo <[email protected]>

2023-12-22 08:27:22

[permalink] [raw]

Subject: Re: [PATCH RFC v3 08/11] scsi: ufs: core: Perform read back after disabling interrupts

On 12/22/2023 3:09 AM, 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 <[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 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);
>
Reviewed-by: Can Guo <[email protected]>

2023-12-22 08:27:39

[permalink] [raw]

Subject: Re: [PATCH RFC v3 09/11] scsi: ufs: core: Perform read back after disabling UIC_COMMAND_COMPL

On 12/22/2023 3:09 AM, 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 <[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 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);
>
Reviewed-by: Can Guo <[email protected]>

2023-12-22 08:28:30

[permalink] [raw]

Subject: Re: [PATCH RFC v3 10/11] scsi: ufs: core: Perform read back to commit doorbell

On 12/22/2023 3:09 AM, 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
>
> 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: ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the doorbell")
> 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 75a03ee9a1ba..caebd589e08c 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7050,7 +7050,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>
> ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
> /* Make sure that doorbell is committed immediately */
> - wmb();
> + ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
>
> spin_unlock_irqrestore(host->host_lock, flags);
>
>
Reviewed-by: Can Guo <[email protected]>

2023-12-22 08:29:02