linux-kernel - RE: [PATCH v2 04/12] i2c: riic: 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 for Android: free password hash cracker in your pocket

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

Date: Wed, 26 Jun 2024 08:15:23 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
CC: Claudiu.Beznea <claudiu.beznea@...on.dev>, Chris Brandt
	<Chris.Brandt@...esas.com>, "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"magnus.damm@...il.com" <magnus.damm@...il.com>, "mturquette@...libre.com"
	<mturquette@...libre.com>, "sboyd@...nel.org" <sboyd@...nel.org>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"wsa+renesas@...g-engineering.com" <wsa+renesas@...g-engineering.com>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, Claudiu Beznea
	<claudiu.beznea.uj@...renesas.com>
Subject: RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@...ux-m68k.org>
> Sent: Wednesday, June 26, 2024 8:11 AM
> Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()
> 
> Hi Biju,
> 
> On Wed, Jun 26, 2024 at 8:23 AM Biju Das <biju.das.jz@...renesas.com> wrote:
> > > From: claudiu beznea <claudiu.beznea@...on.dev> On 25.06.2024 18:53,
> > > Biju Das wrote:
> > > >> From: Claudiu <claudiu.beznea@...on.dev>
> > > >> 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.
> > > >>
> > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> 
> > > >> -  pm_runtime_get_sync(dev);
> > > >> +  ret = pm_runtime_resume_and_get(dev);  if (ret) {
> > > >> +          dev_err(dev, riic_rpm_err_msg);
> > > >
> > > > As at the moment we don't know how to reproduce this error
> > > > condition Can we use WARN_ON_ONCE() instead to catch detailed error condition here??
> > >
> > > [1] states "So, naturally, use of WARN_ON() is also now discouraged
> > > much of the time". I've go with
> > > dev_err() or something similar.
> >
> > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once??
> >
> > Currently we don't know how to trigger pm_runtime_resume_and_get()
> > error condition in our setup using a testapp and we are expecting an
> > error may happen in future. If at all there is an error in future, we
> > need detailed error info so that we can handle it and fix the bug.
> 
> On Renesas systems, pm_runtime_resume_and_get() never fails.
> That's the reason why originally we didn't care to check the return value of pm_runtime_get_sync().

I agree, 

I was under the impression, if the code guarantees balanced usage,
then pm_runtime_get_sync()/put() it will never fails.

But here we are adding checks in frequent calls like xfer
on the assumption it may fail in future due to PM changes.

Xfer, we are incrementing pm usage count with pm_runtime_get_sync()
And then decrementing it with pm_runtime_put() once transfer completed

So, there is no imbalance here.


> 
> The various janitors disagreed, causing cascaded changes all over the place...

Even the core code does not have check for this.
https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L792

> 
> IMHO, WARN_ON_ONCE() is definitely overkill, only bloating the code.

I suggested because we are adding this check because something 
wrong will happen in future due to PM subsystem changes and the check will capture the
issue and will give detailed warning info in kernel log.

Cheers,
Biju