Revert "ASoC: tegra: machine: Handle component name prefix"

Commit Message

Benjamin Bara Oct. 7, 2024, 8:12 a.m. UTC

From: Benjamin Bara <[e-mail]>

This reverts commit f82eb06a40c86c9a82537e956de401d497203d3a.

Tegra is adding the DAPM of the respective widgets directly to the card
and therefore the DAPM has no component. Without the component, the
precondition for snd_soc_dapm_to_component() fails, which results in
undefined behavior. Use the old implementation, as we cannot have a
prefix without component.

Cc: [e-mail] # v6.7+
Signed-off-by: Benjamin Bara <[e-mail]>
---
Original Link:
https://lore.kernel.org/all/[e-mail]/
---
 sound/soc/tegra/tegra_asoc_machine.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)


---
base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
change-id: 20241007-tegra-dapm-ef969b8f762a

Best regards,

Comments

Krzysztof Kozlowski Oct. 7, 2024, 1 p.m. UTC | #1

On 07/10/2024 10:12, Benjamin Bara wrote:
> From: Benjamin Bara <[e-mail]>
> 
> This reverts commit f82eb06a40c86c9a82537e956de401d497203d3a.
> 
> Tegra is adding the DAPM of the respective widgets directly to the card
> and therefore the DAPM has no component. Without the component, the
> precondition for snd_soc_dapm_to_component() fails, which results in
> undefined behavior. Use the old implementation, as we cannot have a
> prefix without component.
> 
> Cc: [e-mail] # v6.7+

Fixes: f82eb06a40c8 ("ASoC: tegra: machine: Handle component name prefix")

I think Samsung speyside from the same patchset might repeat this mistake.

Reviewed-by: Krzysztof Kozlowski <[e-mail]>

Best regards,
Krzysztof

Benjamin Bara Oct. 7, 2024, 1:17 p.m. UTC | #2

Hi Krzysztof,

On Mon, 7 Oct 2024 at 15:01, Krzysztof Kozlowski
<[e-mail]> wrote:
> On 07/10/2024 10:12, Benjamin Bara wrote:
> > From: Benjamin Bara <[e-mail]>
> >
> > This reverts commit f82eb06a40c86c9a82537e956de401d497203d3a.
> >
> > Tegra is adding the DAPM of the respective widgets directly to the card
> > and therefore the DAPM has no component. Without the component, the
> > precondition for snd_soc_dapm_to_component() fails, which results in
> > undefined behavior. Use the old implementation, as we cannot have a
> > prefix without component.
> >
> > Cc: [e-mail] # v6.7+
>
> Fixes: f82eb06a40c8 ("ASoC: tegra: machine: Handle component name prefix")
>
> I think Samsung speyside from the same patchset might repeat this mistake.

Instead of reverting, we could probably also rewrite
snd_soc_dapm_widget_name_cmp() to directly use dapm->component, instead
of using snd_soc_dapm_to_component(). In this case, we can explicitly
check for a NULL and skip the prefix check - not sure why it currently
is implemented this way.

I think fixing snd_soc_dapm_widget_name_cmp() to be able to handle all
cases might be the better option, what do you think?

Thanks & best regards
Benjamin

> Reviewed-by: Krzysztof Kozlowski <[e-mail]>
>
> Best regards,
> Krzysztof
>

Mark Brown Oct. 7, 2024, 1:55 p.m. UTC | #3

On Mon, Oct 07, 2024 at 03:17:45PM +0200, Benjamin Bara wrote:

> Instead of reverting, we could probably also rewrite
> snd_soc_dapm_widget_name_cmp() to directly use dapm->component, instead
> of using snd_soc_dapm_to_component(). In this case, we can explicitly
> check for a NULL and skip the prefix check - not sure why it currently
> is implemented this way.

> I think fixing snd_soc_dapm_widget_name_cmp() to be able to handle all
> cases might be the better option, what do you think?

Yes, I think that makes sense.

Patch

diff --git a/sound/soc/tegra/tegra_asoc_machine.c b/sound/soc/tegra/tegra_asoc_machine.c
index 775ce433fdbfdbcff4d09d078dbb0e65c0b15b60..bc16f805f52c41d6cb983380ee0ac40944531e52 100644
--- a/sound/soc/tegra/tegra_asoc_machine.c
+++ b/sound/soc/tegra/tegra_asoc_machine.c
@@ -81,23 +81,19 @@  static int tegra_machine_event(struct snd_soc_dapm_widget *w,
 	struct snd_soc_dapm_context *dapm = w->dapm;
 	struct tegra_machine *machine = snd_soc_card_get_drvdata(dapm->card);
 
-	if (!snd_soc_dapm_widget_name_cmp(w, "Int Spk") ||
-	    !snd_soc_dapm_widget_name_cmp(w, "Speakers"))
+	if (!strcmp(w->name, "Int Spk") || !strcmp(w->name, "Speakers"))
 		gpiod_set_value_cansleep(machine->gpiod_spkr_en,
 					 SND_SOC_DAPM_EVENT_ON(event));
 
-	if (!snd_soc_dapm_widget_name_cmp(w, "Mic Jack") ||
-	    !snd_soc_dapm_widget_name_cmp(w, "Headset Mic"))
+	if (!strcmp(w->name, "Mic Jack") || !strcmp(w->name, "Headset Mic"))
 		gpiod_set_value_cansleep(machine->gpiod_ext_mic_en,
 					 SND_SOC_DAPM_EVENT_ON(event));
 
-	if (!snd_soc_dapm_widget_name_cmp(w, "Int Mic") ||
-	    !snd_soc_dapm_widget_name_cmp(w, "Internal Mic 2"))
+	if (!strcmp(w->name, "Int Mic") || !strcmp(w->name, "Internal Mic 2"))
 		gpiod_set_value_cansleep(machine->gpiod_int_mic_en,
 					 SND_SOC_DAPM_EVENT_ON(event));
 
-	if (!snd_soc_dapm_widget_name_cmp(w, "Headphone") ||
-	    !snd_soc_dapm_widget_name_cmp(w, "Headphone Jack"))
+	if (!strcmp(w->name, "Headphone") || !strcmp(w->name, "Headphone Jack"))
 		gpiod_set_value_cansleep(machine->gpiod_hp_mute,
 					 !SND_SOC_DAPM_EVENT_ON(event));