Message-ID: <20210728234018.GH4670@sirena.org.uk>
Date:   Thu, 29 Jul 2021 00:40:18 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     pierre-louis.bossart@...ux.intel.com, rander.wang@...ux.intel.com,
        shumingf@...ltek.com, patches@...nsource.cirrus.com,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ASoC: dapm: Revert "use component prefix when checking
 widget names"

On Wed, Jul 28, 2021 at 07:29:30PM +0100, Richard Fitzgerald wrote:
> On 28/07/2021 17:09, Mark Brown wrote:

> > The thing here is that nobody would have thought that that any caller
> > would have been open coding this stuff like the component things were,

> On the contrary, since that was the only way to use these functions with
> a prefixed component it's entirely possible that there is code already
> adding the prefix. Why would you expect nobody has ever written code
> that works?

Prefixes just aren't that widely used, explict DAPM calls to set pin
states are more commonly used but tend not to be used in the simpler
systems that would normally use prefixes (though Soundwire appears to
have changed that quite a bit).  There's a decent chance that the Intel
system that ran into this is only the second or third one that ever saw
the two features used in combination.  This sort of missed case is a
thing we still run into from time to time with prefixes.

> > like this so people wouldn't think of auditing the callers to find uses

> I don't think that it's either safe or desirable to skip checking how
> callers use functionality that you want to change. My understanding of
> Linux development protocol was that if you make a change that affects
> other code, you are responsible for updating that other code to match.
> Regardless of whether you agree with how that other code was
> implemented. It creates a lot of overhead for everyone if it's ok to
> make changes without trying to fix up other code that is affected by
> that change.

Sure, and that's exactly how I spotted that there was a problem with the
patch you posted - your patch was clearly going to break the systems
which rely on the change to _find_widgets() including all CODEC drivers
that use the DAPM level API.  People should and generally do make a
reasonable effort to check things out but it's always going to be that
rather than a guarantee, you can't 100% rely on people spotting things
especially when callers go off piste and do weird things.  Some of the
assessment of reasonableness on the part of everyone concerned is going
to involve looking at things like how secure the abstraction layers
involved are and how one would expect things to work, also influenced by
things like the volume of users and how easy that makes it to find
unusual users.  That's most likely what happened here.

These things are going to happen from time to time, when they do the
thing to do is to raise the issue (as you did initially) and then engage
in any discussion about what's going on and how best to fix the problem
(which was a bit of an issue here).  If you have proposed a patch but
there are other things that need to be considered there is likely to be
some expectation that you might follow up with a new version unless
people have explicitly said otherwise (submitters often have a strong
preference for this).

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

Powered by blists - more mailing lists