Re: [PATCH 5.16 regression fix 2/2] ASoC: Intel: soc-acpi-cht: Revert shrink tables using compatible IDs

From: Hans de Goede <hdegoede@redhat.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Cezary Rojewski <cezary.rojewski@intel.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Jie Yang <yang.jie@linux.intel.com>,
	Mark Brown <broonie@kernel.org>
Cc: alsa-devel@alsa-project.org, Brent Lu <brent.lu@intel.com>
Subject: Re: [PATCH 5.16 regression fix 2/2] ASoC: Intel: soc-acpi-cht: Revert shrink tables using compatible IDs
Date: Wed, 17 Nov 2021 17:26:36 +0100	[thread overview]
Message-ID: <235ed35d-fd0d-cb4c-29ff-9a269ba63f7a@redhat.com> (raw)
In-Reply-To: <f6b445bf-3444-ed7f-b31a-d71dd0599927@linux.intel.com>

Hi,

On 11/17/21 16:54, Pierre-Louis Bossart wrote:
> 
> 
> On 11/17/21 9:10 AM, Hans de Goede wrote:
>> Commit 959ae8215a9e ("ASoC: Intel: soc-acpi-cht: shrink tables using
>> compatible IDs") simplified the match tables in soc-acpi-intel-cht-match.c
>> by merging identical entries using the new .comp_ids snd_soc_acpi_mach
>> field to point a single entry to multiple ACPI HIDs and clearing the
>> previously unique per entry .id field.
>>
>> But various machine drivers from sound/soc/intel/boards rely on mach->id
>> in one or more ways. For example all of the following machine-drivers
>> for entries combined during the shrinking:
>> sound/soc/intel/boards/bytcr_rt5640.c
>> sound/soc/intel/boards/cht_bsw_rt5645.c
>> sound/soc/intel/boards/bytcht_da7213.c
>>
>> Do:
>> 	adev = acpi_dev_get_first_match_dev(mach->id, NULL, -1);
>>
>> Which now no longer works and some of them also do:
>>
>> 	pkg_found = snd_soc_acpi_find_package_from_hid(mach->id, ...
>>
>> 	if (!strncmp(snd_soc_cards[i].codec_id, mach->id, 8)) { ...
>>
>> Which now also no longer works. All these calls need to be fixed before
>> we can shrink the tables, so revert this change for now.
>>
>> Fixes: 959ae8215a9e ("ASoC: Intel: soc-acpi-cht: shrink tables using compatible IDs")
>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Cc: Brent Lu <brent.lu@intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks for the patch, it's embarrassing. I must have tested the wrong
> code or the wrong device...

No worries, we all make mistakes.

> Could we alternatively keep these tables and just copy the information
> using something like this (compile-tested only)

Interesting I was thinking along these lines myself as an
alternate fix, but I expected all the struct snd_soc_acpi_mach
declarations, e.g. the snd_soc_acpi_intel_baytrail_machines
array, to be const.

But I see now that these are not const, at least not for byt + cht.

So yes this should work and seems a bit nicer fix then
reverting.

I'll give this a test run when I'm done fixing some other
5.16 regressions I'm working on ...

Regards,

Hans

> 
> diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h
> index 31f4c4f9aeea..ac0893df9c76 100644
> --- a/include/sound/soc-acpi.h
> +++ b/include/sound/soc-acpi.h
> @@ -147,7 +147,7 @@ struct snd_soc_acpi_link_adr {
>   */
>  /* Descriptor for SST ASoC machine driver */
>  struct snd_soc_acpi_mach {
> -       const u8 id[ACPI_ID_LEN];
> +       u8 id[ACPI_ID_LEN];
>         const struct snd_soc_acpi_codecs *comp_ids;
>         const u32 link_mask;
>         const struct snd_soc_acpi_link_adr *links;
> diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
> index 2ae99b49d3f5..6b9dfa0608a3 100644
> --- a/sound/soc/soc-acpi.c
> +++ b/sound/soc/soc-acpi.c
> @@ -20,8 +20,10 @@ static bool snd_soc_acpi_id_present(struct
> snd_soc_acpi_mach *machine)
> 
>         if (comp_ids) {
>                 for (i = 0; i < comp_ids->num_codecs; i++) {
> -                       if (acpi_dev_present(comp_ids->codecs[i], NULL, -1))
> +                       if (acpi_dev_present(comp_ids->codecs[i], NULL,
> -1)) {
> +                               strncpy(machine->id,
> comp_ids->codecs[i], ACPI_ID_LEN);
>                                 return true;
> +                       }
>                 }
>         }
> 