| Message ID | 20241007035616.2701-3-linux.amoon@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RK3399: PCie Phy using new helper function | expand |
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'
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 }
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
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))
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(-)