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 <linux.amoon@gmail.com>
---
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(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
index 51e636a1ce33..a1b4b0323e9d 100644
--- a/drivers/phy/rockchip/phy-rockchip-pcie.c
+++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
@@ -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))
--
2.44.0Hi 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/202410092019.vGogfPIO-lkp@intel.com/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 <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202410092019.vGogfPIO-lkp@intel.com/
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 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Dan,
Thanks for the report.
On Wed, 9 Oct 2024 at 17:55, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> 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/202410092019.vGogfPIO-lkp@intel.com/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 <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202410092019.vGogfPIO-lkp@intel.com/
>
> 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.
> 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.
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.
>
> 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<-----
diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c
b/drivers/phy/rockchip/phy-rockchip-pcie.c
index 2c4d6f68f02a..09685dc3fe17 100644
--- a/drivers/phy/rockchip/phy-rockchip-pcie.c
+++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
@@ -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
On Wed, Oct 09, 2024 at 07:59:38PM +0530, Anand Moon wrote:
> Hi Dan,
>
> Thanks for the report.
>
> On Wed, 9 Oct 2024 at 17:55, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > 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/202410092019.vGogfPIO-lkp@intel.com/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 <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202410092019.vGogfPIO-lkp@intel.com/
> >
> > 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.
>
> > 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.
>
> 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.
>
> >
> > 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!
> Here are my modified changes on top of my changes for the review process.
> -----8<----------8<----------8<----------8<----------8<----------8<-----
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c
> b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index 2c4d6f68f02a..09685dc3fe17 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -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.
> + }
>
> 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;
return 0; here too.
regards,
dan carpenter
Hi Dan,
On Wed, 9 Oct 2024 at 20:19, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Wed, Oct 09, 2024 at 07:59:38PM +0530, Anand Moon wrote:
> > Hi Dan,
> >
> > Thanks for the report.
> >
> > On Wed, 9 Oct 2024 at 17:55, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >
> > > 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/202410092019.vGogfPIO-lkp@intel.com/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 <lkp@intel.com>
> > > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > | Closes: https://lore.kernel.org/r/202410092019.vGogfPIO-lkp@intel.com/
> > >
> > > 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.
> >
> > > 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.
> >
> > 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.
> >
> > >
> > > 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!
>
> > Here are my modified changes on top of my changes for the review process.
> > -----8<----------8<----------8<----------8<----------8<----------8<-----
> > diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c
> > b/drivers/phy/rockchip/phy-rockchip-pcie.c
> > index 2c4d6f68f02a..09685dc3fe17 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> > @@ -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.
>
Ok.
> > + }
> >
> > 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;
>
> return 0; here too.
>
Ok. I will update the patch.
> regards,
> dan carpenter
>
Thanks
-Anand
Hi Dan,
On Wed, 9 Oct 2024 at 21:38, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Dan,
>
> On Wed, 9 Oct 2024 at 20:19, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Wed, Oct 09, 2024 at 07:59:38PM +0530, Anand Moon wrote:
> > > Hi Dan,
> > >
> > > Thanks for the report.
> > >
> > > On Wed, 9 Oct 2024 at 17:55, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > > >
> > > > 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/202410092019.vGogfPIO-lkp@intel.com/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 <lkp@intel.com>
> > > > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > | Closes: https://lore.kernel.org/r/202410092019.vGogfPIO-lkp@intel.com/
> > > >
> > > > 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.
> > >
> > > > 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.
> > >
> > > 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.
> > >
> > > >
> > > > 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!
> >
> > > Here are my modified changes on top of my changes for the review process.
> > > -----8<----------8<----------8<----------8<----------8<----------8<-----
> > > diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c
> > > b/drivers/phy/rockchip/phy-rockchip-pcie.c
> > > index 2c4d6f68f02a..09685dc3fe17 100644
> > > --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> > > +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> > > @@ -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.
> >
> Ok.
> > > + }
> > >
> > > 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;
> >
> > return 0; here too.
The above warning " missing error code 'err'"
so it's correct to return err. here.
> >
> Ok. I will update the patch.
> > regards,
> > dan carpenter
> >
>
Thanks
-Anand
© 2016 - 2024 Red Hat, Inc.