Re: [PATCH v10 13/26] scsi: ufs: mediatek: Use the common PHY framework

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

New issues:
- [High] The driver now fails to probe on platforms where the UFS PHY is not defined in the device tree, breaking backward compatibility.
- [High] Returning an error late in PM runtime callbacks leaves the device in an inconsistent state and breaks PM state synchronization.

Pre-existing issues:
- [High] The return values of `phy_power_on()` and `phy_power_off()` are ignored, which can lead to unbalanced PM runtime counts and hardware state corruption.
--

--- Patch [13]: [PATCH v10 13/26] scsi: ufs: mediatek: Use the common PHY framework ---
commit 114da5212062d5f11b7f9274ba9701891ef82788
Author: Nicolas Frattaroli <[email protected]>

scsi: ufs: mediatek: Use the common PHY framework
    
This patch refactors the MediaTek UFS driver to use the common PHY framework 
to acquire the device's PHY during ufshcd init, ensuring it is device-linked 
to the correct device.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index e5527969a5e67..385f0180b2309 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c

[ ... ]

> @@ -1185,13 +1147,21 @@ static int ufs_mtk_init(struct ufs_hba *hba)
>  
>  	ufs_mtk_init_mcq_irq(hba);
>  
> -	err = ufs_mtk_bind_mphy(hba);
> -	if (err)
> +	host->mphy = devm_phy_get(dev, NULL);
> +	if (IS_ERR(host->mphy)) {
> +		err = dev_err_probe(dev, PTR_ERR(host->mphy), "Failed to get PHY\n");

[Severity: High]
Will this cause the driver probe to fail on platforms where the UFS PHY
is not defined in the device tree?

The original ufs_mtk_bind_mphy() tolerated -ENODEV to allow for unbound 
mphys since not all platforms require specific mphy control. By switching 
to devm_phy_get() instead of devm_phy_optional_get(), won't this return 
-ENODEV and fail the probe if the "phys" property is missing, breaking 
backward compatibility with older device trees?

> +		goto out_variant_clear;
> +	}
> +
> +	err = phy_init(host->mphy);
> +	if (err) {
> +		dev_err_probe(dev, err, "Failed to initialize PHY\n");
>  		goto out_variant_clear;
> +	}

[ ... ]

> @@ -2448,9 +2393,8 @@ static int ufs_mtk_system_resume(struct device *dev)
>  static int ufs_mtk_runtime_suspend(struct device *dev)
>  {
>  	struct ufs_hba *hba = dev_get_drvdata(dev);
> -	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
>  	struct arm_smccc_res res;
> -	int ret = 0;
> +	int ret;
>  
>  	ret = ufshcd_runtime_suspend(dev);
>  	if (ret)
> @@ -2461,8 +2405,11 @@ static int ufs_mtk_runtime_suspend(struct device *dev)
>  	if (ufs_mtk_is_rtff_mtcmos(hba))
>  		ufs_mtk_mtcmos_ctrl(false, res);
>  
> -	if (host->phy_dev)
> -		pm_runtime_put_sync(host->phy_dev);
> +	ret = ufs_mtk_mphy_power_on(hba, false);
> +	if (ret) {
> +		dev_err(dev, "Failed to power off PHY: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}

[Severity: High]
Does returning an error here leave the device in an inconsistent state?

If ufshcd_runtime_suspend() succeeds, the main UFS controller hardware is
fully suspended (clocks gated, link hibern8). If ufs_mtk_mphy_power_on() 
fails afterward, this returns the error to the PM core. 

Since the PM core intercepts the non-zero return as a runtime suspend 
failure, it leaves the software state as RPM_ACTIVE. If the software 
thinks the device is active while the hardware is actually suspended, 
won't the next hardware access skip runtime_resume and cause a system 
hang or bus error?

[Severity: High]
This is a pre-existing issue, but does ufs_mtk_mphy_power_on() safely
handle phy_power_on() failures?

Looking at ufs_mtk_mphy_power_on() in drivers/ufs/host/ufs-mediatek.c:

static int ufs_mtk_mphy_power_on(struct ufs_hba *hba, bool on)
{
        ...
        if (on) {
                ret = ufs_mtk_09v_on(host);
                if (ret)
                        return ret;
                phy_power_on(mphy);
        } else {
                phy_power_off(mphy);
        ...
        host->mphy_powered_on = on;
        return 0;
}

If phy_power_on() fails, it cleans up its runtime PM and does not 
increment the power_count, but ufs_mtk_mphy_power_on() ignores the return 
value and still sets host->mphy_powered_on = true. 

When ufs_mtk_mphy_power_on(hba, false) is called later, it will blindly 
call phy_power_off(). Won't this decrement the PHY's power_count 
(potentially underflowing it), corrupting the PHY driver's PM usage 
counter and causing an unbalanced PM state?

>  
>  	return 0;
>  }
> @@ -2470,14 +2417,17 @@ static int ufs_mtk_runtime_suspend(struct device *dev)
>  static int ufs_mtk_runtime_resume(struct device *dev)
>  {
>  	struct ufs_hba *hba = dev_get_drvdata(dev);
> -	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
>  	struct arm_smccc_res res;
> +	int ret;
>  
>  	if (ufs_mtk_is_rtff_mtcmos(hba))
>  		ufs_mtk_mtcmos_ctrl(true, res);
>  
> -	if (host->phy_dev)
> -		pm_runtime_get_sync(host->phy_dev);
> +	ret = ufs_mtk_mphy_power_on(hba, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to power on PHY: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}

[Severity: High]
Could a failure here cause similar PM state inconsistencies?

If ufs_mtk_mphy_power_on() fails during runtime resume, returning an 
error without rolling back the prior partial initialization (such as 
ufs_mtk_mtcmos_ctrl) might confuse the PM core. Does this need to clean 
up previously resumed components before returning?

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