linux-kernel - RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

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] [day] [month] [year] [list]
Message-ID:
 <TY3PR01MB11346478F338B7CDF1F683BA786A7A@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Fri, 5 Dec 2025 14:15:08 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Claudiu.Beznea <claudiu.beznea@...on.dev>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>
CC: "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 v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM
 support

Hi Claudiu,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@...on.dev>
> Sent: 05 December 2025 14:02
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> 
> 
> On 12/5/25 15:45, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: Claudiu Beznea <claudiu.beznea@...on.dev>
> >> Sent: 05 December 2025 13:30
> >> To: Biju Das <biju.das.jz@...renesas.com>; p.zabel@...gutronix.de
> >> Cc: linux-kernel@...r.kernel.org; linux-renesas-soc@...r.kernel.org;
> >> Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >> RAM support
> >>
> >>
> >>
> >> On 12/5/25 13:55, Biju Das wrote:
> >>> Hi Claudiu,
> >>>
> >>>> -----Original Message-----
> >>>> From: Biju Das
> >>>> Sent: 05 December 2025 10:57
> >>>> Subject: RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>> to RAM support
> >>>>
> >>>>
> >>>> Hi Claudiu,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Claudiu Beznea <claudiu.beznea@...on.dev>
> >>>>> Sent: 05 December 2025 10:47
> >>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>>> to RAM support
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 12/5/25 12:17, Biju Das wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Claudiu Beznea <claudiu.beznea@...on.dev>
> >>>>>>> Sent: 05 December 2025 10:00
> >>>>>>> To: Biju Das <biju.das.jz@...renesas.com>;
> >>>>>>> p.zabel@...gutronix.de
> >>>>>>> Cc: linux-kernel@...r.kernel.org;
> >>>>>>> linux-renesas-soc@...r.kernel.org;
> >>>>>>> Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add
> >>>>>>> suspend to RAM support
> >>>>>>>
> >>>>>>> Hi, Biju,
> >>>>>>>
> >>>>>>> On 12/5/25 10:53, Biju Das wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Hi Claudiu,
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Claudiu Beznea <claudiu.beznea@...on.dev>
> >>>>>>>>> Sent: 04 December 2025 18:26
> >>>>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add
> >>>>>>>>> suspend to RAM support
> >>>>>>>>>
> >>>>>
> >>>>> From my previous experience with suspend/resume implementations, I
> >>>>> can say restoring the system in failure cases in suspend/resume or
> >>>>> not, is up to the subsystem maintainer. So, I'll let Philipp to
> >>>>> decide how he wants to go with it in this
> >>>> driver.
> >>>>>
> >>>>
> >>>> Agreed.
> >>>>
> >>>>> They are still supporting suspend to idle, where power is
> >>>>> maintained, right? Shouldn't we cover this case?
> >>>>
> >>>> Yes, I agree. Probably best thing is zero failures, if there is a
> >>>> failure in suspend path, the same device will fail in similar
> >>>> fashion, and the system never enters
> >> suspend state.
> >>>>
> >>>> So, report the failure and debug and fix the issue.
> >>>
> >>> FYI, On your resume path, if the below call fails, then there is a pm imbalance for next
> suspend().
> >>>
> >>> ret = pm_runtime_resume_and_get(dev);
> >>>
> >>> Similarly, if reset_assert() fails for a shared reset.
> >>
> >> Wouldn't be the same if there will be no failure path code?
> 
> Could you please reply to this question as I may be wrong?
> 
> >
> >
> > Eg:
> > ret = reset_control_deassert(priv->rstc);
> > +	if (ret)
> > +		goto pwrrdy_off;
> >
> > Here you are skipping pm_runtime_resume_and_get(), The subsequent
> > suspend() Will lead to pm underflow error.
> >
> > Similarly, on suspend() you are checking the error code of
> > reset_assert(), If it fails, you deassert it. Surprisingly, there is
> > no deassert operation On resume().
> Could you please share how would you like to look these functions? It looks to me that you want to
> ignore any operation that might fail (as you proposed in the case of resume from power off) and just
> re-enable everything, unconditionally. If that's the case it wouldn't cover all the cases, either.
> E.g., if resume looks like this:
> 
> static int rzg2l_usbphy_ctrl_resume(struct device *dev) {
> 	struct rzg2l_usbphy_ctrl_priv *priv = dev_get_drvdata(dev);
> 
> 	rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, true);
> 	reset_control_deassert(priv->rstc);
> 	pm_runtime_resume_and_get(dev);
> 	rzg2l_usbphy_ctrl_init(priv);
> 
> 	return 0;
> }
> 
> the rzg2l_usbphy_ctrl_set_pwrrdy(), reset_control_deassert(),
> pm_runtime_resume_and_get() may still fail and may still lead to imbalance refcounters for the next
> suspend execution or other scenarios.

There is no perfect solution for resume(). For suspend(), we can put back the state to previous state.
But resume don't know for the failure cases.

Cheers,
Biju