Re: [PATCH v10 10/26] scsi: ufs: mediatek: Handle misc host voltage regulators

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

Pre-existing issues:
- [High] The UFS driver caches a dangling pointer to the PHY device after prematurely dropping its reference, leading to a Use-After-Free.
- [High] The driver ignores the return value of `ufs_mtk_mphy_power_on()`, potentially leading to hardware hangs if the PHY fails to power on.
--

--- Patch [10]: [PATCH v10 10/26] scsi: ufs: mediatek: Handle misc host voltage regulators ---
commit 8242d1302e10cd1046e43a3ff8124f21223143fe
Author: Nicolas Frattaroli <[email protected]>
scsi: ufs: mediatek: Handle misc host voltage regulators

This commit adds support for acquiring miscellaneous host voltage regulators
using the regulator bulk API in the MediaTek UFS driver. It handles per-SoC
specific regulator names and enables them during acquisition so they are
kept on during suspend.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 55701172bcf15..65036c8390318 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c

[ ... ]

> @@ -1188,8 +1190,21 @@ static int ufs_mtk_get_supplies(struct ufs_mtk_host *host)
>  {
>  	struct device *dev = host->hba->dev;
>  	const struct ufs_mtk_soc_data *data = of_device_get_match_data(dev);
> +	int ret;
> +
> +	if (!data)
> +		return 0;
> +
> +	if (data->num_reg_names) {
> +		ret = devm_regulator_bulk_get_enable(dev, data->num_reg_names,
> +						     data->reg_names);
> +		if (ret) {
> +			dev_err(dev, "Failed to get misc regulators: %pe\n", ERR_PTR(ret));
> +			return ret;
> +		}
> +	}
>  
> -	if (!data || !data->has_avdd09)
> +	if (!data->has_avdd09)
>  		return 0;
>  
>  	host->reg_avdd09 = devm_regulator_get_optional(dev, "avdd09");

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does this driver ignore the 
return value of ufs_mtk_mphy_power_on() when initializing?

In ufs_mtk_init(), the function is called without checking if it fails:

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_init() {
    ...
	/*
	 * ufshcd_vops_init() is invoked after
	 * ufshcd_setup_clock(true) in ufshcd_hba_init() thus
	 * phy clock setup is skipped.
	 *
	 * Enable phy clocks specifically here.
	 */
	ufs_mtk_mphy_power_on(hba, true);

	if (ufs_mtk_is_rtff_mtcmos(hba)) {
    ...
}

If the underlying regulator enablement fails, the PHY could remain unpowered.

Could this cause the UFS host controller driver to assume the PHY is
functional, which might lead to timeouts or unrecoverable hardware hangs
during initialization?

[Severity: High]
This is also a pre-existing issue, but does this driver cache a dangling
pointer to the PHY device?

Looking at ufs_mtk_probe(), the driver acquires a device reference to the
PHY node but immediately drops it:

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_probe() {
    ...
	if (phy_node) {
		phy_dev = of_find_device_by_node(phy_node);
		of_node_put(phy_node);
		if (!phy_dev) {
			dev_err(dev, "phy_dev not found\n");
			goto skip_phy;
		}
		put_device(phy_dev);
		dev_info(dev, "phys node found\n");
	}
    ...
skip_phy:
    ...
	if (phy_node && phy_dev) {
		host = ufshcd_get_variant(hba);
		host->phy_dev = phy_dev;
	}
    ...
}

Because put_device() is called earlier, phy_dev could potentially be freed if
the PHY device is unbound.

If the PHY device is later freed, will accessing pm_runtime_put_sync() or
pm_runtime_get_sync() on host->phy_dev during suspend and resume callbacks
trigger a use-after-free?

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