[PATCH v4 11/25] scsi: ufs: mediatek: Rework probe function

Nicolas Frattaroli posted 25 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH v4 11/25] scsi: ufs: mediatek: Rework probe function
Posted by Nicolas Frattaroli 1 month, 3 weeks ago
Remove the ti,syscon-reset cruft.

Make PHY mandatory. All the compatibles supported by the binding make it
mandatory.

Entertain this driver's insistence on playing with the PHY's RPM, but at
least fix the part where it doesn't increase the reference count, which
would lead to use-after-free.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/ufs/host/ufs-mediatek.c | 87 +++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 55 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 10d6b69e91a5..cc6a3a4c9704 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -2406,74 +2406,49 @@ MODULE_DEVICE_TABLE(of, ufs_mtk_of_match);
  */
 static int ufs_mtk_probe(struct platform_device *pdev)
 {
-	int err;
-	struct device *dev = &pdev->dev, *phy_dev = NULL;
-	struct device_node *reset_node, *phy_node = NULL;
-	struct platform_device *reset_pdev, *phy_pdev = NULL;
-	struct device_link *link;
-	struct ufs_hba *hba;
+	struct platform_device *phy_pdev;
+	struct device *dev = &pdev->dev;
+	struct device_node *phy_node;
 	struct ufs_mtk_host *host;
+	struct device *phy_dev;
+	struct ufs_hba *hba;
+	int err;
 
-	reset_node = of_find_compatible_node(NULL, NULL,
-					     "ti,syscon-reset");
-	if (!reset_node) {
-		dev_notice(dev, "find ti,syscon-reset fail\n");
-		goto skip_reset;
-	}
-	reset_pdev = of_find_device_by_node(reset_node);
-	if (!reset_pdev) {
-		dev_notice(dev, "find reset_pdev fail\n");
-		goto skip_reset;
-	}
-	link = device_link_add(dev, &reset_pdev->dev,
-		DL_FLAG_AUTOPROBE_CONSUMER);
-	put_device(&reset_pdev->dev);
-	if (!link) {
-		dev_notice(dev, "add reset device_link fail\n");
-		goto skip_reset;
-	}
-	/* supplier is not probed */
-	if (link->status == DL_STATE_DORMANT) {
-		err = -EPROBE_DEFER;
-		goto out;
-	}
-
-skip_reset:
 	/* find phy node */
 	phy_node = of_parse_phandle(dev->of_node, "phys", 0);
+	if (!phy_node)
+		return dev_err_probe(dev, -ENOENT, "No PHY node found\n");
 
-	if (phy_node) {
-		phy_pdev = of_find_device_by_node(phy_node);
-		if (!phy_pdev)
-			goto skip_phy;
-		phy_dev = &phy_pdev->dev;
+	phy_pdev = of_find_device_by_node(phy_node);
+	of_node_put(phy_node);
+	if (!phy_pdev)
+		return dev_err_probe(dev, -ENODEV, "No PHY device found\n");
 
-		pm_runtime_set_active(phy_dev);
-		pm_runtime_enable(phy_dev);
-		pm_runtime_get_sync(phy_dev);
+	phy_dev = &phy_pdev->dev;
 
-		put_device(phy_dev);
-		dev_info(dev, "phys node found\n");
-	} else {
-		dev_notice(dev, "phys node not found\n");
+	err = pm_runtime_set_active(phy_dev);
+	if (err) {
+		dev_err_probe(dev, err, "Failed to activate PHY RPM\n");
+		goto err_put_phy;
+	}
+	pm_runtime_enable(phy_dev);
+	err = pm_runtime_get_sync(phy_dev);
+	if (err) {
+		dev_err_probe(dev, err, "Failed to power on PHY\n");
+		goto err_put_phy;
 	}
 
-skip_phy:
 	/* perform generic probe */
 	err = ufshcd_pltfrm_init(pdev, &ufs_hba_mtk_vops);
 	if (err) {
-		dev_err(dev, "probe failed %d\n", err);
-		goto out;
+		dev_err_probe(dev, err, "Generic platform probe failed\n");
+		goto err_put_phy;
 	}
 
 	hba = platform_get_drvdata(pdev);
-	if (!hba)
-		goto out;
 
-	if (phy_node && phy_dev) {
-		host = ufshcd_get_variant(hba);
-		host->phy_dev = phy_dev;
-	}
+	host = ufshcd_get_variant(hba);
+	host->phy_dev = phy_dev;
 
 	/*
 	 * Because the default power setting of VSx (the upper layer of
@@ -2482,9 +2457,11 @@ static int ufs_mtk_probe(struct platform_device *pdev)
 	 */
 	ufs_mtk_dev_vreg_set_lpm(hba, false);
 
-out:
-	of_node_put(phy_node);
-	of_node_put(reset_node);
+	return 0;
+
+err_put_phy:
+	put_device(phy_dev);
+
 	return err;
 }
 

-- 
2.52.0
Re: [PATCH v4 11/25] scsi: ufs: mediatek: Rework probe function
Posted by Peter Wang (王信友) 1 month ago
On Thu, 2025-12-18 at 13:55 +0100, Nicolas Frattaroli wrote:
> 
> Remove the ti,syscon-reset cruft.
> 

Hi Nicolas,

Why do we need to remove the reset node? If an error occurs and the
host 
does not perform a reset, it could lead to error recovery failure.

Thanks.
Peter
Re: [PATCH v4 11/25] scsi: ufs: mediatek: Rework probe function
Posted by Krzysztof Kozlowski 1 month ago
On 06/01/2026 14:23, Peter Wang (王信友) wrote:
> On Thu, 2025-12-18 at 13:55 +0100, Nicolas Frattaroli wrote:
>>
>> Remove the ti,syscon-reset cruft.
>>
> 
> Hi Nicolas,
> 
> Why do we need to remove the reset node? If an error occurs and the

Commit msg should be improved and provide the reason, e.g. because it is
undocumented and unused ABI.

> host 
> does not perform a reset, it could lead to error recovery failure.

This is not a reason to use undocumented ABI.

Best regards,
Krzysztof
Re: [PATCH v4 11/25] scsi: ufs: mediatek: Rework probe function
Posted by Nicolas Frattaroli 1 month ago
On Tuesday, 6 January 2026 14:23:58 Central European Standard Time Peter Wang (王信友) wrote:
> On Thu, 2025-12-18 at 13:55 +0100, Nicolas Frattaroli wrote:
> > 
> > Remove the ti,syscon-reset cruft.
> > 
> 
> Hi Nicolas,
> 
> Why do we need to remove the reset node? If an error occurs and the
> host 
> does not perform a reset, it could lead to error recovery failure.

Because it's not described by the binding, and appears to be a
downstream hack to work around not having the reset controller
properly described and referred to with a `resets` property.

Even if you were to use `ti,syscon-reset` to describe a reset
controller, the UFS controller driver should not be searching
for this compatible. It should access the reset through the
reset API. The common reset code can then take care of probe
ordering without every driver reinventing it.

> 
> Thanks.
> Peter
> 
Re: [PATCH v4 11/25] scsi: ufs: mediatek: Rework probe function
Posted by Peter Wang (王信友) 1 month ago
On Thu, 2026-01-08 at 10:14 +0100, Nicolas Frattaroli wrote:
> Because it's not described by the binding, and appears to be a
> downstream hack to work around not having the reset controller
> properly described and referred to with a `resets` property.
> 
> Even if you were to use `ti,syscon-reset` to describe a reset
> controller, the UFS controller driver should not be searching
> for this compatible. It should access the reset through the
> reset API. The common reset code can then take care of probe
> ordering without every driver reinventing it.
> 
> 

Can we supplement description in binding and revise the reset
controller flow?
Because if we remove it directly, it may cause problems when an error
occurs?

Thanks.
Peter