linux-kernel - RE: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use pm_runtime_resume_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <OSAPR01MB1587065352A1AF2922DD6B4186062@OSAPR01MB1587.jpnprd01.prod.outlook.com>
Date: Wed, 10 Apr 2024 14:54:48 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Claudiu.Beznea <claudiu.beznea@...on.dev>, "wim@...ux-watchdog.org"
	<wim@...ux-watchdog.org>, "linux@...ck-us.net" <linux@...ck-us.net>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>, "geert+renesas@...der.be"
	<geert+renesas@...der.be>, "magnus.damm@...il.com" <magnus.damm@...il.com>
CC: "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: RE: [PATCH RESEND v8 03/10] watchdog: rzg2l_wdt: Use
 pm_runtime_resume_and_get()

Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@...on.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@...on.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@...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@...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
> >