This reverts
commit ae4fc532244b ("ASoC: dapm: use component prefix when checking widget names")
That commit breaks all users of the snd_soc_component_*_pin() functions
because it results in the prefix being added twice. It also breaks code
that correctly uses the snd_soc_dapm_*_pin() functions.
Use the snd_soc_component_*_pin() functions if you want the component
prefix to be prepended automatically.
Use the raw snd_soc_dapm_*_pin() functions if the caller has the full
name that should be matched exactly.
Fixes: commit ae4fc532244b ("ASoC: dapm: use component prefix when checking widget names")
Signed-off-by: Richard Fitzgerald <[email protected]>
---
sound/soc/soc-dapm.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 91bf939d5233..1369a3fea911 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -2528,20 +2528,9 @@ static struct snd_soc_dapm_widget *dapm_find_widget(
{
struct snd_soc_dapm_widget *w;
struct snd_soc_dapm_widget *fallback = NULL;
- char prefixed_pin[80];
- const char *pin_name;
- const char *prefix = soc_dapm_prefix(dapm);
-
- if (prefix) {
- snprintf(prefixed_pin, sizeof(prefixed_pin), "%s %s",
- prefix, pin);
- pin_name = prefixed_pin;
- } else {
- pin_name = pin;
- }
for_each_card_widgets(dapm->card, w) {
- if (!strcmp(w->name, pin_name)) {
+ if (!strcmp(w->name, pin)) {
if (w->dapm == dapm)
return w;
else
--
2.11.0
On Sat, Jul 03, 2021 at 01:50:34PM +0100, Richard Fitzgerald wrote:
> That commit breaks all users of the snd_soc_component_*_pin() functions
> because it results in the prefix being added twice. It also breaks code
> that correctly uses the snd_soc_dapm_*_pin() functions.
> Use the snd_soc_component_*_pin() functions if you want the component
> prefix to be prepended automatically.
> Use the raw snd_soc_dapm_*_pin() functions if the caller has the full
> name that should be matched exactly.
I'm not sure the analysis of which function to use when is correct or
what we want here (though it will work ATM), though looking again more
closely at the patch it doesn't look entirely right either. The way
this used to be done, and the way that older code will most likely
assume things work, was that the DAPM functions would first try to match
on the local DAPM context before falling back to doing a global match.
This is what the fallback loop is intended to do, and the dapm functions
are passing the "search other contexts" flag into dapm_find_widget().
I'd not expect the distinction you seem to expect between component and
DAPM and we probably have a bunch of older drivers that aren't working
correctly like the Realtek driver mentioned in the original fix. I
think what needs to happen is that dapm_find_widget() needs to be
checking both the prefixed and non-prefixed names, and that the
component stuff shouldn't need to bother and just be a convenience
wrapper for users that happene to have a component to hand.
Alternatively we need to do an audit of all the non-machine drivers to
switch them to use the component functions exclusively (and possibly
some of the machine drivers as well), most of the CODEC users look to be
a small number of Wolfson/Cirrus ones.
On 05/07/2021 17:50, Mark Brown wrote:
> On Sat, Jul 03, 2021 at 01:50:34PM +0100, Richard Fitzgerald wrote:
>
>> That commit breaks all users of the snd_soc_component_*_pin() functions
>> because it results in the prefix being added twice. It also breaks code
>> that correctly uses the snd_soc_dapm_*_pin() functions.
>
>> Use the snd_soc_component_*_pin() functions if you want the component
>> prefix to be prepended automatically.
>
>> Use the raw snd_soc_dapm_*_pin() functions if the caller has the full
>> name that should be matched exactly.
>
> I'm not sure the analysis of which function to use when is correct or
> what we want here (though it will work ATM), though looking again more
> closely at the patch it doesn't look entirely right either. The way
> this used to be done, and the way that older code will most likely
> assume things work, was that the DAPM functions would first try to match
> on the local DAPM context before falling back to doing a global match.
> This is what the fallback loop is intended to do, and the dapm functions
> are passing the "search other contexts" flag into dapm_find_widget().
>
> I'd not expect the distinction you seem to expect between component and
> DAPM and we probably have a bunch of older drivers that aren't working
> correctly like the Realtek driver mentioned in the original fix. I
> think what needs to happen is that dapm_find_widget() needs to be
> checking both the prefixed and non-prefixed names, and that the
> component stuff shouldn't need to bother and just be a convenience
> wrapper for users that happene to have a component to hand.
> Alternatively we need to do an audit of all the non-machine drivers to
> switch them to use the component functions exclusively (and possibly
> some of the machine drivers as well), most of the CODEC users look to be
> a small number of Wolfson/Cirrus ones.
>
I don't mind if someone wants to change the core dapm functions if that
is generally useful, providing that it also updates all callers of those
functions to still work.
Changing the behaviour of core code to fix the Realtek driver without
updating other callers of those functions is a problem.
On 22/07/2021 10:55, Richard Fitzgerald wrote:
> On 05/07/2021 17:50, Mark Brown wrote:
>> On Sat, Jul 03, 2021 at 01:50:34PM +0100, Richard Fitzgerald wrote:
>>
>>> That commit breaks all users of the snd_soc_component_*_pin() functions
>>> because it results in the prefix being added twice. It also breaks code
>>> that correctly uses the snd_soc_dapm_*_pin() functions.
>>
>>> Use the snd_soc_component_*_pin() functions if you want the component
>>> prefix to be prepended automatically.
>>
>>> Use the raw snd_soc_dapm_*_pin() functions if the caller has the full
>>> name that should be matched exactly.
>>
>> I'm not sure the analysis of which function to use when is correct or
>> what we want here (though it will work ATM), though looking again more
>> closely at the patch it doesn't look entirely right either.? The way
>> this used to be done, and the way that older code will most likely
>> assume things work, was that the DAPM functions would first try to match
>> on the local DAPM context before falling back to doing a global match.
>> This is what the fallback loop is intended to do, and the dapm functions
>> are passing the "search other contexts" flag into dapm_find_widget().
>>
>> I'd not expect the distinction you seem to expect between component and
>> DAPM and we probably have a bunch of older drivers that aren't working
>> correctly like the Realtek driver mentioned in the original fix.? I
>> think what needs to happen is that dapm_find_widget() needs to be
>> checking both the prefixed and non-prefixed names, and that the
>> component stuff shouldn't need to bother and just be a convenience
>> wrapper for users that happene to have a component to hand.
>> Alternatively we need to do an audit of all the non-machine drivers to
>> switch them to use the component functions exclusively (and possibly
>> some of the machine drivers as well), most of the CODEC users look to be
>> a small number of Wolfson/Cirrus ones.
>>
>
> I don't mind if someone wants to change the core dapm functions if that
> is generally useful, providing that it also updates all callers of those
> functions to still work.
>
> Changing the behaviour of core code to fix the Realtek driver without
> updating other callers of those functions is a problem.
Just to point out this is breaking stuff right now. It's not just
theoretical.
On Fri, Jul 23, 2021 at 04:17:26PM +0100, Richard Fitzgerald wrote:
> On 22/07/2021 10:55, Richard Fitzgerald wrote:
> > On 05/07/2021 17:50, Mark Brown wrote:
> > I don't mind if someone wants to change the core dapm functions if that
> > is generally useful, providing that it also updates all callers of those
> > functions to still work.
> > Changing the behaviour of core code to fix the Realtek driver without
> > updating other callers of those functions is a problem.
> Just to point out this is breaking stuff right now. It's not just
> theoretical.
You took several weeks to respond to my review comment, I'm sure you can
cope with waiting a day or two for a response to your followup (which
I'm having trouble understanding TBH). It would probably help if you
could specifically identify the problem you are seeing and where you're
seeing it - as I said in my review comment there appears to be a
misconception about how the APIs are expected to work.
On Thu, Jul 22, 2021 at 10:55:23AM +0100, Richard Fitzgerald wrote:
> I don't mind if someone wants to change the core dapm functions if that
> is generally useful, providing that it also updates all callers of those
> functions to still work.
> Changing the behaviour of core code to fix the Realtek driver without
> updating other callers of those functions is a problem.
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,
it's simply the wrong abstraction level to be implementing something
like this so people wouldn't think of auditing the callers to find uses
which might notice that prefixing suddenly worked.
On 28/07/2021 17:09, Mark Brown wrote:
> On Thu, Jul 22, 2021 at 10:55:23AM +0100, Richard Fitzgerald wrote:
>
>> I don't mind if someone wants to change the core dapm functions if that
>> is generally useful, providing that it also updates all callers of those
>> functions to still work.
>
>> Changing the behaviour of core code to fix the Realtek driver without
>> updating other callers of those functions is a problem.
>
> 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?
> it's simply the wrong abstraction level to be implementing something
Ok, but that doesn't mean that it could never have happened.
> 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.
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).