Skip to contentSkip to conversation

RK3399: PCie Phy using new helper function

PATCH Superseded Anand Moon 2 participants 9 messages Oct 7, 2024 – Oct 9, 2024

Subsystems (2)
Arm Rockchip Generic PHY framework

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Anand Moon

author
#0 lore raw

v2 cover RK3399: PCie Phy using new helper function

Earlier patch series were part of the clk and reset series [1]

[1] https://lore.kernel.org/all/[email protected]/

Now I treed to split the series for phy changes.

Latest clk and reset changes are below

[2] https://lore.kernel.org/all/[email protected]/

v2: Fix some typo in the subjects.

Thanks
-Anand

Anand Moon (3):
phy: rockchip-pcie: Simplify error handling with dev_err_probe()
phy: rockchip-pcie: Use devm_clk_get_enabled() helper
phy: rockchip-pcie: Use regmap_read_poll_timeout() for PCIe reference
clk PLL status

 drivers/phy/rockchip/phy-rockchip-pcie.c | 92 +++++++-----------------
 1 file changed, 25 insertions(+), 67 deletions(-)

base-commit: 8f602276d3902642fdc3429b548d73c745446601

Signature · 1 line
2.44.0
author
#1 lore raw

PATCH v2 1/3 phy: rockchip-pcie: Simplify error handling with dev_err_probe()

Use the dev_err_probe() helper to simplify error handling during probe. This also handle scenario, when -EDEFER is returned and useless error is printed.

Signed-off-by Anand Moon <[email protected]>

---
v2: None
v1: None

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/phy/rockchip/phy-rockchip-pcie.c +3 -6
index 51cc5ece0e63..51e636a1ce33 100644
@@ -371,12 +371,9 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev)
 	mutex_init(&rk_phy->pcie_mutex);
 
 	rk_phy->phy_rst = devm_reset_control_get(dev, "phy");
-	if (IS_ERR(rk_phy->phy_rst)) {
-		if (PTR_ERR(rk_phy->phy_rst) != -EPROBE_DEFER)
-			dev_err(dev,
-				"missing phy property for reset controller\n");
-		return PTR_ERR(rk_phy->phy_rst);
-	}
+	if (IS_ERR(rk_phy->phy_rst))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rk_phy->phy_rst),
+				     "missing phy property for reset controller\n");
 
 	rk_phy->clk_pciephy_ref = devm_clk_get(dev, "refclk");
 	if (IS_ERR(rk_phy->clk_pciephy_ref)) {
Signature · 1 line
2.44.0
author
#2 lore raw

PATCH v2 2/3 phy: rockchip-pcie: Use devm_clk_get_enabled() helper

In reply to Anand Moon

Use devm_clk_get_enabled() instead of devm_clk_get() to make the code cleaner and avoid calling clk_disable_unprepare(), as this is exactly what this function does. Use the dev_err_probe() helper to simplify error handling during probe.

Signed-off-by Anand Moon <[email protected]>

---
v2: Change the subject drop: Change to use/Use
v1: New patch in this series

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 25 ++++++------------------
 1 file changed, 6 insertions(+), 19 deletions(-)
drivers/phy/rockchip/phy-rockchip-pcie.c +6 -19
index 51e636a1ce33..a1b4b0323e9d 100644
@@ -277,26 +277,16 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 	if (rk_phy->init_cnt++)
 		goto err_out;
 
-	err = clk_prepare_enable(rk_phy->clk_pciephy_ref);
-	if (err) {
-		dev_err(&phy->dev, "Fail to enable pcie ref clock.\n");
-		goto err_refclk;
-	}
-
 	err = reset_control_assert(rk_phy->phy_rst);
 	if (err) {
 		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
-		goto err_reset;
+		goto err_out;
 	}
 
-err_out:
 	mutex_unlock(&rk_phy->pcie_mutex);
 	return 0;
 
-err_reset:
-
-	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
-err_refclk:
+err_out:
 	rk_phy->init_cnt--;
 	mutex_unlock(&rk_phy->pcie_mutex);
 	return err;

@@ -312,8 +302,6 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
 	if (--rk_phy->init_cnt)
 		goto err_init_cnt;
 
-	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
-
 err_init_cnt:
 	mutex_unlock(&rk_phy->pcie_mutex);
 	return 0;

@@ -375,11 +363,10 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(rk_phy->phy_rst),
 				     "missing phy property for reset controller\n");
 
-	rk_phy->clk_pciephy_ref = devm_clk_get(dev, "refclk");
-	if (IS_ERR(rk_phy->clk_pciephy_ref)) {
-		dev_err(dev, "refclk not found.\n");
-		return PTR_ERR(rk_phy->clk_pciephy_ref);
-	}
+	rk_phy->clk_pciephy_ref = devm_clk_get_enabled(dev, "refclk");
+	if (IS_ERR(rk_phy->clk_pciephy_ref))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rk_phy->clk_pciephy_ref),
+				     "failed to get phyclk\n");
 
 	/* parse #phy-cells to see if it's legacy PHY model */
 	if (of_property_read_u32(dev->of_node, "#phy-cells", &phy_num))
Signature · 1 line
2.44.0
#3 lore raw

Re: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper

Hi Anand,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/phy-rockchip-pcie-Simplify-error-handling-with-dev_err_probe/20241007-115910
base:   8f602276d3902642fdc3429b548d73c745446601
patch link:    https://lore.kernel.org/r/20241007035616.2701-3-linux.amoon%40gmail.com
patch subject: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper
config: loongarch-randconfig-r071-20241009 (https://download.01.org/0day-ci/archive/20241009/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags

| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

smatch warnings:
drivers/phy/rockchip/phy-rockchip-pcie.c:278 rockchip_pcie_phy_init() warn: missing error code 'err'

vim +/err +278 drivers/phy/rockchip/phy-rockchip-pcie.c

fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  269  static int rockchip_pcie_phy_init(struct phy *phy)
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  270  {
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  271  	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  272  	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  273  	int err = 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  274  
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  275  	mutex_lock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  276  
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  277  	if (rk_phy->init_cnt++)
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19 @278  		goto err_out;

Originally, this path just unlocked at returned zero.

90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  279  
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  280  	err = reset_control_assert(rk_phy->phy_rst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  281  	if (err) {
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  282  		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  283  		goto err_out;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  284  	}
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  285  
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  286  	mutex_unlock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  287  	return 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  288  
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  289  err_out:
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  290  	rk_phy->init_cnt--;

Now it decrements the counter so presumably it leads to an underflow/use after free.

90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  291  	mutex_unlock(&rk_phy->pcie_mutex);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  292  	return err;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  293  }
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
author
#4 lore raw

Re: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper

Hi Dan,

Thanks for the report.

Quoted 12 lines · Dan Carpenter

Hi Anand,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/phy-rockchip-pcie-Simplify-error-handling-with-dev_err_probe/20241007-115910
base:   8f602276d3902642fdc3429b548d73c745446601
patch link:    https://lore.kernel.org/r/20241007035616.2701-3-linux.amoon%40gmail.com
patch subject: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper
config: loongarch-randconfig-r071-20241009 (https://download.01.org/0day-ci/archive/20241009/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
Quoted 9 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags

| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

smatch warnings:
drivers/phy/rockchip/phy-rockchip-pcie.c:278 rockchip_pcie_phy_init() warn: missing error code 'err'

All the functions in this file explicitly return 0 instead of err, I will fix this.

Quoted 28 lines

vim +/err +278 drivers/phy/rockchip/phy-rockchip-pcie.c

fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  269  static int rockchip_pcie_phy_init(struct phy *phy)
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  270  {
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  271      struct phy_pcie_instance *inst = phy_get_drvdata(phy);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  272      struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  273      int err = 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  274
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  275      mutex_lock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  276
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  277      if (rk_phy->init_cnt++)
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19 @278              goto err_out;

Originally, this path just unlocked at returned zero.

90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  279
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  280      err = reset_control_assert(rk_phy->phy_rst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  281      if (err) {
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  282              dev_err(&phy->dev, "assert phy_rst err %d\n", err);
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  283              goto err_out;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  284      }
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  285
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  286      mutex_unlock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  287      return 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  288
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  289  err_out:
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  290      rk_phy->init_cnt--;
Quoted 2 lines

Now it decrements the counter so presumably it leads to an underflow/use after free.

I was planning to replace the mutex_lock / mutex_unlock with guard(mutex)(&rk_phy->pcie_mutex) in the follow up patch. I will add this in the next revision.

Quoted 5 lines
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  291      mutex_unlock(&rk_phy->pcie_mutex);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  292      return err;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  293  }

Here are my modified changes on top of my changes for the review process. -----8<----------8<----------8<----------8<----------8<----------8<-----

(quoted diff) +0 -0

b/drivers/phy/rockchip/phy-rockchip-pcie.c
index 2c4d6f68f02a..09685dc3fe17 100644

drivers/phy/rockchip/phy-rockchip-pcie.c +7 -8
@@ -248,20 +248,19 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 
        mutex_lock(&rk_phy->pcie_mutex);
 
-       if (rk_phy->init_cnt++)
-               goto err_out;
+       if (rk_phy->init_cnt++) {
+               mutex_unlock(&rk_phy->pcie_mutex);
+               return err;
+       }
 
        err = reset_control_assert(rk_phy->phy_rst);
        if (err) {
                dev_err(&phy->dev, "assert phy_rst err %d\n", err);
-               goto err_out;
+               rk_phy->init_cnt--;
+               mutex_unlock(&rk_phy->pcie_mutex);
+               return err;
        }
 
-       mutex_unlock(&rk_phy->pcie_mutex);
-       return 0;
-
-err_out:
-       rk_phy->init_cnt--;
        mutex_unlock(&rk_phy->pcie_mutex);
        return err;
 }

Thanks
-Anand

#5 lore raw

Re: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper

Quoted 58 lines · Anand Moon

Hi Dan,

Thanks for the report.

Quoted 12 lines · Dan Carpenter

Hi Anand,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/phy-rockchip-pcie-Simplify-error-handling-with-dev_err_probe/20241007-115910
base:   8f602276d3902642fdc3429b548d73c745446601
patch link:    https://lore.kernel.org/r/20241007035616.2701-3-linux.amoon%40gmail.com
patch subject: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper
config: loongarch-randconfig-r071-20241009 (https://download.01.org/0day-ci/archive/20241009/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
Quoted 9 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags

| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

smatch warnings:
drivers/phy/rockchip/phy-rockchip-pcie.c:278 rockchip_pcie_phy_init() warn: missing error code 'err'

All the functions in this file explicitly return 0 instead of err, I will fix this.

Quoted 28 lines

vim +/err +278 drivers/phy/rockchip/phy-rockchip-pcie.c

fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  269  static int rockchip_pcie_phy_init(struct phy *phy)
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  270  {
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  271      struct phy_pcie_instance *inst = phy_get_drvdata(phy);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  272      struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  273      int err = 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  274
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  275      mutex_lock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  276
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  277      if (rk_phy->init_cnt++)
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19 @278              goto err_out;

Originally, this path just unlocked at returned zero.

90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  279
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  280      err = reset_control_assert(rk_phy->phy_rst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  281      if (err) {
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  282              dev_err(&phy->dev, "assert phy_rst err %d\n", err);
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  283              goto err_out;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  284      }
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  285
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  286      mutex_unlock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  287      return 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  288
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  289  err_out:
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  290      rk_phy->init_cnt--;
Quoted 13 lines
Quoted 2 lines

Now it decrements the counter so presumably it leads to an underflow/use after free.

I was planning to replace the mutex_lock / mutex_unlock with guard(mutex)(&rk_phy->pcie_mutex) in the follow up patch. I will add this in the next revision.

Quoted 5 lines
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  291      mutex_unlock(&rk_phy->pcie_mutex);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  292      return err;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  293  }

Thanks!

Quoted 8 lines

Here are my modified changes on top of my changes for the review process. -----8<----------8<----------8<----------8<----------8<----------8<-----

drivers/phy/rockchip/phy-rockchip-pcie.c +0 -0
index 2c4d6f68f02a..09685dc3fe17 100644

drivers/phy/rockchip/phy-rockchip-pcie.c static int rockchip_pcie_phy_init(struct phy *phy) · line 253

Quoted 8 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +3 -2
@@ -248,20 +248,19 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 
        mutex_lock(&rk_phy->pcie_mutex);
 
-       if (rk_phy->init_cnt++)
-               goto err_out;
+       if (rk_phy->init_cnt++) {
+               mutex_unlock(&rk_phy->pcie_mutex);
+               return err;

Please make this a return 0. It's faster to not have to look up what a variable is.

Quoted 8 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +3 -1
+       }
 
        err = reset_control_assert(rk_phy->phy_rst);
        if (err) {
                dev_err(&phy->dev, "assert phy_rst err %d\n", err);
-               goto err_out;
+               rk_phy->init_cnt--;
+               mutex_unlock(&rk_phy->pcie_mutex);

drivers/phy/rockchip/phy-rockchip-pcie.c static int rockchip_pcie_phy_init(struct phy *phy) · line 265

Quoted 10 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +1 -5
+               return err;
        }
 
-       mutex_unlock(&rk_phy->pcie_mutex);
-       return 0;
-
-err_out:
-       rk_phy->init_cnt--;
        mutex_unlock(&rk_phy->pcie_mutex);
        return err;

return 0; here too.

regards,
dan carpenter

author
#6 lore raw

Re: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper

Hi Dan,

Quoted 87 lines · Dan Carpenter
Quoted 58 lines · Anand Moon

Hi Dan,

Thanks for the report.

Quoted 12 lines · Dan Carpenter

Hi Anand,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/phy-rockchip-pcie-Simplify-error-handling-with-dev_err_probe/20241007-115910
base:   8f602276d3902642fdc3429b548d73c745446601
patch link:    https://lore.kernel.org/r/20241007035616.2701-3-linux.amoon%40gmail.com
patch subject: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper
config: loongarch-randconfig-r071-20241009 (https://download.01.org/0day-ci/archive/20241009/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
Quoted 9 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags

| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

smatch warnings:
drivers/phy/rockchip/phy-rockchip-pcie.c:278 rockchip_pcie_phy_init() warn: missing error code 'err'

All the functions in this file explicitly return 0 instead of err, I will fix this.

Quoted 28 lines

vim +/err +278 drivers/phy/rockchip/phy-rockchip-pcie.c

fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  269  static int rockchip_pcie_phy_init(struct phy *phy)
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  270  {
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  271      struct phy_pcie_instance *inst = phy_get_drvdata(phy);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  272      struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  273      int err = 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  274
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  275      mutex_lock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  276
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  277      if (rk_phy->init_cnt++)
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19 @278              goto err_out;

Originally, this path just unlocked at returned zero.

90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  279
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  280      err = reset_control_assert(rk_phy->phy_rst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  281      if (err) {
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  282              dev_err(&phy->dev, "assert phy_rst err %d\n", err);
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  283              goto err_out;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  284      }
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  285
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  286      mutex_unlock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  287      return 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  288
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  289  err_out:
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  290      rk_phy->init_cnt--;
Quoted 13 lines
Quoted 2 lines

Now it decrements the counter so presumably it leads to an underflow/use after free.

I was planning to replace the mutex_lock / mutex_unlock with guard(mutex)(&rk_phy->pcie_mutex) in the follow up patch. I will add this in the next revision.

Quoted 5 lines
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  291      mutex_unlock(&rk_phy->pcie_mutex);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  292      return err;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  293  }

Thanks!

Quoted 11 lines

Here are my modified changes on top of my changes for the review process. -----8<----------8<----------8<----------8<----------8<----------8<-----

drivers/phy/rockchip/phy-rockchip-pcie.c +0 -0
index 2c4d6f68f02a..09685dc3fe17 100644
@@ -248,20 +248,19 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 
        mutex_lock(&rk_phy->pcie_mutex);
Quoted 9 lines

drivers/phy/rockchip/phy-rockchip-pcie.c static int rockchip_pcie_phy_init(struct phy *phy) · line 253

Quoted 5 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +3 -2
-       if (rk_phy->init_cnt++)
-               goto err_out;
+       if (rk_phy->init_cnt++) {
+               mutex_unlock(&rk_phy->pcie_mutex);
+               return err;

Please make this a return 0. It's faster to not have to look up what a variable is.

Ok.

Quoted 11 lines
Quoted 11 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +4 -1
+       }
 
        err = reset_control_assert(rk_phy->phy_rst);
        if (err) {
                dev_err(&phy->dev, "assert phy_rst err %d\n", err);
-               goto err_out;
+               rk_phy->init_cnt--;
+               mutex_unlock(&rk_phy->pcie_mutex);
+               return err;
        }
Quoted 10 lines

drivers/phy/rockchip/phy-rockchip-pcie.c static int rockchip_pcie_phy_init(struct phy *phy) · line 265

Quoted 7 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +0 -5
-       mutex_unlock(&rk_phy->pcie_mutex);
-       return 0;
-
-err_out:
-       rk_phy->init_cnt--;
        mutex_unlock(&rk_phy->pcie_mutex);
        return err;

return 0; here too.

Ok. I will update the patch.

Quoted 3 lines

regards,
dan carpenter

Thanks
-Anand

author
#7 lore raw

Re: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper

Hi Dan,

Quoted 100 lines · Anand Moon

Hi Dan,

Quoted 87 lines · Dan Carpenter
Quoted 58 lines · Anand Moon

Hi Dan,

Thanks for the report.

Quoted 12 lines · Dan Carpenter

Hi Anand,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/phy-rockchip-pcie-Simplify-error-handling-with-dev_err_probe/20241007-115910
base:   8f602276d3902642fdc3429b548d73c745446601
patch link:    https://lore.kernel.org/r/20241007035616.2701-3-linux.amoon%40gmail.com
patch subject: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper
config: loongarch-randconfig-r071-20241009 (https://download.01.org/0day-ci/archive/20241009/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
Quoted 9 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags

| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

smatch warnings:
drivers/phy/rockchip/phy-rockchip-pcie.c:278 rockchip_pcie_phy_init() warn: missing error code 'err'

All the functions in this file explicitly return 0 instead of err, I will fix this.

Quoted 28 lines

vim +/err +278 drivers/phy/rockchip/phy-rockchip-pcie.c

fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  269  static int rockchip_pcie_phy_init(struct phy *phy)
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  270  {
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  271      struct phy_pcie_instance *inst = phy_get_drvdata(phy);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  272      struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  273      int err = 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  274
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  275      mutex_lock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  276
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  277      if (rk_phy->init_cnt++)
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19 @278              goto err_out;

Originally, this path just unlocked at returned zero.

90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  279
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  280      err = reset_control_assert(rk_phy->phy_rst);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  281      if (err) {
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  282              dev_err(&phy->dev, "assert phy_rst err %d\n", err);
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  283              goto err_out;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  284      }
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  285
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  286      mutex_unlock(&rk_phy->pcie_mutex);
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  287      return 0;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  288
3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  289  err_out:
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  290      rk_phy->init_cnt--;
Quoted 13 lines
Quoted 2 lines

Now it decrements the counter so presumably it leads to an underflow/use after free.

I was planning to replace the mutex_lock / mutex_unlock with guard(mutex)(&rk_phy->pcie_mutex) in the follow up patch. I will add this in the next revision.

Quoted 5 lines
90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  291      mutex_unlock(&rk_phy->pcie_mutex);
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  292      return err;
fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  293  }

Thanks!

Quoted 11 lines

Here are my modified changes on top of my changes for the review process. -----8<----------8<----------8<----------8<----------8<----------8<-----

drivers/phy/rockchip/phy-rockchip-pcie.c +0 -0
index 2c4d6f68f02a..09685dc3fe17 100644
@@ -248,20 +248,19 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 
        mutex_lock(&rk_phy->pcie_mutex);
Quoted 9 lines

drivers/phy/rockchip/phy-rockchip-pcie.c static int rockchip_pcie_phy_init(struct phy *phy) · line 253

Quoted 5 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +3 -2
-       if (rk_phy->init_cnt++)
-               goto err_out;
+       if (rk_phy->init_cnt++) {
+               mutex_unlock(&rk_phy->pcie_mutex);
+               return err;

Please make this a return 0. It's faster to not have to look up what a variable is.

Quoted 21 lines

Ok.

Quoted 20 lines
Quoted 8 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +3 -1
+       }
 
        err = reset_control_assert(rk_phy->phy_rst);
        if (err) {
                dev_err(&phy->dev, "assert phy_rst err %d\n", err);
-               goto err_out;
+               rk_phy->init_cnt--;
+               mutex_unlock(&rk_phy->pcie_mutex);

drivers/phy/rockchip/phy-rockchip-pcie.c static int rockchip_pcie_phy_init(struct phy *phy) · line 265

Quoted 10 lines
drivers/phy/rockchip/phy-rockchip-pcie.c +1 -5
+               return err;
        }
 
-       mutex_unlock(&rk_phy->pcie_mutex);
-       return 0;
-
-err_out:
-       rk_phy->init_cnt--;
        mutex_unlock(&rk_phy->pcie_mutex);
        return err;

return 0; here too.

The above warning " missing error code 'err'"
so it's correct to return err. here.

Quoted 6 lines
Quoted 1 line

Ok. I will update the patch.

Quoted 3 lines

regards,
dan carpenter

Thanks
-Anand

author
#8 lore raw

PATCH v2 3/3 phy: rockchip-pcie: Use regmap_read_poll_timeout() for PCIe reference clk PLL status

In reply to Anand Moon

Replace open-coded phy PCIe reference clk PLL status polling with regmap_read_poll_timeout API. This change simplifies the code without altering functionality.

Signed-off-by Anand Moon <[email protected]>

---
v2: Fix the subject, add the missing () in the function name,
Fix the typo reference
v1: None.

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 56 +++++++-----------------
 1 file changed, 15 insertions(+), 41 deletions(-)
drivers/phy/rockchip/phy-rockchip-pcie.c +15 -41
index a1b4b0323e9d..2c4d6f68f02a 100644
@@ -162,7 +162,6 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
 	int err = 0;
 	u32 status;
-	unsigned long timeout;
 
 	mutex_lock(&rk_phy->pcie_mutex);
 

@@ -191,21 +190,11 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	 * so we make it large enough here. And we use loop-break
 	 * method which should not be harmful.
 	 */
-	timeout = jiffies + msecs_to_jiffies(1000);
-
-	err = -EINVAL;
-	while (time_before(jiffies, timeout)) {
-		regmap_read(rk_phy->reg_base,
-			    rk_phy->phy_data->pcie_status,
-			    &status);
-		if (status & PHY_PLL_LOCKED) {
-			dev_dbg(&phy->dev, "pll locked!\n");
-			err = 0;
-			break;
-		}
-		msleep(20);
-	}
-
+	err = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->phy_data->pcie_status,
+				       status,
+				       status & PHY_PLL_LOCKED,
+				       200, 100000);
 	if (err) {
 		dev_err(&phy->dev, "pll lock timeout!\n");
 		goto err_pll_lock;

@@ -214,19 +203,11 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	phy_wr_cfg(rk_phy, PHY_CFG_CLK_TEST, PHY_CFG_SEPE_RATE);
 	phy_wr_cfg(rk_phy, PHY_CFG_CLK_SCC, PHY_CFG_PLL_100M);
 
-	err = -ETIMEDOUT;
-	while (time_before(jiffies, timeout)) {
-		regmap_read(rk_phy->reg_base,
-			    rk_phy->phy_data->pcie_status,
-			    &status);
-		if (!(status & PHY_PLL_OUTPUT)) {
-			dev_dbg(&phy->dev, "pll output enable done!\n");
-			err = 0;
-			break;
-		}
-		msleep(20);
-	}
-
+	err = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->phy_data->pcie_status,
+				       status,
+				       !(status & PHY_PLL_OUTPUT),
+				       200, 100000);
 	if (err) {
 		dev_err(&phy->dev, "pll output enable timeout!\n");
 		goto err_pll_lock;

@@ -236,19 +217,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 		     HIWORD_UPDATE(PHY_CFG_PLL_LOCK,
 				   PHY_CFG_ADDR_MASK,
 				   PHY_CFG_ADDR_SHIFT));
-	err = -EINVAL;
-	while (time_before(jiffies, timeout)) {
-		regmap_read(rk_phy->reg_base,
-			    rk_phy->phy_data->pcie_status,
-			    &status);
-		if (status & PHY_PLL_LOCKED) {
-			dev_dbg(&phy->dev, "pll relocked!\n");
-			err = 0;
-			break;
-		}
-		msleep(20);
-	}
 
+	err = regmap_read_poll_timeout(rk_phy->reg_base,
+				       rk_phy->phy_data->pcie_status,
+				       status,
+				       status & PHY_PLL_LOCKED,
+				       200, 100000);
 	if (err) {
 		dev_err(&phy->dev, "pll relock timeout!\n");
 		goto err_pll_lock;
Signature · 1 line
2.44.0