Re: [PATCH v10 08/26] scsi: ufs: mediatek: Rework init function

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

Pre-existing issues:
- [High] The return value of `ufs_mtk_mphy_power_on()` is ignored during host initialization.
- [High] Use-After-Free risk due to saving a released device pointer in `host->phy_dev`.
--

--- Patch [8]: [PATCH v10 08/26] scsi: ufs: mediatek: Rework init function ---
commit 5d79268ac1559dd4c6c615601ae1b9838e4b77bc
Author: Nicolas Frattaroli <[email protected]>

scsi: ufs: mediatek: Rework init function

This commit reworks the initialization function for the MediaTek UFS driver.
It removes unnecessary error prints for memory allocation failures and removes
redundant device tree matching. It also simplifies the error handling path
by removing unnecessary goto statements and returning directly on success.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index f6859cd0b1d4d..50e7701d7943b 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -1356,11 +1334,10 @@ static int ufs_mtk_init(struct ufs_hba *hba)
>  
>  	ufs_mtk_get_hw_ip_version(hba);
>  
> -	goto out;
> +	return 0;
>  
>  out_variant_clear:
>  	ufshcd_set_variant(hba, NULL);
> -out:
>  	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this code ignore the
return value of ufs_mtk_mphy_power_on() earlier in ufs_mtk_init()?

Looking at ufs_mtk_init():

	err = ufs_mtk_get_supplies(host);
	if (err)
		goto out_variant_clear;

	/*
	 * 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 ufs_mtk_mphy_power_on() fails (e.g. regulator_enable fails in
ufs_mtk_09v_on), the driver will proceed as if the PHY powered on
successfully.  Should this check the return value and handle the error?

[ ... ]
> @@ -2402,6 +2379,18 @@ static const struct ufs_hba_variant_ops ufs_hba_mtk_vops = {
>  	.config_scsi_dev     = ufs_mtk_config_scsi_dev,
>  };
>  
> +static const struct ufs_mtk_soc_data mt8183_data = {
> +	.has_avdd09 = true,
> +};
> +
> +static const struct of_device_id ufs_mtk_of_match[] = {
> +	{ .compatible = "mediatek,mt8183-ufshci", .data = &mt8183_data },
> +	{ .compatible = "mediatek,mt8192-ufshci" },
> +	{ .compatible = "mediatek,mt8195-ufshci" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ufs_mtk_of_match);
> +
>  /**
>   * ufs_mtk_probe - probe routine of the driver
>   * @pdev: pointer to Platform device handle

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk in
ufs_mtk_probe() when saving the released device pointer?

Looking at ufs_mtk_probe():

	if (phy_node) {
		phy_pdev = of_find_device_by_node(phy_node);
		if (!phy_pdev)
			goto skip_phy;
		phy_dev = &phy_pdev->dev;

		...
		put_device(phy_dev);
	}
	...
skip_phy:
	err = ufshcd_pltfrm_init(pdev, &ufs_hba_mtk_vops);
	...
	if (phy_node && phy_dev) {
		host = ufshcd_get_variant(hba);
		host->phy_dev = phy_dev;
	}

of_find_device_by_node() acquires a reference to the phy device. This
reference is unconditionally dropped via put_device(phy_dev), but the
unreferenced pointer is then stored in host->phy_dev.

If the phy device is removed or unbound and the memory is freed, the UFS
driver continues to dereference it during runtime PM operations:

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_runtime_suspend() {
    ...
	if (host->phy_dev)
		pm_runtime_put_sync(host->phy_dev);
    ...
}

Should the reference be held until the UFS host device is unbound?

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