diff mbox series

[v1] ALSA: hda: intel-dsp-config: Fix JSL Chromebook quirk detection

Message ID 20231018235944.1860717-1-markhas@chromium.org
State New
Headers
Series [v1] ALSA: hda: intel-dsp-config: Fix JSL Chromebook quirk detection |

Commit Message

Mark Hasemeyer Oct. 18, 2023, 11:59 p.m. UTC 
  Some Jasperlake Chromebooks overwrite the system vendor DMI value to the
name of the OEM that manufactured the device. This breaks Chromebook
quirk detection as it expects the system vendor to be "Google".

Add another quirk detection entry that looks for "Google" in the BIOS
version.

Cc: stable@vger.kernel.org
Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

 sound/hda/intel-dsp-config.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Amadeusz Sławiński Oct. 19, 2023, 2:22 p.m. UTC | #1 
On 10/19/2023 1:59 AM, Mark Hasemeyer wrote:
> Some Jasperlake Chromebooks overwrite the system vendor DMI value to the
> name of the OEM that manufactured the device. This breaks Chromebook
> quirk detection as it expects the system vendor to be "Google".
> 
> Add another quirk detection entry that looks for "Google" in the BIOS
> version.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
> 
>   sound/hda/intel-dsp-config.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c
> index 24a948baf1bc..756fa0aa69bb 100644
> --- a/sound/hda/intel-dsp-config.c
> +++ b/sound/hda/intel-dsp-config.c
> @@ -336,6 +336,12 @@ static const struct config_entry config_table[] = {
>   					DMI_MATCH(DMI_SYS_VENDOR, "Google"),
>   				}
>   			},
> +			{
> +				.ident = "Google firmware",
> +				.matches = {
> +					DMI_MATCH(DMI_BIOS_VERSION, "Google"),
> +				}
> +			},
>   			{}
>   		}
>   	},

I would assume that platform that has DMI_SYS_VENDOR set to "Google", 
also has DMI_BIOS_VERSION set to "Google", so perhaps just replace 
DMI_SYS_VENDOR match with DMI_BIOS_VERSION, to keep table small? Or is 
that not a case?
Mark Hasemeyer Oct. 19, 2023, 4:43 p.m. UTC | #2 
> I would assume that platform that has DMI_SYS_VENDOR set to "Google",
> also has DMI_BIOS_VERSION set to "Google", so perhaps just replace
> DMI_SYS_VENDOR match with DMI_BIOS_VERSION, to keep table small? Or is
> that not a case?

That is the case. But I'm inclined to keep it for two reasons:
1. There is precedent in the kernel to use DMI_SYS_VENDOR=="Google"
for Chromebook detection.
2. If the coreboot version schema for Chromebooks were to change, this
check would fail for all JSL Chromebooks instead of just a few models.
Pierre-Louis Bossart Oct. 19, 2023, 5 p.m. UTC | #3 
On 10/19/23 11:43, Mark Hasemeyer wrote:
>> I would assume that platform that has DMI_SYS_VENDOR set to "Google",
>> also has DMI_BIOS_VERSION set to "Google", so perhaps just replace
>> DMI_SYS_VENDOR match with DMI_BIOS_VERSION, to keep table small? Or is
>> that not a case?
> 
> That is the case. But I'm inclined to keep it for two reasons:
> 1. There is precedent in the kernel to use DMI_SYS_VENDOR=="Google"
> for Chromebook detection.
> 2. If the coreboot version schema for Chromebooks were to change, this
> check would fail for all JSL Chromebooks instead of just a few models.

I also prefer a low-risk addition to a higher-risk change. It's not like
we really care about the size of the table at this point.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

FWIW we use this other quirk:
DMI_MATCH(DMI_PRODUCT_FAMILY, "Google"),

How many engineers does it take to identify a Chromebook, eh?
Mark Hasemeyer Oct. 20, 2023, 3:36 p.m. UTC | #4 
> FWIW we use this other quirk:
> DMI_MATCH(DMI_PRODUCT_FAMILY, "Google"),

Unfortunately DMI_PRODUCT_FAMILY is empty on these particular devices.
The coreboot version field is the only entry that has "Google" in it.

> How many engineers does it take to identify a Chromebook, eh?

Ha! There has been some discussion about this: to come up with a
canonical way for Chromebook identification throughout the kernel. But
nothing has been settled on AFAIK.
Takashi Iwai Oct. 20, 2023, 3:40 p.m. UTC | #5 
On Thu, 19 Oct 2023 01:59:31 +0200,
Mark Hasemeyer wrote:
> 
> Some Jasperlake Chromebooks overwrite the system vendor DMI value to the
> name of the OEM that manufactured the device. This breaks Chromebook
> quirk detection as it expects the system vendor to be "Google".
> 
> Add another quirk detection entry that looks for "Google" in the BIOS
> version.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>

Applied now.  Thanks.


Takashi
Pierre-Louis Bossart Oct. 20, 2023, 4:59 p.m. UTC | #6 
On 10/20/23 10:36, Mark Hasemeyer wrote:
>> FWIW we use this other quirk:
>> DMI_MATCH(DMI_PRODUCT_FAMILY, "Google"),
> 
> Unfortunately DMI_PRODUCT_FAMILY is empty on these particular devices.
> The coreboot version field is the only entry that has "Google" in it.

well then you have additional issues with the DMI quirk for the firmware
selection in sound/soc/sof/sof-pci-dev.c,

      {
		.ident = "Google Chromebooks",
		.callback = chromebook_use_community_key,
		.matches = {
			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google"),
		}
	},

which means you need additional kernel parameters to provide the
location of the firmware....

>> How many engineers does it take to identify a Chromebook, eh?
> 
> Ha! There has been some discussion about this: to come up with a
> canonical way for Chromebook identification throughout the kernel. But
> nothing has been settled on AFAIK.

There's been multiple rounds of discussions with Curtis, we introduced
DMI_OEM_STRING but it's still not good enough, and now the previous
conventions are not being followed on what is a relatively old platform
already...
Mark Hasemeyer Oct. 20, 2023, 7:35 p.m. UTC | #7 
> There's been multiple rounds of discussions with Curtis, we introduced
> DMI_OEM_STRING but it's still not good enough, and now the previous
> conventions are not being followed on what is a relatively old platform
> already...

Thanks for pointing that out. ChromeOS uses the fw_path module
parameter for JSL boards so I missed this. The DMI_BIOS_VERSION should
be consistent across all Chromebooks assuming 3rd party firmware
wasn't installed. I'll throw up another patch addressing this.
diff mbox series

Patch

diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c
index 24a948baf1bc..756fa0aa69bb 100644
--- a/sound/hda/intel-dsp-config.c
+++ b/sound/hda/intel-dsp-config.c
@@ -336,6 +336,12 @@  static const struct config_entry config_table[] = {
 					DMI_MATCH(DMI_SYS_VENDOR, "Google"),
 				}
 			},
+			{
+				.ident = "Google firmware",
+				.matches = {
+					DMI_MATCH(DMI_BIOS_VERSION, "Google"),
+				}
+			},
 			{}
 		}
 	},