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] [thread-next>] [day] [month] [year] [list]

Message-ID: <562eda90-6ca2-40b5-b1f8-fcc4034dd122@tuxon.dev>
Date: Fri, 5 Dec 2025 16:01:51 +0200
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Biju Das <biju.das.jz@...renesas.com>,
 "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



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.

Thank you,
Claudiu