Linux SCSI subsystem development
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox