linux-kernel - Re: [PATCH v3 05/18] mux: add mux_chip

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]

Date: Fri, 16 Feb 2024 17:07:21 +0200
From: Andy Shevchenko <andy@...nel.org>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Tony Lindgren <tony@...mide.com>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Vignesh R <vigneshr@...com>, Aaro Koskinen <aaro.koskinen@....fi>,
	Janusz Krzysztofik <jmkrzyszt@...il.com>,
	Andi Shyti <andi.shyti@...nel.org>, Peter Rosin <peda@...ntia.se>,
	Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-omap@...r.kernel.org,
	linux-i2c@...r.kernel.org, linux-phy@...ts.infradead.org,
	linux-pci@...r.kernel.org, gregory.clement@...tlin.com,
	theo.lebrun@...tlin.com, thomas.petazzoni@...tlin.com,
	u-kumar1@...com
Subject: Re: [PATCH v3 05/18] mux: add mux_chip_resume() function

On Fri, Feb 16, 2024 at 08:52:17AM +0100, Thomas Richard wrote:
> On 2/15/24 16:29, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote:

..

> >> +int mux_chip_resume(struct mux_chip *mux_chip)
> >> +{
> >> +	int global_ret = 0;
> >> +	int ret, i;
> >> +
> >> +	for (i = 0; i < mux_chip->controllers; ++i) {
> >> +		struct mux_control *mux = &mux_chip->mux[i];
> >> +
> >> +		if (mux->cached_state == MUX_CACHE_UNKNOWN)
> >> +			continue;
> >> +
> >> +		ret = mux_control_set(mux, mux->cached_state);
> >> +		if (ret < 0) {
> >> +			dev_err(&mux_chip->dev, "unable to restore state\n");
> >> +			if (!global_ret)
> >> +				global_ret = ret;
> > 
> > Hmm... This will record the first error and continue.
> 
> In the v2 we talked about this with Peter Rosin.
> 
> In fact, in the v1 (mux_chip_resume() didn't exists yet, everything was
> done in the mmio driver) I had the same behavior: try to restore all
> muxes and in case of error restore the first one.
> 
> I don't know what is the right solution. I just restored the behavior I
> had in v1.

Okay, I believe you know what you are doing, folks. But to me this approach
sounds at bare minimum "unusual". Because the failures here are not fatal
and recording the first one may or may not make sense and it's so fragile
as it completely implementation-dependent.

> >> +		}
> >> +	}
> >> +	return global_ret;
> > 
> > So here, we actually will get stale data in case there are > 1 failures.
> 
> Yes, indeed. But we will have an error message for each failure.

Which is also problematic. PM calls may easily spam the logs and outshadow
really important messages (like oopses).

-- 
With Best Regards,
Andy Shevchenko