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

Message-ID:
 <TY3PR01MB11346774C4D6D95C27057158986A2A@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Mon, 8 Dec 2025 10:42:55 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Uwe Kleine-König <ukleinek@...nel.org>, biju.das.au
	<biju.das.au@...il.com>
CC: 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 Kleine-König,

> -----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.)

One of your concerns is addressed here[1]. In case of reset_assert failure() we sets the register
bit back to deassert and vice versa to make it operational in case of suspend failure and
power saving mode in case of resume failure.

If the calls in error path for resume fails, it is set to operational state.

[1]
https://lore.kernel.org/all/20251208101356.101379-1-biju.das.jz@bp.renesas.com/

Cheers,
Biju