linux-kernel - Re: [PATCH v5 2/5] pinctrl: samsung: refactor drvdata suspend & resume callbacks

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: <5ff8d26d-65bd-4b99-90b1-ae01f0ee9eb7@kernel.org>
Date: Tue, 18 Mar 2025 20:47:47 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Peter Griffin <peter.griffin@...aro.org>,
 Sylwester Nawrocki <s.nawrocki@...sung.com>,
 Alim Akhtar <alim.akhtar@...sung.com>,
 Linus Walleij <linus.walleij@...aro.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
 andre.draszik@...aro.org, tudor.ambarus@...aro.org, willmcvicker@...gle.com,
 semen.protsenko@...aro.org, kernel-team@...roid.com, jaewon02.kim@...sung.com
Subject: Re: [PATCH v5 2/5] pinctrl: samsung: refactor drvdata suspend &
 resume callbacks

On 12/03/2025 22:58, Peter Griffin wrote:
> Move the call of drvdata->suspend()/resume into the loop which is
> iterating drvdata for each bank.


Side effect is that now each drvdata->suspend will be called before
saving registers. Please mention it here and this lead me to one more
comment.

> This allows the clk_enable() and clk_disable() logic to be removed


For suspend path - yes. For resume path - nothing changed, because
drvdata->resume(drvdata) was called with clock enabled.

> from each callback, and also avoids iterating the same loop again
> in the next function.

...

> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 963060920301ec90affb2ee6d758d3d602ffb4a9..375634d8cc79d6533603e3eed562452181e2ee25 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1349,6 +1349,9 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
>  		const u8 *widths = bank->type->fld_width;
>  		enum pincfg_type type;
>  
> +		if (drvdata->suspend)
> +			drvdata->suspend(bank);

Here suspend() is called before saving common register state (was
*after*)...

> +
>  		/* Registers without a powerdown config aren't lost */
>  		if (!widths[PINCFG_TYPE_CON_PDN])
>  			continue;
> @@ -1373,8 +1376,6 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
>  
>  	clk_disable(drvdata->pclk);
>  
> -	if (drvdata->suspend)
> -		drvdata->suspend(drvdata);
>  	if (drvdata->retention_ctrl && drvdata->retention_ctrl->enable)
>  		drvdata->retention_ctrl->enable(drvdata);
>  
> @@ -1406,9 +1407,6 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
>  		return ret;
>  	}
>  
> -	if (drvdata->resume)
> -		drvdata->resume(drvdata);
> -
>  	for (i = 0; i < drvdata->nr_banks; i++) {
>  		struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
>  		void __iomem *reg = bank->pctl_base + bank->pctl_offset;
> @@ -1416,6 +1414,9 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
>  		const u8 *widths = bank->type->fld_width;
>  		enum pincfg_type type;
>  
> +		if (drvdata->resume)
> +			drvdata->resume(bank);

But this is not symmetrically reversed now - resume() is before
restoring from saved state.

Maybe this change is intentional, but then it should be expressed in
commit msg and in commit why this was chosen.

I guess you decided to do that way only because of code:
	if (!widths[PINCFG_TYPE_CON_PDN])

This code should be symmetrically reversed, otherwise it just raises
questions. For saving register state, it does not really matter, but in
general if we assume driver-specific suspend callback is run the last,
then driver-specific resume callback should be first, no?


Best regards,
Krzysztof