linux-kernel - RE: [PATCH v3 6/8] pwm: rzg2l-gpt: Add suspend/resume 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:
 <TY3PR01MB113464F283A3ED4A8AAC813D886DBA@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Mon, 1 Dec 2025 15:04:08 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
CC: biju.das.au <biju.das.au@...il.com>, Philipp Zabel
	<p.zabel@...gutronix.de>, "linux-pwm@...r.kernel.org"
	<linux-pwm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Geert Uytterhoeven <geert+renesas@...der.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: RE: [PATCH v3 6/8] pwm: rzg2l-gpt: Add suspend/resume support

Hi Uwe,

> -----Original Message-----
> From: Biju Das
> Sent: 01 December 2025 14:43
> Subject: RE: [PATCH v3 6/8] pwm: rzg2l-gpt: Add suspend/resume support
> 
> Hi Uwe,
> 
> > -----Original Message-----
> > From: Uwe Kleine-König <ukleinek@...nel.org>
> > Sent: 01 December 2025 14:16
> > Subject: Re: [PATCH v3 6/8] pwm: rzg2l-gpt: Add suspend/resume support
> >
> > Hello Biju,
> >
> > On Mon, Dec 01, 2025 at 11:09:50AM +0000, Biju Das wrote:
> > > > -----Original Message-----
> > > > From: Uwe Kleine-König <ukleinek@...nel.org>
> > > > Sent: 30 November 2025 08:39
> > > > Subject: Re: [PATCH v3 6/8] pwm: rzg2l-gpt: Add suspend/resume
> > > > support
> > > >
> > > > On Tue, Sep 23, 2025 at 03:45:10PM +0100, Biju wrote:
> > > > > +static int rzg2l_gpt_suspend(struct device *dev) {
> > > > > +	struct pwm_chip *chip = dev_get_drvdata(dev);
> > > > > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > > > > +	unsigned int i;
> > > > > +
> > > > > +	for (i = 0; i < RZG2L_MAX_HW_CHANNELS; i++) {
> > > > > +		if (!rzg2l_gpt->channel_enable_count[i])
> > > > > +			continue;
> > > > > +
> > > > > +		rzg2l_gpt->hw_cache[i].gtpr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(i));
> > > > > +		rzg2l_gpt->hw_cache[i].gtccr[0] = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(i, 0));
> > > > > +		rzg2l_gpt->hw_cache[i].gtccr[1] = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(i, 1));
> > > > > +		rzg2l_gpt->hw_cache[i].gtcr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(i));
> > > > > +		rzg2l_gpt->hw_cache[i].gtior = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTIOR(i));
> > > > > +	}
> > > > > +
> > > > > +	clk_disable_unprepare(rzg2l_gpt->clk);
> > > > > +	clk_disable_unprepare(rzg2l_gpt->bus_clk);
> > > > > +	reset_control_assert(rzg2l_gpt->rst_s);
> > > > > +	reset_control_assert(rzg2l_gpt->rst);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int rzg2l_gpt_resume(struct device *dev) {
> > > > > +	struct pwm_chip *chip = dev_get_drvdata(dev);
> > > > > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > > > > +	unsigned int i;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = reset_control_deassert(rzg2l_gpt->rst);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = reset_control_deassert(rzg2l_gpt->rst_s);
> > > > > +	if (ret)
> > > > > +		goto fail_reset;
> > > > > +
> > > > > +	ret = clk_prepare_enable(rzg2l_gpt->bus_clk);
> > > > > +	if (ret)
> > > > > +		goto fail_reset_all;
> > > > > +
> > > > > +	ret = clk_prepare_enable(rzg2l_gpt->clk);
> > > > > +	if (ret)
> > > > > +		goto fail_bus_clk;
> > > > > +
> > > > > +	for (i = 0; i < RZG2L_MAX_HW_CHANNELS; i++) {
> > > > > +		if (!rzg2l_gpt->channel_enable_count[i])
> > > > > +			continue;
> > > > > +
> > > > > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(i), rzg2l_gpt->hw_cache[i].gtpr);
> > > > > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(i, 0), rzg2l_gpt->hw_cache[i].gtccr[0]);
> > > > > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(i, 1), rzg2l_gpt->hw_cache[i].gtccr[1]);
> > > > > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCR(i), rzg2l_gpt->hw_cache[i].gtcr);
> > > > > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTIOR(i), rzg2l_gpt->hw_cache[i].gtior);
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +fail_bus_clk:
> > > > > +	clk_disable_unprepare(rzg2l_gpt->bus_clk);
> > > > > +fail_reset_all:
> > > > > +	reset_control_assert(rzg2l_gpt->rst_s);
> > > > > +fail_reset:
> > > > > +	reset_control_assert(rzg2l_gpt->rst);
> > > > > +	return ret;
> > > >
> > > > I wonder what happens if these calls in the error path fail. I
> > > > think the correct way would be to track the actual state to handle
> > > > the state on the next invokation for .resume() properly. But note
> > > > that suspend/resume is a somewhat blind spot for me, so I'm unsure here.
> > > > (And I'm aware that most resume callbacks don't cope cleanly
> > > > here.)
> > >
> > > In str case, there is no power on the system during suspend and exit
> > > is, SoC reset followed by restoring registers from DDR. So, it does not matter for the suspend
> path.
> > >
> > > In the resume case, If the calls to error path fail, then device won't work.
> >
> > I'm not sure you understand my concern. IFAIUI a device that fails to
> > resume stays suspended from the POV of the kernel. When in this state
> > the resume is tried again at a later point in time you get
> > inconsistencies if the first reset was already deasserted from the
> > previous resume run (because
> > reset_control_assert() failed in the resume callback's error path).
> 
> I have simulated a possible error condition in driver by adding a hack.
> 
> Unlike probe(), .resume() never retries.
> 
> For the first time: I got the pwm resume error [2] For the second time: I got clk related warnings,
> but device enter into
> 			   suspend mode and on resume I got pwm resume error [3]

You mean to avoid unbalance between suspend()/resume(), we should not
do error handling in resume()??

We just keep accumulating error in ret variable and avoid register access if there is error
and return error to the caller. In this way there is no unbalance across suspend()/resume()??
This way second trial may pass.

Cheers,
Biju