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

[Biju Das]

Hi Guenter Roeck,

Thanks for the feedback.

> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> 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@xxxxxxxxx>
> >> 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@xxxxxxxxx>
> >>>> 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@xxxxxxxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxxxxxxx>
> >>>> ---
> >>>>
> >>>> 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