linux-kernel - Re: [PATCH v2] gpio: pca953x: do not enable regmap cache when there is no regulator

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: <f71643eb-ee9c-4b39-af26-738ae82fd4bd@sirena.org.uk>
Date: Thu, 28 Nov 2024 12:28:24 +0000
From: Mark Brown <broonie@...nel.org>
To: haibo.chen@....com
Cc: linus.walleij@...aro.org, brgl@...ev.pl, lgirdwood@...il.com,
	marek.vasut@...il.com, linux-gpio@...r.kernel.org,
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev
Subject: Re: [PATCH v2] gpio: pca953x: do not enable regmap cache when there
 is no regulator

On Thu, Nov 28, 2024 at 10:00:42AM +0800, haibo.chen@....com wrote:

> Regmap cache mechanism is enabled in default. Thus, IO expander wouldn't
> handle GPIO set really before resuming back.

> But there are cases need to toggle gpio in NO_IRQ stage.
> e.g. To align with PCIe specification, PERST# signal connected on the IO
> expander must be toggled during PCIe RC's NO_IRQ_RESUME.

> Do not enable the regmap cache when IO expander doesn't have the regulator
> during system PM. That means the power of IO expander would be kept on,
> and the GPIOs of the IO expander can be toggled really during system PM.

Aside from the fact that the device will always have a regulator (power
isn't optional) this is obviously not related to any sequencing needs
that you have on resume.  It might happen to work on your system, but it
will potentially break other systems which do actually need the
registers restoring but don't have a regulator explicitly defined and
will fail to do anything on a system with ordering requirements that do
have one defined.  The fix for this is to make this driver resume early
if it's needed by other things that run in the resume sequence.

> -	reg = devm_regulator_get(dev, "vcc");
> -	if (IS_ERR(reg))
> -		return dev_err_probe(dev, PTR_ERR(reg), "reg get err\n");
> +	reg = devm_regulator_get_optional(dev, "vcc");
> +	if (IS_ERR(reg)) {

This is obviously buggy, the main supply for the device is not going to
be optional.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)