RE: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@xxxxxxx]
> Sent: Monday, February 5, 2018 7:15 AM
> To: Alex Hung <alex.hung@xxxxxxxxxxxxx>
> Cc: rjw@xxxxxxxxxxxxx; lenb@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; mika.westerberg@xxxxxxxxxxxxxxx;
> andriy.shevchenko@xxxxxxxxxxxxxxx; f.fainelli@xxxxxxxxx;
> dmitry.torokhov@xxxxxxxxx; kishon@xxxxxx; karniksayli1995@xxxxxxxxx; linux-
> acpi@xxxxxxxxxxxxxxx; Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> 
> Hi Alex,
> 
> On Wed, 31 Jan 2018 00:40:05 -0800, Alex Hung wrote:
> > A number of Dell systems require an OEM _OSI string "Linux-Dell-Video" as
> > a BIOS workaround for a system hang bug caused by discrete VGA. The form of
> > the OEM _OSI string is discussed in Documentation/acpi/osi.txt and is
> > defined by each OEM.
> 
> I admit I don't understand how it is the operating system's job to
> carry the information from the BIOS to the BIOS.

The reason it's done this way is so that Linux kernel can revert this behavior when it's
no longer appropriate without system vendor having to issue a BIOS update to do this.

For example if underlying issue that required this BIOS ASL workaround is fixed in a
newer kernel.

You can read the documentation about this in Documentations/acpi/osi.txt.

> 
> > Signed-off-by: Alex Hung <alex.hung@xxxxxxxxxxxxx>
> > ---
> >  drivers/acpi/osi.c | 107
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> >
> > diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
> > index 76998a5..43e4349 100644
> > --- a/drivers/acpi/osi.c
> > +++ b/drivers/acpi/osi.c
> > @@ -477,6 +477,112 @@ static const struct dmi_system_id
> acpi_osi_dmi_table[] __initconst = {
> >  	{}
> >  };
> >
> > +static int __init dmi_oem_osi_add(const struct dmi_system_id *d)
> > +{
> > +	struct acpi_osi_entry *osi;
> > +	const char *str = d->driver_data;
> > +	int i;
> > +
> > +	for (i = 0; i < OSI_STRING_ENTRIES_MAX; i++) {
> > +		osi = &osi_setup_entries[i];
> > +		if (!strcmp(osi->string, str)) {
> 
> This can only happen if the user passes acpi_osi=Linux-Dell-Video or
> acpi_osi=!Linux-Dell-Video on the boot command line, right?
> 
> > +			osi->enable = true;
> 
> Does this not prevent the user from explicitly disabling it with
> acpi_osi=!Linux-Dell-Video ?
> 
> > +			continue;
> 
> Are you not done at this point? I think you want to break, not
> continue, else you may add a duplicate Linux-Dell-Video entry below.

You might have two different entries that apply to the same system in the future.

At least the way that Dell is intending to use this is that "Linux-Dell-Video" would
only apply to ASL related to video configuration.

If for example there is later an issue with audio on a different platform that also
needed "Linux-Dell-Video", the ASL to do the audio configuration would be activated
by "Linux-Dell-Audio".

> 
> > +		} else if (osi->string[0] == '\0') {
> > +			osi->enable = true;
> > +			strncpy(osi->string, str, OSI_STRING_LENGTH_MAX);
> > +			break;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dmi_system_id acpi_oem_osi_dmi_table[] __initconst = {
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell Latitude 5491",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[0818]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell Latitude 5591",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[0819]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell Precision 3530",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[0820]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell Inspiron 7777 AIO",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[0850]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell Inspiron 5477 AIO",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[0851]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell G5 5779",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[0886]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell G5 5779",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[0870]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell G5 5579",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[086F]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{
> > +	.callback = dmi_oem_osi_add,
> > +	.ident = "Dell G5 5579",
> > +	.matches = {
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "Dell System"),
> > +		     DMI_EXACT_MATCH(DMI_OEM_STRING, "1[0885]"),
> > +		},
> > +	.driver_data = "Linux-Dell-Video",
> > +	},
> > +	{}
> > +};
> 
> G5 5779 and G5 5579 appear twice each, are these copy-and-paste errors?

No, there are two different PCBs that can be used in these systems.  They share
same "Product Name".  You can only differentiate between them via OEM string.

> 
> Choosing a sort rule and sticking to it would make it easier to add
> entries later with not risk of duplicates.
> 
> > +
> >  static __init void acpi_osi_dmi_blacklisted(void)
> >  {
> >  	dmi_check_system(acpi_osi_dmi_table);
> > @@ -496,6 +602,7 @@ int __init early_acpi_osi_init(void)
> >  int __init acpi_osi_init(void)
> >  {
> >  	acpi_install_interface_handler(acpi_osi_handler);
> > +	dmi_check_system(acpi_oem_osi_dmi_table);
> >  	acpi_osi_setup_late();
> >
> >  	return 0;
> 
> 
> --
> Jean Delvare
> SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux