ufs: fix POST_CHANGE pairing and AHIT race condition

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

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

Quoted 1 line

  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?

Quoted 21 lines

  ufshcd_hold(hba);

drivers/ufs/core/ufshcd.c +2 -1

-		ufshcd_configure_auto_hibern8(hba);
+		/*
+		 * Serialize with the host driver's AHIT control.

Skip the

(quoted diff) +1 -0

+		 * register write while the driver has AHIT forced

off; the

(quoted diff) +1 -0

+		 * driver programs hba->ahit back when it re-

enables it.

(quoted diff) +1 -0

+		 * Mutex is taken here (not across the rpm calls

above) so it

(quoted diff) +1 -0

+		 * never blocks a resume that the driver's path

may need.

drivers/ufs/core/ufshcd.c +5 -0

+		 */
+		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?

From: Peter Wang <[email protected]>

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(-)

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

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

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.

[...]