Re: [PATCH v10 20/26] scsi: ufs: mediatek: Back up idle timer in per-instance struct

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

Pre-existing issues:
- [High] Skipped POST_CHANGE notification on power change failure leads to permanent loss of auto-hibernate configuration, disabling Auto-Hibern8.
--

--- Patch [20]: [PATCH v10 20/26] scsi: ufs: mediatek: Back up idle timer in per-instance struct ---
commit 31da80f38058b251310b08adf616cded137a1bcd
Author: Nicolas Frattaroli <[email protected]>

scsi: ufs: mediatek: Back up idle timer in per-instance struct

This commit moves the backup of the auto-hibernate idle timer from a
static local variable to a per-instance host struct member. This ensures
correct behavior when multiple instances of the driver are loaded.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index c784ecb30e74e..115056927223f 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1400,27 +1400,24 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba *hba,
>  				enum ufs_notify_change_status stage,
>  				struct ufs_pa_layer_attr *dev_req_params)
>  {
> -	int ret = 0;
> -	static u32 reg;
> +	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
>  
>  	switch (stage) {
>  	case PRE_CHANGE:
>  		if (ufshcd_is_auto_hibern8_supported(hba)) {
> -			reg = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
> +			host->ahit = ufshcd_readl(
> +				hba, REG_AUTO_HIBERNATE_IDLE_TIMER);

[Severity: High]
This isn't a bug introduced by this patch, but does this sequence permanently
disable Auto-Hibern8 if a power change fails?

If a DME power mode change fails, the UFS core in ufshcd_change_power_mode()
skips the POST_CHANGE notification:

drivers/ufs/core/ufshcd.c:ufshcd_change_power_mode() {
    ...
    ret = ufshcd_dme_change_power_mode(hba, pwr_mode);
    if (!ret)
        ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, pwr_mode);
    ...
}

Because POST_CHANGE is skipped, the hardware register remains at 0 (since
it was disabled right below by ufs_mtk_auto_hibern8_disable).

On the next power mode change attempt, PRE_CHANGE will run again and read the
current register value (0) into host->ahit, overwriting the valid backup.
When POST_CHANGE eventually succeeds, it will write 0 back to the register,
leaving Auto-Hibern8 permanently disabled until a reset or suspend/resume.

>  			ufs_mtk_auto_hibern8_disable(hba);
>  		}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=20