diff mbox series

[v2,04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()

Message ID 20240131102017.1841495-5-claudiu.beznea.uj@bp.renesas.com
State Superseded
Headers show
Series watchdog: rzg2l_wdt: Add support for RZ/G3S | expand

Commit Message

Claudiu Beznea Jan. 31, 2024, 10:20 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

pm_runtime_put() may return an error code. Check its return status.

Along with it the rzg2l_wdt_set_timeout() function was updated to
propagate the result of rzg2l_wdt_stop() 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 v2:
- propagate the return code of rzg2l_wdt_stop() to it's callers

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

Comments

Biju Das Jan. 31, 2024, 10:32 a.m. UTC | #1 
Hi Claudiu,

Thanks for the feedback.

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, January 31, 2024 10:20 AM
> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
> pm_runtime_put()
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> pm_runtime_put() may return an error code. Check its return status.
> 
> Along with it the rzg2l_wdt_set_timeout() function was updated to
> propagate the result of rzg2l_wdt_stop() 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 v2:
> - propagate the return code of rzg2l_wdt_stop() to it's callers
> 
>  drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index d87d4f50180c..7bce093316c4 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct watchdog_device
> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	int ret;
> 
>  	rzg2l_wdt_reset(priv);
> -	pm_runtime_put(wdev->parent);
> +
> +	ret = pm_runtime_put(wdev->parent);
> +	if (ret < 0)
> +		return ret;

Do we need to check the return code? So far we didn't hit this condition.
If you are planning to do it, then just 

return pm_runtime_put(wdev->parent);

Cheers,
Biju

> 
>  	return 0;


>  }
> @@ -163,7 +167,10 @@ static int rzg2l_wdt_set_timeout(struct
> watchdog_device *wdev, unsigned int time
>  	 * to reset the module) so that it is updated with new timeout
> values.
>  	 */
>  	if (watchdog_active(wdev)) {
> -		rzg2l_wdt_stop(wdev);
> +		ret = rzg2l_wdt_stop(wdev);
> +		if (ret)
> +			return ret;
> +
>  		ret = rzg2l_wdt_start(wdev);
>  	}
> 
> --
> 2.39.2
Biju Das Jan. 31, 2024, 10:41 a.m. UTC | #2 
Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, January 31, 2024 10:36 AM
> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
> pm_runtime_put()
> 
> Hi, Biju,
> 
> On 31.01.2024 12:32, Biju Das wrote:
> > Hi Claudiu,
> >
> > Thanks for the feedback.
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Wednesday, January 31, 2024 10:20 AM
> >> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
> >> pm_runtime_put()
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> pm_runtime_put() may return an error code. Check its return status.
> >>
> >> Along with it the rzg2l_wdt_set_timeout() function was updated to
> >> propagate the result of rzg2l_wdt_stop() 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 v2:
> >> - propagate the return code of rzg2l_wdt_stop() to it's callers
> >>
> >>  drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
> >>  1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
> >> 100644
> >> --- a/drivers/watchdog/rzg2l_wdt.c
> >> +++ b/drivers/watchdog/rzg2l_wdt.c
> >> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
> >> watchdog_device
> >> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
> >>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >> +	int ret;
> >>
> >>  	rzg2l_wdt_reset(priv);
> >> -	pm_runtime_put(wdev->parent);
> >> +
> >> +	ret = pm_runtime_put(wdev->parent);
> >> +	if (ret < 0)
> >> +		return ret;
> >
> > Do we need to check the return code? So far we didn't hit this
> condition.
> > If you are planning to do it, then just
> >
> > return pm_runtime_put(wdev->parent);
> 
> pm_runtime_put() may return 1 if the device is suspended (which is not
> considered error) as explained here:

Oops, I missed that discussion. Out of curiosity,
What watchdog framework/consumer is going to do with a 
Non-error return value of 1?

Cheers,
Biju
Claudiu Beznea Jan. 31, 2024, 11 a.m. UTC | #3 
On 31.01.2024 12:41, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Wednesday, January 31, 2024 10:36 AM
>> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>> pm_runtime_put()
>>
>> Hi, Biju,
>>
>> On 31.01.2024 12:32, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>> Thanks for the feedback.
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>> Sent: Wednesday, January 31, 2024 10:20 AM
>>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>> pm_runtime_put()
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> pm_runtime_put() may return an error code. Check its return status.
>>>>
>>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>>>> propagate the result of rzg2l_wdt_stop() 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 v2:
>>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>>>
>>>>  drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
>>>> 100644
>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
>>>> watchdog_device
>>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>>>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>> +	int ret;
>>>>
>>>>  	rzg2l_wdt_reset(priv);
>>>> -	pm_runtime_put(wdev->parent);
>>>> +
>>>> +	ret = pm_runtime_put(wdev->parent);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>
>>> Do we need to check the return code? So far we didn't hit this
>> condition.
>>> If you are planning to do it, then just
>>>
>>> return pm_runtime_put(wdev->parent);
>>
>> pm_runtime_put() may return 1 if the device is suspended (which is not
>> considered error) as explained here:
> 
> Oops, I missed that discussion. Out of curiosity,
> What watchdog framework/consumer is going to do with a 
> Non-error return value of 1?

Looking at this:
https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L809

it seems that the positive values are not considered errors thus, indeed,
we may return directly:

return pm_runtime_put();

Guenter,

With this (and previous discussion from [1]), are you OK to change it like:

return pm_runtime_put();

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju
Guenter Roeck Jan. 31, 2024, 1:14 p.m. UTC | #4 
On 1/31/24 02:41, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Wednesday, January 31, 2024 10:36 AM
>> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>> pm_runtime_put()
>>
>> Hi, Biju,
>>
>> On 31.01.2024 12:32, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>> Thanks for the feedback.
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>> Sent: Wednesday, January 31, 2024 10:20 AM
>>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>> pm_runtime_put()
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> pm_runtime_put() may return an error code. Check its return status.
>>>>
>>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>>>> propagate the result of rzg2l_wdt_stop() 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 v2:
>>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>>>
>>>>   drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
>>>> 100644
>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
>>>> watchdog_device
>>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>>>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>> +	int ret;
>>>>
>>>>   	rzg2l_wdt_reset(priv);
>>>> -	pm_runtime_put(wdev->parent);
>>>> +
>>>> +	ret = pm_runtime_put(wdev->parent);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>
>>> Do we need to check the return code? So far we didn't hit this
>> condition.
>>> If you are planning to do it, then just
>>>
>>> return pm_runtime_put(wdev->parent);
>>
>> pm_runtime_put() may return 1 if the device is suspended (which is not
>> considered error) as explained here:
> 
> Oops, I missed that discussion. Out of curiosity,
> What watchdog framework/consumer is going to do with a
> Non-error return value of 1?
> 

You mean what the watchdog subsystem does if a driver violates its API ?
That is undefined. The API says:

* start: this is a pointer to the routine that starts the watchdog timer
   device.
   The routine needs a pointer to the watchdog timer device structure as a
   parameter. It returns zero on success or a negative errno code for failure.
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We are not going to change the API, if that is what you are suggesting.

Thanks,
Guenter
Guenter Roeck Jan. 31, 2024, 1:16 p.m. UTC | #5 
On 1/31/24 03:00, claudiu beznea wrote:
> 
> 
> On 31.01.2024 12:41, Biju Das wrote:
>> Hi Claudiu,
>>
>>> -----Original Message-----
>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>> Sent: Wednesday, January 31, 2024 10:36 AM
>>> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>> pm_runtime_put()
>>>
>>> Hi, Biju,
>>>
>>> On 31.01.2024 12:32, Biju Das wrote:
>>>> Hi Claudiu,
>>>>
>>>> Thanks for the feedback.
>>>>
>>>>> -----Original Message-----
>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>>> Sent: Wednesday, January 31, 2024 10:20 AM
>>>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>>> pm_runtime_put()
>>>>>
>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>
>>>>> pm_runtime_put() may return an error code. Check its return status.
>>>>>
>>>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>>>>> propagate the result of rzg2l_wdt_stop() 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 v2:
>>>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>>>>
>>>>>   drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
>>>>> 100644
>>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
>>>>> watchdog_device
>>>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>>>>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>>> +	int ret;
>>>>>
>>>>>   	rzg2l_wdt_reset(priv);
>>>>> -	pm_runtime_put(wdev->parent);
>>>>> +
>>>>> +	ret = pm_runtime_put(wdev->parent);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>
>>>> Do we need to check the return code? So far we didn't hit this
>>> condition.
>>>> If you are planning to do it, then just
>>>>
>>>> return pm_runtime_put(wdev->parent);
>>>
>>> pm_runtime_put() may return 1 if the device is suspended (which is not
>>> considered error) as explained here:
>>
>> Oops, I missed that discussion. Out of curiosity,
>> What watchdog framework/consumer is going to do with a
>> Non-error return value of 1?
> 
> Looking at this:
> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L809
> 
> it seems that the positive values are not considered errors thus, indeed,
> we may return directly:
> 
> return pm_runtime_put();
> 
> Guenter,
> 
> With this (and previous discussion from [1]), are you OK to change it like:
> 
> return pm_runtime_put();
> 

Instead of looking at the source, I would kindly ask you to look at the API.

Guenter
Biju Das Jan. 31, 2024, 1:38 p.m. UTC | #6 
Hi Guenter Roeck,

Thanks for the feedback.

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Wednesday, January 31, 2024 1:14 PM
> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
> pm_runtime_put()
> 
> On 1/31/24 02:41, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> >> Sent: Wednesday, January 31, 2024 10:36 AM
> >> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return
> >> status of
> >> pm_runtime_put()
> >>
> >> Hi, Biju,
> >>
> >> On 31.01.2024 12:32, Biju Das wrote:
> >>> Hi Claudiu,
> >>>
> >>> Thanks for the feedback.
> >>>
> >>>> -----Original Message-----
> >>>> From: Claudiu <claudiu.beznea@tuxon.dev>
> >>>> Sent: Wednesday, January 31, 2024 10:20 AM
> >>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status
> >>>> of
> >>>> pm_runtime_put()
> >>>>
> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>
> >>>> pm_runtime_put() may return an error code. Check its return status.
> >>>>
> >>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
> >>>> propagate the result of rzg2l_wdt_stop() 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 v2:
> >>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
> >>>>
> >>>>   drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
> >>>>   1 file changed, 9 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
> >>>> 100644
> >>>> --- a/drivers/watchdog/rzg2l_wdt.c
> >>>> +++ b/drivers/watchdog/rzg2l_wdt.c
> >>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
> >>>> watchdog_device
> >>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
> >>>>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >>>> +	int ret;
> >>>>
> >>>>   	rzg2l_wdt_reset(priv);
> >>>> -	pm_runtime_put(wdev->parent);
> >>>> +
> >>>> +	ret = pm_runtime_put(wdev->parent);
> >>>> +	if (ret < 0)
> >>>> +		return ret;
> >>>
> >>> Do we need to check the return code? So far we didn't hit this
> >> condition.
> >>> If you are planning to do it, then just
> >>>
> >>> return pm_runtime_put(wdev->parent);
> >>
> >> pm_runtime_put() may return 1 if the device is suspended (which is
> >> not considered error) as explained here:
> >
> > Oops, I missed that discussion. Out of curiosity, What watchdog
> > framework/consumer is going to do with a Non-error return value of 1?
> >
> 
> You mean what the watchdog subsystem does if a driver violates its API ?
> That is undefined. The API says:

The return value of 1 from pm_runtime_put() is not an error
If it is propagated to framework, it return as an error.
So as you suggested below, the driver needs to either pass zero or error code.
But mot non-error value such as 1.

See watchdog_reboot_notifier():
 
The driver stop() callback may return 1 and reboot won't work
as it is returning NOTIFY_BAD.

ret = wdd->ops->stop(wdd);
trace_watchdog_stop(wdd, ret);
if (ret)
	return NOTIFY_BAD;

> 
> * start: this is a pointer to the routine that starts the watchdog timer
>    device.
>    The routine needs a pointer to the watchdog timer device structure as a
>    parameter. It returns zero on success or a negative errno code for
> failure.
> 
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> We are not going to change the API, if that is what you are suggesting.

OK.

Cheers,
Biju
Claudiu Beznea Feb. 1, 2024, 6:11 a.m. UTC | #7 
On 31.01.2024 15:16, Guenter Roeck wrote:
> On 1/31/24 03:00, claudiu beznea wrote:
>>
>>
>> On 31.01.2024 12:41, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>>> -----Original Message-----
>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>>> Sent: Wednesday, January 31, 2024 10:36 AM
>>>> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>> pm_runtime_put()
>>>>
>>>> Hi, Biju,
>>>>
>>>> On 31.01.2024 12:32, Biju Das wrote:
>>>>> Hi Claudiu,
>>>>>
>>>>> Thanks for the feedback.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>>>> Sent: Wednesday, January 31, 2024 10:20 AM
>>>>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>>>> pm_runtime_put()
>>>>>>
>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>
>>>>>> pm_runtime_put() may return an error code. Check its return status.
>>>>>>
>>>>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>>>>>> propagate the result of rzg2l_wdt_stop() 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 v2:
>>>>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>>>>>
>>>>>>   drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
>>>>>> 100644
>>>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
>>>>>> watchdog_device
>>>>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>>>>>       struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>>>> +    int ret;
>>>>>>
>>>>>>       rzg2l_wdt_reset(priv);
>>>>>> -    pm_runtime_put(wdev->parent);
>>>>>> +
>>>>>> +    ret = pm_runtime_put(wdev->parent);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>
>>>>> Do we need to check the return code? So far we didn't hit this
>>>> condition.
>>>>> If you are planning to do it, then just
>>>>>
>>>>> return pm_runtime_put(wdev->parent);
>>>>
>>>> pm_runtime_put() may return 1 if the device is suspended (which is not
>>>> considered error) as explained here:
>>>
>>> Oops, I missed that discussion. Out of curiosity,
>>> What watchdog framework/consumer is going to do with a
>>> Non-error return value of 1?
>>
>> Looking at this:
>> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L809
>>
>> it seems that the positive values are not considered errors thus, indeed,
>> we may return directly:
>>
>> return pm_runtime_put();
>>
>> Guenter,
>>
>> With this (and previous discussion from [1]), are you OK to change it like:
>>
>> return pm_runtime_put();
>>
> 
> Instead of looking at the source, I would kindly ask you to look at the API.

OK, I'll keep the patch as is. Thank you for your input.

Claudiu Beznea

> 
> Guenter
>
diff mbox series

Patch

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index d87d4f50180c..7bce093316c4 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -144,9 +144,13 @@  static int rzg2l_wdt_start(struct watchdog_device *wdev)
 static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	int ret;
 
 	rzg2l_wdt_reset(priv);
-	pm_runtime_put(wdev->parent);
+
+	ret = pm_runtime_put(wdev->parent);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -163,7 +167,10 @@  static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int time
 	 * to reset the module) so that it is updated with new timeout values.
 	 */
 	if (watchdog_active(wdev)) {
-		rzg2l_wdt_stop(wdev);
+		ret = rzg2l_wdt_stop(wdev);
+		if (ret)
+			return ret;
+
 		ret = rzg2l_wdt_start(wdev);
 	}