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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <kfaiindnm4wpyr4iruczjlichq5g7n5ru5nvzeproty3qx3n6g@tu3fenmprzjy>
Date: Thu, 27 Jun 2024 21:21:39 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: claudiu beznea <claudiu.beznea@...on.dev>
Cc: Biju Das <biju.das.jz@...renesas.com>, 
	Chris Brandt <Chris.Brandt@...esas.com>, "robh@...nel.org" <robh@...nel.org>, 
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>, 
	"geert+renesas@...der.be" <geert+renesas@...der.be>, "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 Claudiu,

First of all, thanks Biju for checking the code and bringing up
this topic.

On Wed, Jun 26, 2024 at 09:30:52AM GMT, claudiu beznea wrote:
> >> On 25.06.2024 18:53, Biju Das wrote:

...

> >>>>  static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)  {
> >>>>  	writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10
> >>>> +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >>>>  	struct riic_dev *riic = i2c_get_adapdata(adap);
> >>>>  	struct device *dev = adap->dev.parent;
> >>>>  	unsigned long time_left;
> >>>> -	int i;
> >>>> +	int i, ret;
> >>>>  	u8 start_bit;
> >>>>
> >>>> -	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??
> 
> Ok, I'm leaving this to I2C maintainers.
> 
> Andi, Wolfram,
> 
> Would you prefer having WARN_ON_ONCE() instead of dev_err() for potential
> failures of pm_runtime_resume_and_get()?

I prefer dev_err. WARN_ON should be used for some serious
failures in the code.

E.g. memory corruption, like:

	a = 5;
	WARN_ON(a != 5);

but the system might still work even with such errors (otherwise
there is BUG_ON()).

Besides, WARN_ON() prints some information that are not really
useful to understand why the system didn't resume. For example
you don't need the stack trace for power management failures, but
you need it for code tracing code bugs.

Andi