Re: [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver

[sashiko-bot@kernel.org]

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