[PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function

Nicolas Frattaroli posted 24 patches 1 month ago
There is a newer version of this series
[PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by Nicolas Frattaroli 1 month 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.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
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 954d6768aa64..fc72bf54ec2a 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -2382,74 +2382,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
@@ -2458,9 +2433,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 v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by Krzysztof Kozlowski 1 month ago
On 08/01/2026 11:49, Nicolas Frattaroli wrote:
> Remove the ti,syscon-reset cruft.

Please provide here reason, e.g. undocumented ABI. Normally I would ask
about ABI impact, but considering this is was just copied from some
downstream code I would just not care.

Mediatek should really stop and rethink their upstream process.

Best regards,
Krzysztof
Re: [PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by Peter Wang (王信友) 1 month ago
On Thu, 2026-01-08 at 13:25 +0100, Krzysztof Kozlowski wrote:
> 
> Please provide here reason, e.g. undocumented ABI. Normally I would
> ask
> about ABI impact, but considering this is was just copied from some
> downstream code I would just not care.
> 

Is it sufficient for us to supplement the ABI document?
This ABI might affect the ability to reset and recover after 
an UFS error in upstream world.

> Mediatek should really stop and rethink their upstream process.
> 
> Best regards,
> Krzysztof

Yes, you are right.
MediaTek does have some shortcomings in the upstream process.
We will make improvements.

Thanks.
Peter
Re: [PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by Krzysztof Kozlowski 1 month ago
On 09/01/2026 07:22, Peter Wang (王信友) wrote:
> On Thu, 2026-01-08 at 13:25 +0100, Krzysztof Kozlowski wrote:
>>
>> Please provide here reason, e.g. undocumented ABI. Normally I would
>> ask
>> about ABI impact, but considering this is was just copied from some
>> downstream code I would just not care.
>>
> 
> Is it sufficient for us to supplement the ABI document?
> This ABI might affect the ability to reset and recover after 
> an UFS error in upstream world.


In normal case yes, but I cannot imagine arguments justifying your usage
of TI properties. Basically it would not pass review.

Best regards,
Krzysztof
Re: [PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by Peter Wang (王信友) 1 month ago
On Fri, 2026-01-09 at 08:24 +0100, Krzysztof Kozlowski wrote:
> On 09/01/2026 07:22, Peter Wang (王信友) wrote:
> > 
> > 
> > Is it sufficient for us to supplement the ABI document?
> > This ABI might affect the ability to reset and recover after 
> > an UFS error in upstream world.
> 
> 
> In normal case yes, but I cannot imagine arguments justifying your
> usage
> of TI properties. Basically it would not pass review.
> 
> Best regards,
> Krzysztof


Yes, this part is indeed because MediaTek’s reset hardware 
implementation is the same as TI’s. That’s why we used “compatible” 
instead of actually implementing MediaTek’s own reset controller.
So, are you suggesting that we upstream a MediaTek reset controller,
even though the code is almost identical to TI’s?

Thanks
Peter

Re: [PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by Krzysztof Kozlowski 1 month ago
On 09/01/2026 09:38, Peter Wang (王信友) wrote:
> On Fri, 2026-01-09 at 08:24 +0100, Krzysztof Kozlowski wrote:
>> On 09/01/2026 07:22, Peter Wang (王信友) wrote:
>>>
>>>
>>> Is it sufficient for us to supplement the ABI document?
>>> This ABI might affect the ability to reset and recover after 
>>> an UFS error in upstream world.
>>
>>
>> In normal case yes, but I cannot imagine arguments justifying your
>> usage
>> of TI properties. Basically it would not pass review.
>>
>> Best regards,
>> Krzysztof
> 
> 
> Yes, this part is indeed because MediaTek’s reset hardware 
> implementation is the same as TI’s. That’s why we used “compatible” 
> instead of actually implementing MediaTek’s own reset controller.

So that's another purely downstream code. Additionally very poor quality
downstream code.

> So, are you suggesting that we upstream a MediaTek reset controller,
> even though the code is almost identical to TI’s?

If you ask about DT, this is already answered in writing bindings
document. You cannot use someone else's compatible. Was also re-iterated
on mailing list bazillions of times.

Best regards,
Krzysztof
Re: [PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by Peter Wang (王信友) 1 month ago
On Fri, 2026-01-09 at 09:43 +0100, Krzysztof Kozlowski wrote:
> On 09/01/2026 09:38, Peter Wang (王信友) wrote:
> > On Fri, 2026-01-09 at 08:24 +0100, Krzysztof Kozlowski wrote:
> > > On 09/01/2026 07:22, Peter Wang (王信友) wrote:
> > > > 
> > > > 
> > > > Is it sufficient for us to supplement the ABI document?
> > > > This ABI might affect the ability to reset and recover after 
> > > > an UFS error in upstream world.
> > > 
> > > 
> > > In normal case yes, but I cannot imagine arguments justifying
> > > your
> > > usage
> > > of TI properties. Basically it would not pass review.
> > > 
> > > Best regards,
> > > Krzysztof
> > 
> > 
> > Yes, this part is indeed because MediaTek’s reset hardware 
> > implementation is the same as TI’s. That’s why we used “compatible”
> > instead of actually implementing MediaTek’s own reset controller.
> 
> So that's another purely downstream code. Additionally very poor
> quality
> downstream code.
> 
> > So, are you suggesting that we upstream a MediaTek reset
> > controller,
> > even though the code is almost identical to TI’s?
> 
> If you ask about DT, this is already answered in writing bindings
> document. You cannot use someone else's compatible. Was also re-
> iterated
> on mailing list bazillions of times.
> 
> Best regards,
> Krzysztof

Okay, we will correct these incorrect usages.

Thanks
Peter



Re: [PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by AngeloGioacchino Del Regno 4 weeks ago
Il 09/01/26 10:16, Peter Wang (王信友) ha scritto:
> On Fri, 2026-01-09 at 09:43 +0100, Krzysztof Kozlowski wrote:
>> On 09/01/2026 09:38, Peter Wang (王信友) wrote:
>>> On Fri, 2026-01-09 at 08:24 +0100, Krzysztof Kozlowski wrote:
>>>> On 09/01/2026 07:22, Peter Wang (王信友) wrote:
>>>>>
>>>>>
>>>>> Is it sufficient for us to supplement the ABI document?
>>>>> This ABI might affect the ability to reset and recover after
>>>>> an UFS error in upstream world.
>>>>
>>>>
>>>> In normal case yes, but I cannot imagine arguments justifying
>>>> your
>>>> usage
>>>> of TI properties. Basically it would not pass review.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>>
>>> Yes, this part is indeed because MediaTek’s reset hardware
>>> implementation is the same as TI’s.

No, MediaTek's reset hardware implementation is not the same as Texas Instruments.
It was *very similar* to TI in the past (years ago, around the MT6795 Helio
generation times).

MediaTek's reset controller - by hardware - is definitely different from the one
found in TI SoCs.

Regards,
Angelo

>>> That’s why we used “compatible”
>>> instead of actually implementing MediaTek’s own reset controller.
>>
>> So that's another purely downstream code. Additionally very poor
>> quality
>> downstream code.
>>
>>> So, are you suggesting that we upstream a MediaTek reset
>>> controller,
>>> even though the code is almost identical to TI’s?
>>
>> If you ask about DT, this is already answered in writing bindings
>> document. You cannot use someone else's compatible. Was also re-
>> iterated
>> on mailing list bazillions of times.
>>
>> Best regards,
>> Krzysztof
> 
> Okay, we will correct these incorrect usages.
> 
> Thanks
> Peter
> 
> 
> 


Re: [PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by Peter Wang (王信友) 3 weeks, 6 days ago
On Mon, 2026-01-12 at 16:02 +0100, AngeloGioacchino Del Regno wrote:
> No, MediaTek's reset hardware implementation is not the same as Texas
> Instruments.
> It was *very similar* to TI in the past (years ago, around the MT6795
> Helio
> generation times).
> 
> MediaTek's reset controller - by hardware - is definitely different
> from the one
> found in TI SoCs.
> 
> Regards,
> Angelo

I did not notice this change.
Will you be helping to upstream MediaTek's reset controller instead of
TI's?

Thanks
Peter

Re: [PATCH v5 11/24] scsi: ufs: mediatek: Rework probe function
Posted by AngeloGioacchino Del Regno 2 weeks, 6 days ago
Il 13/01/26 08:26, Peter Wang (王信友) ha scritto:
> On Mon, 2026-01-12 at 16:02 +0100, AngeloGioacchino Del Regno wrote:
>> No, MediaTek's reset hardware implementation is not the same as Texas
>> Instruments.
>> It was *very similar* to TI in the past (years ago, around the MT6795
>> Helio
>> generation times).
>>
>> MediaTek's reset controller - by hardware - is definitely different
>> from the one
>> found in TI SoCs.
>>
>> Regards,
>> Angelo
> 
> I did not notice this change.
> Will you be helping to upstream MediaTek's reset controller instead of
> TI's?
> 

The main reset controllers are already integrated in clock drivers since
... well, years ago.

If there's any additional reset controller that is missing, and special to
UFS, and that's not in the UFS clock driver, yes we can upstream that.

Cheers,
Angelo

> Thanks
> Peter
>