All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition
@ 2026-09-14  8:49 peter.wang
  2026-09-14  8:49 ` [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure peter.wang
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: peter.wang @ 2026-09-14  8:49 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar, bvanassche,
	santosh.sy, h.vinayak
  Cc: wsd_upstream, linux-mediatek, peter.wang, chun-hung.wu,
	alice.chao, cc.chou, chaotian.jing, tun-yu.yu, naomi.chu, ed.tsai

From: Peter Wang <peter.wang@mediatek.com>

Patch 1 fixes a bug where POST_CHANGE was skipped on a failed gear
switch, leaving the host in an inconsistent state if resources acquired
in PRE_CHANGE were never released.

Patch 2 fixes a race between the sysfs AHIT update path and the
MediaTek host driver's power mode change path. A mutex and depth
counter are introduced to serialize AHIT register access and ensure
AHIT is correctly restored after the last disabler completes.

The two patches are related: patch 1 guarantees POST_CHANGE is always
called, ensuring the depth counter introduced in patch 2 is always
properly decremented.

Peter Wang (2):
  ufs: always notify POST_CHANGE even on gear switch failure
  ufs: serialize AHIT register access between sysfs and host driver

 drivers/ufs/core/ufshcd.c       | 22 +++++++++++++++++++---
 drivers/ufs/host/ufs-exynos.c   |  4 +++-
 drivers/ufs/host/ufs-mediatek.c | 16 ++++++++++++----
 drivers/ufs/host/ufshcd-pci.c   |  3 ++-
 include/ufs/ufshcd.h            |  8 ++++++++
 5 files changed, 44 insertions(+), 9 deletions(-)

-- 
2.45.2



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure
  2026-09-14  8:49 [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition peter.wang
@ 2026-09-14  8:49 ` peter.wang
  2026-09-14  9:02   ` sashiko-bot
  2026-09-14  8:49 ` [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver peter.wang
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: peter.wang @ 2026-09-14  8:49 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar, bvanassche,
	santosh.sy, h.vinayak
  Cc: wsd_upstream, linux-mediatek, peter.wang, chun-hung.wu,
	alice.chao, cc.chou, chaotian.jing, tun-yu.yu, naomi.chu, ed.tsai

From: Peter Wang <peter.wang@mediatek.com>

Previously, POST_CHANGE was only notified on a successful gear switch.
This could leave the host in an inconsistent state if resources were
acquired in PRE_CHANGE (e.g. disabling auto-hibern8) but never
released due to a missing POST_CHANGE.

Fix this by always calling POST_CHANGE regardless of the gear switch
result. On failure, pass dev_req_params = NULL to signal that there is
nothing to apply; hosts that receive NULL simply skip their post work.

Update the affected vendor drivers (ufs-exynos and ufshcd-pci) to
guard their POST_CHANGE logic with a NULL check so they correctly
handle the failure case.

Fixes: f5ca8d0c7a63 ("scsi: ufs: host: mediatek: Disable auto-hibern8 during power mode changes")
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c     | 8 ++++++--
 drivers/ufs/host/ufs-exynos.c | 4 +++-
 drivers/ufs/host/ufshcd-pci.c | 3 ++-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2ba244cf40ac..39a4bdafc1af 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4979,8 +4979,12 @@ int ufshcd_change_power_mode(struct ufs_hba *hba,
 
 	ret = ufshcd_dme_change_power_mode(hba, pwr_mode, pmc_policy);
 
-	if (!ret)
-		ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, pwr_mode);
+	/*
+	 * Always notify POST_CHANGE to pair with PRE_CHANGE, even on failure.
+	 * On failure, pass dev_req_params = NULL to indicate nothing to apply.
+	 * POST_CHANGE return value is advisory and does not mask @ret.
+	 */
+	ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, ret ? NULL : pwr_mode);
 
 	return ret;
 }
diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index b2f65c465525..3bcbc9ebbb28 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1782,7 +1782,9 @@ static int exynos_ufs_pwr_change_notify(struct ufs_hba *hba,
 		ret = exynos_ufs_pre_pwr_mode(hba, dev_req_params);
 		break;
 	case POST_CHANGE:
-		ret = exynos_ufs_post_pwr_mode(hba, dev_req_params);
+		/* NULL when the gear switch failed: nothing to apply */
+		if (dev_req_params)
+			ret = exynos_ufs_post_pwr_mode(hba, dev_req_params);
 		break;
 	}
 
diff --git a/drivers/ufs/host/ufshcd-pci.c b/drivers/ufs/host/ufshcd-pci.c
index 21bb11c724be..668b57403e62 100644
--- a/drivers/ufs/host/ufshcd-pci.c
+++ b/drivers/ufs/host/ufshcd-pci.c
@@ -166,7 +166,8 @@ static int ufs_intel_lkf_pwr_change_notify(struct ufs_hba *hba,
 			ufs_intel_set_lanes(hba, 2);
 		break;
 	case POST_CHANGE:
-		if (ufshcd_is_hs_mode(dev_req_params)) {
+		/* NULL when the gear switch failed: nothing to apply */
+		if (dev_req_params && ufshcd_is_hs_mode(dev_req_params)) {
 			u32 peer_granularity;
 
 			usleep_range(1000, 1250);
-- 
2.45.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver
  2026-09-14  8:49 [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition peter.wang
  2026-09-14  8:49 ` [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure peter.wang
@ 2026-09-14  8:49 ` peter.wang
  2026-09-14  9:13   ` sashiko-bot
  2026-09-22 10:06 ` [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition chun-hung.wu
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: peter.wang @ 2026-09-14  8:49 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar, bvanassche,
	santosh.sy, h.vinayak
  Cc: wsd_upstream, linux-mediatek, peter.wang, chun-hung.wu,
	alice.chao, cc.chou, chaotian.jing, tun-yu.yu, naomi.chu, ed.tsai

From: Peter Wang <peter.wang@mediatek.com>

A race exists between ufshcd_auto_hibern8_update() and
ufs_mtk_pwr_change_notify() when both paths concurrently access
REG_AUTO_HIBERNATE_IDLE_TIMER. Additionally, the static local
variable used to save/restore AHIT in the power change path is
not re-entrant and may hold a stale value if a sysfs write races
with a gear change.

Introduce ahit_mutex and ahit_disable_depth in struct ufs_hba to
serialize AHIT register access. The depth counter tracks how many
host-driver paths have forced AHIT off; sysfs writes are suppressed
while it is non-zero and the register is restored from hba->ahit
only when the last disabler drops it to zero.

Fixes: f5ca8d0c7a63 ("scsi: ufs: host: mediatek: Disable auto-hibern8 during power mode changes")
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c       | 14 +++++++++++++-
 drivers/ufs/host/ufs-mediatek.c | 16 ++++++++++++----
 include/ufs/ufshcd.h            |  8 ++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 39a4bdafc1af..c54ff35b4196 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4748,7 +4748,17 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
 	if (!pm_runtime_suspended(&hba->ufs_device_wlun->sdev_gendev)) {
 		ufshcd_rpm_get_sync(hba);
 		ufshcd_hold(hba);
-		ufshcd_configure_auto_hibern8(hba);
+		/*
+		 * Serialize with the host driver's AHIT control. Skip the
+		 * register write while the driver has AHIT forced off; the
+		 * driver programs hba->ahit back when it re-enables it.
+		 * Mutex is taken here (not across the rpm calls above) so it
+		 * never blocks a resume that the driver's path may need.
+		 */
+		mutex_lock(&hba->ahit_mutex);
+		if (!hba->ahit_disable_depth)
+			ufshcd_configure_auto_hibern8(hba);
+		mutex_unlock(&hba->ahit_mutex);
 		ufshcd_release(hba);
 		ufshcd_rpm_put_sync(hba);
 	}
@@ -11272,6 +11282,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	mutex_init(&hba->wb_mutex);
 
+	mutex_init(&hba->ahit_mutex);
+
 	init_rwsem(&hba->clk_scaling_lock);
 
 	ufshcd_init_clk_gating(hba);
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 814c1b7343b9..b6279edde176 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1507,19 +1507,27 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba *hba,
 				struct ufs_pa_layer_attr *dev_req_params)
 {
 	int ret = 0;
-	static u32 reg;
 
 	switch (stage) {
 	case PRE_CHANGE:
 		if (ufshcd_is_auto_hibern8_supported(hba)) {
-			reg = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
+			/* Block sysfs AHIT writes while we force AHIT off */
+			mutex_lock(&hba->ahit_mutex);
+			hba->ahit_disable_depth++;
+			mutex_unlock(&hba->ahit_mutex);
 			ufs_mtk_auto_hibern8_disable(hba);
 		}
 		ret = ufs_mtk_pre_pwr_change(hba, dev_req_params);
 		break;
 	case POST_CHANGE:
-		if (ufshcd_is_auto_hibern8_supported(hba))
-			ufshcd_writel(hba, reg, REG_AUTO_HIBERNATE_IDLE_TIMER);
+		if (ufshcd_is_auto_hibern8_supported(hba)) {
+			mutex_lock(&hba->ahit_mutex);
+			/* re-enable only when the last disabler drops off */
+			if (!--hba->ahit_disable_depth)
+				ufshcd_writel(hba, hba->ahit,
+					      REG_AUTO_HIBERNATE_IDLE_TIMER);
+			mutex_unlock(&hba->ahit_mutex);
+		}
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index dfd302f2dc7c..be62165b785e 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1132,6 +1132,14 @@ struct ufs_hba {
 
 	/* Auto-Hibernate Idle Timer register value */
 	u32 ahit;
+	/* Serializes AHIT register access between sysfs and the host driver */
+	struct mutex ahit_mutex;
+	/*
+	 * Nesting count of host-driver paths that have AHIT forced off.
+	 * Non-zero blocks sysfs AHIT writes; AHIT is reprogrammed only when
+	 * the last disabler drops it back to zero. Guarded by ahit_mutex.
+	 */
+	int ahit_disable_depth;
 
 	unsigned long outstanding_tasks;
 	spinlock_t outstanding_lock;
-- 
2.45.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure
  2026-09-14  8:49 ` [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure peter.wang
@ 2026-09-14  9:02   ` sashiko-bot
  2026-09-14 12:42     ` Peter Wang (王信友)
  0 siblings, 1 reply; 12+ messages in thread
From: sashiko-bot @ 2026-09-14  9:02 UTC (permalink / raw)
  To: peter.wang; +Cc: linux-scsi

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Passing `NULL` for `dev_req_params` on failure causes spurious error logs in unmodified vendor drivers (`ufs-qcom`, `ufs-hisi`) and destroys the context needed for proper resource rollback.
--

commit e9e864935f979762e71567d19e93d8a30870f23b
Author: Peter Wang <peter.wang@mediatek.com>

ufs: always notify POST_CHANGE even on gear switch failure

This commit modifies ufshcd_change_power_mode() to always notify POST_CHANGE,
even if the gear switch fails. On failure, it passes NULL for dev_req_params
and updates ufs-exynos and ufshcd-pci to handle the NULL argument.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 234e18b5078f2..2e7a592dbf2f1 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5003,8 +5003,12 @@ int ufshcd_change_power_mode(struct ufs_hba *hba,
>  
>  	ret = ufshcd_dme_change_power_mode(hba, pwr_mode, pmc_policy);
>  
> -	if (!ret)
> -		ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, pwr_mode);
> +	/*
> +	 * Always notify POST_CHANGE to pair with PRE_CHANGE, even on failure.
> +	 * On failure, pass dev_req_params = NULL to indicate nothing to apply.
> +	 * POST_CHANGE return value is advisory and does not mask @ret.
> +	 */
> +	ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, ret ? NULL : pwr_mode);

[Severity: Medium]
Will passing NULL for dev_req_params on failure cause issues for unmodified
vendor drivers?

Looking at ufs_qcom_pwr_change_notify() in drivers/ufs/host/ufs-qcom.c, it
has an early check that will trap this NULL argument and log a spurious error
before any POST_CHANGE cleanup can execute:

drivers/ufs/host/ufs-qcom.c:ufs_qcom_pwr_change_notify() {
	if (!dev_req_params) {
		pr_err("%s: incoming dev_req_params is NULL\n", __func__);
		return -EINVAL;
	}
	...
}

Similarly, ufs_hisi_pwr_change_notify() in drivers/ufs/host/ufs-hisi.c will
also log a spurious error on every power mode change failure:

drivers/ufs/host/ufs-hisi.c:ufs_hisi_pwr_change_notify() {
	if (!dev_req_params) {
		dev_err(hba->dev,
			    "%s: incoming dev_req_params is NULL\n", __func__);
		ret = -EINVAL;
		goto out;
	}
	...
}

Additionally, does passing NULL here destroy the context needed for proper
resource rollback?

For instance, in ufs_qcom_pwr_change_notify(), resources are acquired in
PRE_CHANGE based on the dev_req_params:

drivers/ufs/host/ufs-qcom.c:ufs_qcom_pwr_change_notify() {
	...
		/* enable the device ref clock before changing to HS mode */
		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
			ufshcd_is_hs_mode(dev_req_params))
			ufs_qcom_dev_ref_clk_ctrl(host, true);
	...
}

By omitting the original parameters on failure, the target drivers are prevented
from using the requested mode context in POST_CHANGE to safely revert their
state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914085148.136429-1-peter.wang@mediatek.com?part=1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver
  2026-09-14  8:49 ` [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver peter.wang
@ 2026-09-14  9:13   ` sashiko-bot
  2026-09-14 12:44     ` Peter Wang (王信友)
  0 siblings, 1 reply; 12+ messages in thread
From: sashiko-bot @ 2026-09-14  9:13 UTC (permalink / raw)
  To: peter.wang; +Cc: linux-scsi

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The `ufshcd_auto_hibern8_update()` function ignores the return value of `ufshcd_rpm_get_sync()`. If the runtime PM resume fails, it proceeds to unsafely write to the `REG_AUTO_HIBERNATE_IDLE_TIMER` register on a potentially suspended device.
- [High] Potential NULL pointer dereference in `ufshcd_auto_hibern8_update` if called via sysfs before the device WLUN is fully initialized.
- [High] The UFS core driver registers a shared IRQ (`IRQF_SHARED`) but calls `disable_irq()` during runtime suspend instead of `synchronize_irq()`, and fails to verify device activity in its interrupt handler.
--

commit 10d014f6ed545565699c7988448c9337de308b17
Author: Peter Wang <peter.wang@mediatek.com>

ufs: serialize AHIT register access between sysfs and host driver

This commit introduces a mutex and a depth counter to serialize AHIT register
access between the host driver and sysfs writes. This prevents stale values
from being programmed if a sysfs write races with a gear change.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 2e7a592dbf2f1..1d168eecfd013 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4772,7 +4772,17 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
>  	if (!pm_runtime_suspended(&hba->ufs_device_wlun->sdev_gendev)) {

[Severity: High]
This isn't a bug introduced by this patch, but can this dereference a
NULL pointer?

Sysfs attributes are exposed in ufshcd_init() via ufs_sysfs_add_nodes()
before hba->ufs_device_wlun is allocated.  The initialization is deferred
to the ufshcd_async_scan worker.

If a user writes to the sysfs node before ufshcd_add_lus() allocates
hba->ufs_device_wlun, can auto_hibern8_store() call
ufshcd_auto_hibern8_update() and locklessly dereference a NULL pointer here?

>  		ufshcd_rpm_get_sync(hba);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value
of ufshcd_rpm_get_sync() here?

If the runtime PM resume fails (e.g., due to a power domain failure or
device removal), pm_runtime_get_sync() returns a negative error code.

Does ignoring this error allow the function to proceed and write to
REG_AUTO_HIBERNATE_IDLE_TIMER on a potentially suspended device, causing a
synchronous external abort?

>  		ufshcd_hold(hba);
> -		ufshcd_configure_auto_hibern8(hba);
> +		/*
> +		 * Serialize with the host driver's AHIT control. Skip the
> +		 * register write while the driver has AHIT forced off; the
> +		 * driver programs hba->ahit back when it re-enables it.
> +		 * Mutex is taken here (not across the rpm calls above) so it
> +		 * never blocks a resume that the driver's path may need.
> +		 */
> +		mutex_lock(&hba->ahit_mutex);
> +		if (!hba->ahit_disable_depth)
> +			ufshcd_configure_auto_hibern8(hba);
> +		mutex_unlock(&hba->ahit_mutex);
>  		ufshcd_release(hba);
>  		ufshcd_rpm_put_sync(hba);
>  	}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the UFS core driver handle shared
IRQs correctly?

The driver registers its IRQ with IRQF_SHARED. When the UFS controller enters
runtime suspend, ufshcd_suspend() disables the interrupt line entirely:

drivers/ufs/core/ufshcd.c:ufshcd_suspend() {
    ...
	ufshcd_disable_irq(hba);
    ...
}

Does calling disable_irq() mask interrupts for any other devices sharing the
line? Should this use synchronize_irq() instead?

Additionally, ufshcd_intr() reads the hardware register unconditionally:

drivers/ufs/core/ufshcd.c:ufshcd_intr() {
    ...
	last_intr_status = intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
    ...
}

Can this access powered-off hardware if the interrupt belongs to another
device on the same shared line? Should this verify the device is actively
powered using pm_runtime_get_if_active() before accessing registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914085148.136429-1-peter.wang@mediatek.com?part=2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure
  2026-09-14  9:02   ` sashiko-bot
@ 2026-09-14 12:42     ` Peter Wang (王信友)
  0 siblings, 0 replies; 12+ messages in thread
From: Peter Wang (王信友) @ 2026-09-14 12:42 UTC (permalink / raw)
  To: sashiko-reviews@lists.linux.dev; +Cc: linux-scsi@vger.kernel.org

On Mon, 2026-09-14 at 09:02 +0000, sashiko-bot@kernel.org wrote
> 
> [Severity: Medium]
> Will passing NULL for dev_req_params on failure cause issues for
> unmodified
> vendor drivers?
> 
> Looking at ufs_qcom_pwr_change_notify() in drivers/ufs/host/ufs-
> qcom.c, it
> has an early check that will trap this NULL argument and log a
> spurious error
> before any POST_CHANGE cleanup can execute:
> 
> drivers/ufs/host/ufs-qcom.c:ufs_qcom_pwr_change_notify() {
> 	if (!dev_req_params) {
> 		pr_err("%s: incoming dev_req_params is NULL\n",
> __func__);
> 		return -EINVAL;
> 	}
> 	...
> }
> 
> Similarly, ufs_hisi_pwr_change_notify() in drivers/ufs/host/ufs-
> hisi.c will
> also log a spurious error on every power mode change failure:
> 
> drivers/ufs/host/ufs-hisi.c:ufs_hisi_pwr_change_notify() {
> 	if (!dev_req_params) {
> 		dev_err(hba->dev,
> 			    "%s: incoming dev_req_params is NULL\n",
> __func__);
> 		ret = -EINVAL;
> 		goto out;
> 	}
> 	...
> }
> 

Yes, both the qcom and hisi host drivers ignore NULL and only print an
error log that indicates a power‑mode‑change failure.


> Additionally, does passing NULL here destroy the context needed for
> proper
> resource rollback?
> 
> For instance, in ufs_qcom_pwr_change_notify(), resources are acquired
> in
> PRE_CHANGE based on the dev_req_params:
> 

This patch doesn't change the behavior of the qcom driver.

Thanks
Peter

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver
  2026-09-14  9:13   ` sashiko-bot
@ 2026-09-14 12:44     ` Peter Wang (王信友)
  0 siblings, 0 replies; 12+ messages in thread
From: Peter Wang (王信友) @ 2026-09-14 12:44 UTC (permalink / raw)
  To: sashiko-reviews@lists.linux.dev; +Cc: linux-scsi@vger.kernel.org

On Mon, 2026-09-14 at 09:13 +0000, sashiko-bot@kernel.org wrote:
> [Severity: High]
> This isn't a bug introduced by this patch, but can this dereference a
> NULL pointer?
> 
> Sysfs attributes are exposed in ufshcd_init() via
> ufs_sysfs_add_nodes()
> before hba->ufs_device_wlun is allocated.  The initialization is
> deferred
> to the ufshcd_async_scan worker.
> 
> If a user writes to the sysfs node before ufshcd_add_lus() allocates
> hba->ufs_device_wlun, can auto_hibern8_store() call
> ufshcd_auto_hibern8_update() and locklessly dereference a NULL
> pointer here?
> 

This patch focuses on the current issue.
The previous issue can be addressed with a separate patch, 
but not with this one.

> >  		ufshcd_rpm_get_sync(hba);
> 
> [Severity: High]
> This is a pre-existing issue, but is it safe to ignore the return
> value
> of ufshcd_rpm_get_sync() here?
> 
> If the runtime PM resume fails (e.g., due to a power domain failure
> or
> device removal), pm_runtime_get_sync() returns a negative error code.
> 
> Does ignoring this error allow the function to proceed and write to
> REG_AUTO_HIBERNATE_IDLE_TIMER on a potentially suspended device,
> causing a
> synchronous external abort?
> 

Same, pre-existing issue should be addressed with a separate patch.


> >  		ufshcd_hold(hba);
> > -		ufshcd_configure_auto_hibern8(hba);
> > +		/*
> > +		 * Serialize with the host driver's AHIT control.
> > Skip the
> > +		 * register write while the driver has AHIT forced
> > off; the
> > +		 * driver programs hba->ahit back when it re-
> > enables it.
> > +		 * Mutex is taken here (not across the rpm calls
> > above) so it
> > +		 * never blocks a resume that the driver's path
> > may need.
> > +		 */
> > +		mutex_lock(&hba->ahit_mutex);
> > +		if (!hba->ahit_disable_depth)
> > +			ufshcd_configure_auto_hibern8(hba);
> > +		mutex_unlock(&hba->ahit_mutex);
> >  		ufshcd_release(hba);
> >  		ufshcd_rpm_put_sync(hba);
> >  	}
> 
> [ ... ]
> 
> [Severity: High]
> This is a pre-existing issue, but does the UFS core driver handle
> shared
> IRQs correctly?
> 

Same, pre-existing issue should be addressed with a separate patch.


> The driver registers its IRQ with IRQF_SHARED. When the UFS
> controller enters
> runtime suspend, ufshcd_suspend() disables the interrupt line
> entirely:
> 
> drivers/ufs/core/ufshcd.c:ufshcd_suspend() {
>     ...
> 	ufshcd_disable_irq(hba);
>     ...
> }
> 
> Does calling disable_irq() mask interrupts for any other devices
> sharing the
> line? Should this use synchronize_irq() instead?
> 
> Additionally, ufshcd_intr() reads the hardware register
> unconditionally:
> 
> drivers/ufs/core/ufshcd.c:ufshcd_intr() {
>     ...
> 	last_intr_status = intr_status = ufshcd_readl(hba,
> REG_INTERRUPT_STATUS);
>     ...
> }
> 
> Can this access powered-off hardware if the interrupt belongs to
> another
> device on the same shared line? Should this verify the device is
> actively
> powered using pm_runtime_get_if_active() before accessing registers?
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition
  2026-09-14  8:49 [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition peter.wang
  2026-09-14  8:49 ` [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure peter.wang
  2026-09-14  8:49 ` [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver peter.wang
@ 2026-09-22 10:06 ` chun-hung.wu
  2026-09-26  2:30 ` Martin K. Petersen (Oracle)
  2026-10-06  2:44 ` Martin K. Petersen (Oracle)
  4 siblings, 0 replies; 12+ messages in thread
From: chun-hung.wu @ 2026-09-22 10:06 UTC (permalink / raw)
  To: peter.wang, linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	bvanassche, santosh.sy, h.vinayak
  Cc: wsd_upstream, linux-mediatek, alice.chao, cc.chou, chaotian.jing,
	tun-yu.yu, naomi.chu, ed.tsai

On Mon, 2026-09-14 at 16:49 +0800, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> Patch 1 fixes a bug where POST_CHANGE was skipped on a failed gear
> switch, leaving the host in an inconsistent state if resources
> acquired
> in PRE_CHANGE were never released.
> 
> Patch 2 fixes a race between the sysfs AHIT update path and the
> MediaTek host driver's power mode change path. A mutex and depth
> counter are introduced to serialize AHIT register access and ensure
> AHIT is correctly restored after the last disabler completes.
> 
> The two patches are related: patch 1 guarantees POST_CHANGE is always
> called, ensuring the depth counter introduced in patch 2 is always
> properly decremented.
> 
> Peter Wang (2):
>   ufs: always notify POST_CHANGE even on gear switch failure
>   ufs: serialize AHIT register access between sysfs and host driver
> 
>  drivers/ufs/core/ufshcd.c       | 22 +++++++++++++++++++---
>  drivers/ufs/host/ufs-exynos.c   |  4 +++-
>  drivers/ufs/host/ufs-mediatek.c | 16 ++++++++++++----
>  drivers/ufs/host/ufshcd-pci.c   |  3 ++-
>  include/ufs/ufshcd.h            |  8 ++++++++
>  5 files changed, 44 insertions(+), 9 deletions(-)
> 

Reviewed-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition
  2026-09-14  8:49 [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition peter.wang
                   ` (2 preceding siblings ...)
  2026-09-22 10:06 ` [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition chun-hung.wu
@ 2026-09-26  2:30 ` Martin K. Petersen (Oracle)
  2026-10-05 15:45   ` Sandeep Dhavale
  2026-10-06  2:44 ` Martin K. Petersen (Oracle)
  4 siblings, 1 reply; 12+ messages in thread
From: Martin K. Petersen (Oracle) @ 2026-09-26  2:30 UTC (permalink / raw)
  To: peter.wang
  Cc: linux-scsi, martin.petersen, avri.altman, alim.akhtar, bvanassche,
	santosh.sy, h.vinayak, wsd_upstream, linux-mediatek, chun-hung.wu,
	alice.chao, cc.chou, chaotian.jing, tun-yu.yu, naomi.chu, ed.tsai


> Patch 1 fixes a bug where POST_CHANGE was skipped on a failed gear
> switch, leaving the host in an inconsistent state if resources acquired
> in PRE_CHANGE were never released.
>
> Patch 2 fixes a race between the sysfs AHIT update path and the
> MediaTek host driver's power mode change path. A mutex and depth
> counter are introduced to serialize AHIT register access and ensure
> AHIT is correctly restored after the last disabler completes.

Applied to 7.4/scsi-staging, thanks!

-- 
Martin K. Petersen


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition
  2026-09-26  2:30 ` Martin K. Petersen (Oracle)
@ 2026-10-05 15:45   ` Sandeep Dhavale
  2026-10-06  2:42     ` Martin K. Petersen
  0 siblings, 1 reply; 12+ messages in thread
From: Sandeep Dhavale @ 2026-10-05 15:45 UTC (permalink / raw)
  To: Martin K. Petersen (Oracle)
  Cc: peter.wang, linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	bvanassche, santosh.sy, h.vinayak, wsd_upstream, linux-mediatek,
	chun-hung.wu, alice.chao, cc.chou, chaotian.jing, tun-yu.yu,
	naomi.chu, ed.tsai

>
> Applied to 7.4/scsi-staging, thanks!
>
> --
> Martin K. Petersen

Hi Martin,
I don't see this series in in 7.4/scsi-staging at [1], or perhaps just
push is missing?

Thanks,
Sandeep.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/log/?h=7.4/scsi-staging


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition
  2026-10-05 15:45   ` Sandeep Dhavale
@ 2026-10-06  2:42     ` Martin K. Petersen
  0 siblings, 0 replies; 12+ messages in thread
From: Martin K. Petersen @ 2026-10-06  2:42 UTC (permalink / raw)
  To: Sandeep Dhavale
  Cc: Martin K. Petersen (Oracle), peter.wang, linux-scsi,
	martin.petersen, avri.altman, alim.akhtar, bvanassche, santosh.sy,
	h.vinayak, wsd_upstream, linux-mediatek, chun-hung.wu, alice.chao,
	cc.chou, chaotian.jing, tun-yu.yu, naomi.chu, ed.tsai


Sandeep,

> I don't see this series in in 7.4/scsi-staging at [1], or perhaps just
> push is missing?

Just pushed.

-- 
Martin K. Petersen


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition
  2026-09-14  8:49 [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition peter.wang
                   ` (3 preceding siblings ...)
  2026-09-26  2:30 ` Martin K. Petersen (Oracle)
@ 2026-10-06  2:44 ` Martin K. Petersen (Oracle)
  4 siblings, 0 replies; 12+ messages in thread
From: Martin K. Petersen (Oracle) @ 2026-10-06  2:44 UTC (permalink / raw)
  To: linux-scsi, avri.altman, alim.akhtar, bvanassche, santosh.sy,
	h.vinayak, Martin K. Petersen, peter.wang
  Cc: wsd_upstream, linux-mediatek, chun-hung.wu, alice.chao, cc.chou,
	chaotian.jing, tun-yu.yu, naomi.chu, ed.tsai

On Mon, 14 Sep 2026 16:49:23 +0800, peter.wang@mediatek.com wrote:

> Patch 1 fixes a bug where POST_CHANGE was skipped on a failed gear
> switch, leaving the host in an inconsistent state if resources acquired
> in PRE_CHANGE were never released.
> 
> Patch 2 fixes a race between the sysfs AHIT update path and the
> MediaTek host driver's power mode change path. A mutex and depth
> counter are introduced to serialize AHIT register access and ensure
> AHIT is correctly restored after the last disabler completes.
> 
> [...]

Applied to 7.4/scsi-queue, thanks!

[1/2] ufs: always notify POST_CHANGE even on gear switch failure
      https://git.kernel.org/mkp/scsi/c/6efee14ca0ae
[2/2] ufs: serialize AHIT register access between sysfs and host driver
      https://git.kernel.org/mkp/scsi/c/7dec5cedf7b2

-- 
Martin K. Petersen


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-10-06  2:44 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-14  8:49 [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition peter.wang
2026-09-14  8:49 ` [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure peter.wang
2026-09-14  9:02   ` sashiko-bot
2026-09-14 12:42     ` Peter Wang (王信友)
2026-09-14  8:49 ` [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver peter.wang
2026-09-14  9:13   ` sashiko-bot
2026-09-14 12:44     ` Peter Wang (王信友)
2026-09-22 10:06 ` [PATCH v2 0/2] ufs: fix POST_CHANGE pairing and AHIT race condition chun-hung.wu
2026-09-26  2:30 ` Martin K. Petersen (Oracle)
2026-10-05 15:45   ` Sandeep Dhavale
2026-10-06  2:42     ` Martin K. Petersen
2026-10-06  2:44 ` Martin K. Petersen (Oracle)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.