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

Hi Guenter Roeck,

Thanks for the feedback.

-----Original Message-----
From: Guenter Roeck <redacted> 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:

quoted

Hi Claudiu,

quoted

-----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:

quoted

Hi Claudiu,

Thanks for the feedback.

quoted

-----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.

quoted

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