linux-kernel - Re: [PATCH] mux: suppress lookup errors for mux controls

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: <Z_0VMCmfnDZxd4GC@hovoldconsulting.com>
Date: Mon, 14 Apr 2025 16:01:20 +0200
From: Johan Hovold <johan@...nel.org>
To: Peter Rosin <peda@...ntia.se>
Cc: Johan Hovold <johan+linaro@...nel.org>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
	Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mux: suppress lookup errors for mux controls

On Mon, Apr 14, 2025 at 03:18:03PM +0200, Peter Rosin wrote:
> 2025-04-14 at 14:42, Johan Hovold wrote:
> > Since commit eec611d26f84 ("ASoC: codecs: wcd938x: add mux control
> > support for hp audio mux") we have drivers looking up mux controls that
> > are optional. This results in errors incorrectly being logged on
> > machines like the Lenovo ThinkPad X13s where the mux is missing:
> > 
> >     wcd938x_codec audio-codec: /audio-codec: failed to get mux-control (0)
> > 
> > Suppress the error message when lookup of mux controls fails and make
> > sure to return -ENOENT consistently also when looking up controls by
> > name so that consumer drivers can easily determine how to proceed.
> > 
> > Note that most current consumers already log mux lookup failures
> > themselves.
> > 
> > Fixes: eec611d26f84 ("ASoC: codecs: wcd938x: add mux control support for hp audio mux")
> > Link: https://lore.kernel.org/lkml/Z-z_ZAyVBK5ui50k@hovoldconsulting.com/
> > Signed-off-by: Johan Hovold <johan+linaro@...nel.org>

> > --- a/drivers/mux/core.c
> > +++ b/drivers/mux/core.c
> > @@ -544,8 +544,13 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
> >  			index = of_property_match_string(np, "mux-control-names",
> >  							 mux_name);
> >  		if (index < 0) {
> > -			dev_err(dev, "mux controller '%s' not found\n",
> > -				mux_name);
> > +			if (!state && index == -EINVAL)
> > +				index = -ENOENT;
> 
> Why exclude states? For me, that's entirely random and inconsistent. If there's
> a reason to exclude them, I'd like to hear about it. If there is no reason and
> this is just defensive programming, then I'd like for someone to dig into it
> and either find a reason for the difference or clean up the inconsistency.

I only found one user of "mux states" and I'm still not quite sure why
there are two interfaces for looking up muxes. But my impression was
that if you need a mux set to a specific state and you even encode that
directly in DT, then there should be no need to support optional
resources.

After taking a closer look at the single consumer now, I see that it
already implements optional lookups itself and thus could benefit from
generalising this.

There's no other reason for why this could not be extended to "mux
states".

> I think the model of explicitly marking when you'd like a mux to be optional
> is a better and less fragile model. Who is to say that -EINVAL from some other
> call is, and will remain, a perfect match for the optional case you are aiming
> for?

-EINVAL is simply the error returned from the OF helpers when the name
properties are missing. I map that to -ENOENT for consistency with index
lookups (i.e. when the "mux-controls" property is missing) and that
error is much less likely to be returned for other reasons.

> Srinivas Kandagatla is looking into optional muxes as a side issue to
> exclusive muxes.
> https://lore.kernel.org/all/20250326154613.3735-1-srinivas.kandagatla@linaro.org/

The audio codec change introduces a de-facto regression so if you want
something different, we'll have to fix this in the codec driver directly
by checking for a "mux-controls" property before doing the lookup for
now (i.e. like is done in the TI driver looking up an optional "mux
state").

Johan