diff mbox series

[RESEND,v8,03/10] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()

Message ID 20240410134044.2138310-4-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series watchdog: rzg2l_wdt: Add support for RZ/G3S | expand

Commit Message

Claudiu Beznea April 10, 2024, 1:40 p.m. UTC
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

pm_runtime_get_sync() may return with error. In case it returns with error
dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
takes care of this. Thus use it.

Along with it the rzg2l_wdt_set_timeout() function was updated to
propagate the result of rzg2l_wdt_start() to its caller.

Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v8:
- none

Changes in v7:
- none

Changes in v6:
- none

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- propagate the return code of rzg2l_wdt_start() to it's callers


 drivers/watchdog/rzg2l_wdt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Biju Das April 10, 2024, 1:51 p.m. UTC | #1
Hi Claudiu,

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, April 10, 2024 2:41 PM
> Subject: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> pm_runtime_get_sync() may return with error. In case it returns with error
> dev->power.usage_count needs to be decremented.
> dev->pm_runtime_resume_and_get()
> takes care of this. Thus use it.
> 
> Along with it the rzg2l_wdt_set_timeout() function was updated to propagate the result of
> rzg2l_wdt_start() to its caller.
> 
> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v8:
> - none
> 
> Changes in v7:
> - none
> 
> Changes in v6:
> - none
> 
> Changes in v5:
> - none
> 
> Changes in v4:
> - none
> 
> Changes in v3:
> - none
> 
> Changes in v2:
> - propagate the return code of rzg2l_wdt_start() to it's callers
> 
> 
>  drivers/watchdog/rzg2l_wdt.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index
> 1741f98ca67c..d87d4f50180c 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)  static int
> rzg2l_wdt_start(struct watchdog_device *wdev)  {
>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	int ret;
> 
> -	pm_runtime_get_sync(wdev->parent);
> +	ret = pm_runtime_resume_and_get(wdev->parent);
> +	if (ret)
> +		return ret;
> 
>  	/* Initialize time out */
>  	rzg2l_wdt_init_timeout(wdev);
> @@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
> 
>  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)  {
> +	int ret = 0;
> +
>  	wdev->timeout = timeout;
> 
>  	/*
> @@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int
> time
>  	 */
>  	if (watchdog_active(wdev)) {
>  		rzg2l_wdt_stop(wdev);
> -		rzg2l_wdt_start(wdev);
> +		ret = rzg2l_wdt_start(wdev);

This IP won't be able to update WDT settings once you have set it.

But we can update it, if we do a reset assert followed by deassert.
So the previous code looks correct to me.

Current case is if the WDT is active, then start it. Maybe I ma missing something here.

Cheers,
Biju

>  	}
> 
> -	return 0;
> +	return ret;
>  }
> 
>  static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> --
> 2.39.2
Claudiu Beznea April 10, 2024, 2:01 p.m. UTC | #2
On 10.04.2024 16:51, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Wednesday, April 10, 2024 2:41 PM
>> Subject: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> pm_runtime_get_sync() may return with error. In case it returns with error
>> dev->power.usage_count needs to be decremented.
>> dev->pm_runtime_resume_and_get()
>> takes care of this. Thus use it.
>>
>> Along with it the rzg2l_wdt_set_timeout() function was updated to propagate the result of
>> rzg2l_wdt_start() to its caller.
>>
>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v8:
>> - none
>>
>> Changes in v7:
>> - none
>>
>> Changes in v6:
>> - none
>>
>> Changes in v5:
>> - none
>>
>> Changes in v4:
>> - none
>>
>> Changes in v3:
>> - none
>>
>> Changes in v2:
>> - propagate the return code of rzg2l_wdt_start() to it's callers
>>
>>
>>  drivers/watchdog/rzg2l_wdt.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index
>> 1741f98ca67c..d87d4f50180c 100644
>> --- a/drivers/watchdog/rzg2l_wdt.c
>> +++ b/drivers/watchdog/rzg2l_wdt.c
>> @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)  static int
>> rzg2l_wdt_start(struct watchdog_device *wdev)  {
>>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>> +	int ret;
>>
>> -	pm_runtime_get_sync(wdev->parent);
>> +	ret = pm_runtime_resume_and_get(wdev->parent);
>> +	if (ret)
>> +		return ret;
>>
>>  	/* Initialize time out */
>>  	rzg2l_wdt_init_timeout(wdev);
>> @@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>>
>>  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)  {
>> +	int ret = 0;
>> +
>>  	wdev->timeout = timeout;
>>
>>  	/*
>> @@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int
>> time
>>  	 */
>>  	if (watchdog_active(wdev)) {
>>  		rzg2l_wdt_stop(wdev);
>> -		rzg2l_wdt_start(wdev);
>> +		ret = rzg2l_wdt_start(wdev);
> 
> This IP won't be able to update WDT settings once you have set it.
> 
> But we can update it, if we do a reset assert followed by deassert.
> So the previous code looks correct to me.
> 
> Current case is if the WDT is active, then start it. Maybe I ma missing something here.
> 

I'm not sure I got you correctly.

This patch keeps the previous functionality, thus, if the watchdog is
active rzg2l_wdt_stop() is called which does a reset assert. Then
rzg2l_wdt_start() is called which does the reset deassert.

Thank you,
Claudiu Beznea

> Cheers,
> Biju
> 
>>  	}
>>
>> -	return 0;
>> +	return ret;
>>  }
>>
>>  static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>> --
>> 2.39.2
>
Biju Das April 10, 2024, 2:05 p.m. UTC | #3
Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, April 10, 2024 3:02 PM
> Subject: Re: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
> 
> 
> 
> On 10.04.2024 16:51, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Wednesday, April 10, 2024 2:41 PM
> >> Subject: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use
> >> pm_runtime_resume_and_get()
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> pm_runtime_get_sync() may return with error. In case it returns with
> >> error
> >> dev->power.usage_count needs to be decremented.
> >> dev->pm_runtime_resume_and_get()
> >> takes care of this. Thus use it.
> >>
> >> Along with it the rzg2l_wdt_set_timeout() function was updated to
> >> propagate the result of
> >> rzg2l_wdt_start() to its caller.
> >>
> >> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for
> >> RZ/G2L")
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v8:
> >> - none
> >>
> >> Changes in v7:
> >> - none
> >>
> >> Changes in v6:
> >> - none
> >>
> >> Changes in v5:
> >> - none
> >>
> >> Changes in v4:
> >> - none
> >>
> >> Changes in v3:
> >> - none
> >>
> >> Changes in v2:
> >> - propagate the return code of rzg2l_wdt_start() to it's callers
> >>
> >>
> >>  drivers/watchdog/rzg2l_wdt.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >> b/drivers/watchdog/rzg2l_wdt.c index 1741f98ca67c..d87d4f50180c
> >> 100644
> >> --- a/drivers/watchdog/rzg2l_wdt.c
> >> +++ b/drivers/watchdog/rzg2l_wdt.c
> >> @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct
> >> watchdog_device *wdev)  static int rzg2l_wdt_start(struct watchdog_device *wdev)  {
> >>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >> +	int ret;
> >>
> >> -	pm_runtime_get_sync(wdev->parent);
> >> +	ret = pm_runtime_resume_and_get(wdev->parent);
> >> +	if (ret)
> >> +		return ret;
> >>
> >>  	/* Initialize time out */
> >>  	rzg2l_wdt_init_timeout(wdev);
> >> @@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device
> >> *wdev)
> >>
> >>  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev,
> >> unsigned int timeout)  {
> >> +	int ret = 0;
> >> +
> >>  	wdev->timeout = timeout;
> >>
> >>  	/*
> >> @@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct
> >> watchdog_device *wdev, unsigned int time
> >>  	 */
> >>  	if (watchdog_active(wdev)) {
> >>  		rzg2l_wdt_stop(wdev);
> >> -		rzg2l_wdt_start(wdev);
> >> +		ret = rzg2l_wdt_start(wdev);
> >
> > This IP won't be able to update WDT settings once you have set it.
> >
> > But we can update it, if we do a reset assert followed by deassert.
> > So the previous code looks correct to me.
> >
> > Current case is if the WDT is active, then start it. Maybe I ma missing something here.
> >
> 
> I'm not sure I got you correctly.
> 
> This patch keeps the previous functionality, thus, if the watchdog is active rzg2l_wdt_stop() is
> called which does a reset assert. Then
> rzg2l_wdt_start() is called which does the reset deassert.

You are correct. I overlooked and thought you have removed *_stop() as well.
Sorry for the noise.

Cheers,
Biju


> >>  	}
> >>
> >> -	return 0;
> >> +	return ret;
> >>  }
> >>
> >>  static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> >> --
> >> 2.39.2
> >
Biju Das April 10, 2024, 2:13 p.m. UTC | #4
Hi Claudiu,

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, April 10, 2024 2:41 PM
> Subject: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> pm_runtime_get_sync() may return with error. In case it returns with error
> dev->power.usage_count needs to be decremented.
> dev->pm_runtime_resume_and_get()
> takes care of this. Thus use it.
> 
> Along with it the rzg2l_wdt_set_timeout() function was updated to propagate the result of
> rzg2l_wdt_start() to its caller.
> 
> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v8:
> - none
> 
> Changes in v7:
> - none
> 
> Changes in v6:
> - none
> 
> Changes in v5:
> - none
> 
> Changes in v4:
> - none
> 
> Changes in v3:
> - none
> 
> Changes in v2:
> - propagate the return code of rzg2l_wdt_start() to it's callers
> 
> 
>  drivers/watchdog/rzg2l_wdt.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index
> 1741f98ca67c..d87d4f50180c 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)  static int
> rzg2l_wdt_start(struct watchdog_device *wdev)  {
>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	int ret;
> 
> -	pm_runtime_get_sync(wdev->parent);
> +	ret = pm_runtime_resume_and_get(wdev->parent);

Do we need this change at all? If we have balanced usage then
this won't be a issue. Did any unbalanced usage count popup
during the testing?

Cheers,
Biju

> +	if (ret)
> +		return ret;
> 
>  	/* Initialize time out */
>  	rzg2l_wdt_init_timeout(wdev);
> @@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
> 
>  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)  {
> +	int ret = 0;
> +
>  	wdev->timeout = timeout;
> 
>  	/*
> @@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int
> time
>  	 */
>  	if (watchdog_active(wdev)) {
>  		rzg2l_wdt_stop(wdev);
> -		rzg2l_wdt_start(wdev);
> +		ret = rzg2l_wdt_start(wdev);
>  	}
> 
> -	return 0;
> +	return ret;
>  }
> 
>  static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> --
> 2.39.2
Claudiu Beznea April 10, 2024, 2:48 p.m. UTC | #5
On 10.04.2024 17:13, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Wednesday, April 10, 2024 2:41 PM
>> Subject: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> pm_runtime_get_sync() may return with error. In case it returns with error
>> dev->power.usage_count needs to be decremented.
>> dev->pm_runtime_resume_and_get()
>> takes care of this. Thus use it.
>>
>> Along with it the rzg2l_wdt_set_timeout() function was updated to propagate the result of
>> rzg2l_wdt_start() to its caller.
>>
>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v8:
>> - none
>>
>> Changes in v7:
>> - none
>>
>> Changes in v6:
>> - none
>>
>> Changes in v5:
>> - none
>>
>> Changes in v4:
>> - none
>>
>> Changes in v3:
>> - none
>>
>> Changes in v2:
>> - propagate the return code of rzg2l_wdt_start() to it's callers
>>
>>
>>  drivers/watchdog/rzg2l_wdt.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index
>> 1741f98ca67c..d87d4f50180c 100644
>> --- a/drivers/watchdog/rzg2l_wdt.c
>> +++ b/drivers/watchdog/rzg2l_wdt.c
>> @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)  static int
>> rzg2l_wdt_start(struct watchdog_device *wdev)  {
>>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>> +	int ret;
>>
>> -	pm_runtime_get_sync(wdev->parent);
>> +	ret = pm_runtime_resume_and_get(wdev->parent);
> 
> Do we need this change at all? 

I haven't encountered issues w/o this patch, though I've did all my testing
(including suspend to RAM) with this patch on my tree.

> If we have balanced usage then
> this won't be a issue.

I think we should just use the proper APIs w/o making assumptions.

> Did any unbalanced usage count popup
> during the testing?
> 
> Cheers,
> Biju
> 
>> +	if (ret)
>> +		return ret;
>>
>>  	/* Initialize time out */
>>  	rzg2l_wdt_init_timeout(wdev);
>> @@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>>
>>  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)  {
>> +	int ret = 0;
>> +
>>  	wdev->timeout = timeout;
>>
>>  	/*
>> @@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int
>> time
>>  	 */
>>  	if (watchdog_active(wdev)) {
>>  		rzg2l_wdt_stop(wdev);
>> -		rzg2l_wdt_start(wdev);
>> +		ret = rzg2l_wdt_start(wdev);
>>  	}
>>
>> -	return 0;
>> +	return ret;
>>  }
>>
>>  static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>> --
>> 2.39.2
>
Biju Das April 10, 2024, 2:54 p.m. UTC | #6
Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, April 10, 2024 3:49 PM
> Subject: Re: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
> 
> 
> 
> On 10.04.2024 17:13, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Wednesday, April 10, 2024 2:41 PM
> >> Subject: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use
> >> pm_runtime_resume_and_get()
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> pm_runtime_get_sync() may return with error. In case it returns with
> >> error
> >> dev->power.usage_count needs to be decremented.
> >> dev->pm_runtime_resume_and_get()
> >> takes care of this. Thus use it.
> >>
> >> Along with it the rzg2l_wdt_set_timeout() function was updated to
> >> propagate the result of
> >> rzg2l_wdt_start() to its caller.
> >>
> >> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for
> >> RZ/G2L")
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v8:
> >> - none
> >>
> >> Changes in v7:
> >> - none
> >>
> >> Changes in v6:
> >> - none
> >>
> >> Changes in v5:
> >> - none
> >>
> >> Changes in v4:
> >> - none
> >>
> >> Changes in v3:
> >> - none
> >>
> >> Changes in v2:
> >> - propagate the return code of rzg2l_wdt_start() to it's callers
> >>
> >>
> >>  drivers/watchdog/rzg2l_wdt.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >> b/drivers/watchdog/rzg2l_wdt.c index 1741f98ca67c..d87d4f50180c
> >> 100644
> >> --- a/drivers/watchdog/rzg2l_wdt.c
> >> +++ b/drivers/watchdog/rzg2l_wdt.c
> >> @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct
> >> watchdog_device *wdev)  static int rzg2l_wdt_start(struct watchdog_device *wdev)  {
> >>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >> +	int ret;
> >>
> >> -	pm_runtime_get_sync(wdev->parent);
> >> +	ret = pm_runtime_resume_and_get(wdev->parent);
> >
> > Do we need this change at all?
> 
> I haven't encountered issues w/o this patch, though I've did all my testing (including suspend to
> RAM) with this patch on my tree.
> 
> > If we have balanced usage then
> > this won't be a issue.
> 
> I think we should just use the proper APIs w/o making assumptions.

Currently many subsystems in linux kernel use pm_runtime_get_sync() 
without any error checks and it is fine to use as long as the usage is balanced.

Moreover, currently you are not able to reproduce an error condition
hence checking do we need this change?

Cheers,
Biju

> 
> > Did any unbalanced usage count popup
> > during the testing?
> >
> > Cheers,
> > Biju
> >
> >> +	if (ret)
> >> +		return ret;
> >>
> >>  	/* Initialize time out */
> >>  	rzg2l_wdt_init_timeout(wdev);
> >> @@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device
> >> *wdev)
> >>
> >>  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev,
> >> unsigned int timeout)  {
> >> +	int ret = 0;
> >> +
> >>  	wdev->timeout = timeout;
> >>
> >>  	/*
> >> @@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct
> >> watchdog_device *wdev, unsigned int time
> >>  	 */
> >>  	if (watchdog_active(wdev)) {
> >>  		rzg2l_wdt_stop(wdev);
> >> -		rzg2l_wdt_start(wdev);
> >> +		ret = rzg2l_wdt_start(wdev);
> >>  	}
> >>
> >> -	return 0;
> >> +	return ret;
> >>  }
> >>
> >>  static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> >> --
> >> 2.39.2
> >
Guenter Roeck April 10, 2024, 4:39 p.m. UTC | #7
On Wed, Apr 10, 2024 at 04:40:37PM +0300, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> pm_runtime_get_sync() may return with error. In case it returns with error
> dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
> takes care of this. Thus use it.
> 
> Along with it the rzg2l_wdt_set_timeout() function was updated to
> propagate the result of rzg2l_wdt_start() to its caller.
> 
> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> Changes in v8:
> - none
> 
> Changes in v7:
> - none
> 
> Changes in v6:
> - none
> 
> Changes in v5:
> - none
> 
> Changes in v4:
> - none
> 
> Changes in v3:
> - none
> 
> Changes in v2:
> - propagate the return code of rzg2l_wdt_start() to it's callers
> 
> 
>  drivers/watchdog/rzg2l_wdt.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 1741f98ca67c..d87d4f50180c 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)
>  static int rzg2l_wdt_start(struct watchdog_device *wdev)
>  {
>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	int ret;
>  
> -	pm_runtime_get_sync(wdev->parent);
> +	ret = pm_runtime_resume_and_get(wdev->parent);
> +	if (ret)
> +		return ret;
>  
>  	/* Initialize time out */
>  	rzg2l_wdt_init_timeout(wdev);
> @@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>  
>  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)
>  {
> +	int ret = 0;
> +
>  	wdev->timeout = timeout;
>  
>  	/*
> @@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int time
>  	 */
>  	if (watchdog_active(wdev)) {
>  		rzg2l_wdt_stop(wdev);
> -		rzg2l_wdt_start(wdev);
> +		ret = rzg2l_wdt_start(wdev);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> -- 
> 2.39.2
>
diff mbox series

Patch

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 1741f98ca67c..d87d4f50180c 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -123,8 +123,11 @@  static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)
 static int rzg2l_wdt_start(struct watchdog_device *wdev)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	int ret;
 
-	pm_runtime_get_sync(wdev->parent);
+	ret = pm_runtime_resume_and_get(wdev->parent);
+	if (ret)
+		return ret;
 
 	/* Initialize time out */
 	rzg2l_wdt_init_timeout(wdev);
@@ -150,6 +153,8 @@  static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 
 static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)
 {
+	int ret = 0;
+
 	wdev->timeout = timeout;
 
 	/*
@@ -159,10 +164,10 @@  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int time
 	 */
 	if (watchdog_active(wdev)) {
 		rzg2l_wdt_stop(wdev);
-		rzg2l_wdt_start(wdev);
+		ret = rzg2l_wdt_start(wdev);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int rzg2l_wdt_restart(struct watchdog_device *wdev,