Message-ID: <5025239.GXAFRqVoOG@workhorse>
Date: Mon, 10 Nov 2025 10:23:44 +0100
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Peter Wang (王信友) <peter.wang@...iatek.com>,
 Chunfeng Yun (云春峰) <Chunfeng.Yun@...iatek.com>,
 "kishon@...nel.org" <kishon@...nel.org>,
 "avri.altman@....com" <avri.altman@....com>,
 "bvanassche@....org" <bvanassche@....org>,
 "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
 "broonie@...nel.org" <broonie@...nel.org>,
 "alim.akhtar@...sung.com" <alim.akhtar@...sung.com>,
 "chu.stanley@...il.com" <chu.stanley@...il.com>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
 "robh@...nel.org" <robh@...nel.org>,
 "James.Bottomley@...senPartnership.com"
 <James.Bottomley@...senpartnership.com>,
 "lgirdwood@...il.com" <lgirdwood@...il.com>,
 "vkoul@...nel.org" <vkoul@...nel.org>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Chaotian Jing (井朝天) <Chaotian.Jing@...iatek.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "kernel@...labora.com" <kernel@...labora.com>,
 Louis-Alexis Eyraud <louisalexis.eyraud@...labora.com>,
 "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>
Subject: Re: [PATCH v3 10/24] scsi: ufs: mediatek: Rework probe function

On Wednesday, 5 November 2025 07:28:39 Central European Standard Time Chaotian Jing (井朝天) wrote:
> On Thu, 2025-10-23 at 21:49 +0200, Nicolas Frattaroli wrote:
> > Remove the ti,syscon-reset cruft.
> > 
> > Make PHY mandatory. All the compatibles supported by the binding make
> > it
> > mandatory.
> > 
> why make the PHY mandatory ? note that not all of MediaTek SoCs have
> the PHY node.

Why don't they have the PHY node? Does the hardware not have a PHY?

The mainline binding makes the phys property mandatory. If you have
downstream device trees that don't have the PHY node properly
described in the DT even though the PHY exists, then that is not a
thing the mainline kernel should support.

If the hardware really doesn't have a PHY, which would surprise me,
then the binding should properly document this, so that the DT checks
pass without warnings.

> > 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@...labora.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 9c0ac72d6e43..889a1d58a041 100644
> > --- a/drivers/ufs/host/ufs-mediatek.c
> > +++ b/drivers/ufs/host/ufs-mediatek.c
> > @@ -2353,74 +2353,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
> > @@ -2429,9 +2404,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;
> >  }
> >  
> > 
> 

Powered by blists - more mailing lists