| Message ID | 20230425100456.32718-1-xingyu.wu@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] watchdog: starfive: Fix the probe return error if PM and early_enable are both disabled | expand |
On 4/25/23 03:04, Xingyu Wu wrote: > When the starfive watchdog driver uses 'pm_runtime_put_sync()' as probe > return value at last and 'early_enable' is disabled, it could return the > error '-ENOSYS' if the CONFIG_PM is disabled, but the driver should works > normally. > > Add a check to make sure the PM is enabled and then use > 'pm_runtime_put_sync()' as return value when 'early_enable' is disabled. > > Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver") > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> > --- > > Hi, Guenter and Wim, > > This patch fixes the issue of StarFive watchdog driver and rebases on > the master branch of linux-next. > > Thanks. > > --- > drivers/watchdog/starfive-wdt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c > index 1995cceca51e..51e487e09960 100644 > --- a/drivers/watchdog/starfive-wdt.c > +++ b/drivers/watchdog/starfive-wdt.c > @@ -492,7 +492,8 @@ static int starfive_wdt_probe(struct platform_device *pdev) > goto err_exit; > > if (!early_enable) > - return pm_runtime_put_sync(&pdev->dev); > + if (pm_runtime_enabled(&pdev->dev)) > + return pm_runtime_put_sync(&pdev->dev); > Why not just if (!early_enable) pm_runtime_put_sync(&pdev->dev) like almost every other caller of pm_runtime_put_sync() ? Guenter
On 2023/4/25 22:06, Guenter Roeck wrote: > On 4/25/23 03:04, Xingyu Wu wrote: >> When the starfive watchdog driver uses 'pm_runtime_put_sync()' as probe >> return value at last and 'early_enable' is disabled, it could return the >> error '-ENOSYS' if the CONFIG_PM is disabled, but the driver should works >> normally. >> >> Add a check to make sure the PM is enabled and then use >> 'pm_runtime_put_sync()' as return value when 'early_enable' is disabled. >> >> Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver") >> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >> --- >> >> Hi, Guenter and Wim, >> >> This patch fixes the issue of StarFive watchdog driver and rebases on >> the master branch of linux-next. >> >> Thanks. >> --- >> drivers/watchdog/starfive-wdt.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c >> index 1995cceca51e..51e487e09960 100644 >> --- a/drivers/watchdog/starfive-wdt.c >> +++ b/drivers/watchdog/starfive-wdt.c >> @@ -492,7 +492,8 @@ static int starfive_wdt_probe(struct platform_device *pdev) >> goto err_exit; >> if (!early_enable) >> - return pm_runtime_put_sync(&pdev->dev); >> + if (pm_runtime_enabled(&pdev->dev)) >> + return pm_runtime_put_sync(&pdev->dev); >> > > Why not just > > if (!early_enable) > pm_runtime_put_sync(&pdev->dev) > > like almost every other caller of pm_runtime_put_sync() ? > The function of pm_runtime_put_sync() is that: static inline int pm_runtime_put_sync(struct device *dev) { return __pm_runtime_idle(dev, RPM_GET_PUT); } and when do not enable CONFIG_PM, the __pm_runtime_idle() is that: static inline int __pm_runtime_idle(struct device *dev, int rpmflags) { return -ENOSYS; } If I do not open PM, it will return error saying probe failed but it works fine without PM. I had tested that and probe really was failed. So I should add this check before using pm_runtime_put_sync(). Best regards, Xingyu Wu
On 4/25/23 18:49, Xingyu Wu wrote: > On 2023/4/25 22:06, Guenter Roeck wrote: >> On 4/25/23 03:04, Xingyu Wu wrote: >>> When the starfive watchdog driver uses 'pm_runtime_put_sync()' as probe >>> return value at last and 'early_enable' is disabled, it could return the >>> error '-ENOSYS' if the CONFIG_PM is disabled, but the driver should works >>> normally. >>> >>> Add a check to make sure the PM is enabled and then use >>> 'pm_runtime_put_sync()' as return value when 'early_enable' is disabled. >>> >>> Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver") >>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >>> --- >>> >>> Hi, Guenter and Wim, >>> >>> This patch fixes the issue of StarFive watchdog driver and rebases on >>> the master branch of linux-next. >>> >>> Thanks. >>> --- >>> drivers/watchdog/starfive-wdt.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c >>> index 1995cceca51e..51e487e09960 100644 >>> --- a/drivers/watchdog/starfive-wdt.c >>> +++ b/drivers/watchdog/starfive-wdt.c >>> @@ -492,7 +492,8 @@ static int starfive_wdt_probe(struct platform_device *pdev) >>> goto err_exit; >>> if (!early_enable) >>> - return pm_runtime_put_sync(&pdev->dev); >>> + if (pm_runtime_enabled(&pdev->dev)) >>> + return pm_runtime_put_sync(&pdev->dev); >>> >> >> Why not just >> >> if (!early_enable) >> pm_runtime_put_sync(&pdev->dev) >> >> like almost every other caller of pm_runtime_put_sync() ? >> > > The function of pm_runtime_put_sync() is that: > > static inline int pm_runtime_put_sync(struct device *dev) > { > return __pm_runtime_idle(dev, RPM_GET_PUT); > } > > and when do not enable CONFIG_PM, the __pm_runtime_idle() is that: > > static inline int __pm_runtime_idle(struct device *dev, int rpmflags) > { > return -ENOSYS; > } > > If I do not open PM, it will return error saying probe failed > but it works fine without PM. I had tested that and probe really > was failed. So I should add this check before using pm_runtime_put_sync(). > You did not answer my question. Guenter
On 2023/4/26 10:01, Guenter Roeck wrote: > On 4/25/23 18:49, Xingyu Wu wrote: >> On 2023/4/25 22:06, Guenter Roeck wrote: >>> On 4/25/23 03:04, Xingyu Wu wrote: >>>> When the starfive watchdog driver uses 'pm_runtime_put_sync()' as probe >>>> return value at last and 'early_enable' is disabled, it could return the >>>> error '-ENOSYS' if the CONFIG_PM is disabled, but the driver should works >>>> normally. >>>> >>>> Add a check to make sure the PM is enabled and then use >>>> 'pm_runtime_put_sync()' as return value when 'early_enable' is disabled. >>>> >>>> Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver") >>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >>>> --- >>>> >>>> Hi, Guenter and Wim, >>>> >>>> This patch fixes the issue of StarFive watchdog driver and rebases on >>>> the master branch of linux-next. >>>> >>>> Thanks. >>>> --- >>>> drivers/watchdog/starfive-wdt.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c >>>> index 1995cceca51e..51e487e09960 100644 >>>> --- a/drivers/watchdog/starfive-wdt.c >>>> +++ b/drivers/watchdog/starfive-wdt.c >>>> @@ -492,7 +492,8 @@ static int starfive_wdt_probe(struct platform_device *pdev) >>>> goto err_exit; >>>> if (!early_enable) >>>> - return pm_runtime_put_sync(&pdev->dev); >>>> + if (pm_runtime_enabled(&pdev->dev)) >>>> + return pm_runtime_put_sync(&pdev->dev); >>>> >>> >>> Why not just >>> >>> if (!early_enable) >>> pm_runtime_put_sync(&pdev->dev) >>> >>> like almost every other caller of pm_runtime_put_sync() ? >>> >> >> The function of pm_runtime_put_sync() is that: >> >> static inline int pm_runtime_put_sync(struct device *dev) >> { >> return __pm_runtime_idle(dev, RPM_GET_PUT); >> } >> >> and when do not enable CONFIG_PM, the __pm_runtime_idle() is that: >> >> static inline int __pm_runtime_idle(struct device *dev, int rpmflags) >> { >> return -ENOSYS; >> } >> >> If I do not open PM, it will return error saying probe failed >> but it works fine without PM. I had tested that and probe really >> was failed. So I should add this check before using pm_runtime_put_sync(). >> > > You did not answer my question. > Oh sorry, I did not notice that you dropped the 'return'. It looks simpler. Best regards, Xingyu Wu
diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c index 1995cceca51e..51e487e09960 100644 --- a/drivers/watchdog/starfive-wdt.c +++ b/drivers/watchdog/starfive-wdt.c @@ -492,7 +492,8 @@ static int starfive_wdt_probe(struct platform_device *pdev) goto err_exit; if (!early_enable) - return pm_runtime_put_sync(&pdev->dev); + if (pm_runtime_enabled(&pdev->dev)) + return pm_runtime_put_sync(&pdev->dev); return 0;
When the starfive watchdog driver uses 'pm_runtime_put_sync()' as probe return value at last and 'early_enable' is disabled, it could return the error '-ENOSYS' if the CONFIG_PM is disabled, but the driver should works normally. Add a check to make sure the PM is enabled and then use 'pm_runtime_put_sync()' as return value when 'early_enable' is disabled. Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver") Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> --- Hi, Guenter and Wim, This patch fixes the issue of StarFive watchdog driver and rebases on the master branch of linux-next. Thanks. --- drivers/watchdog/starfive-wdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)