[PATCH 1/2] firmware: dmi_scan: add DMI_OEM_STRING support to dmi_matches

[PATCH 1/2] firmware: dmi_scan: add DMI_OEM_STRING support to dmi_matches

[Alex Hung]

OEM strings are defined by each OEM and they contain customized and
useful OEM information. Supporting it provides more flexible uses of
the dmi_matches function.

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 drivers/firmware/dmi_scan.c     | 8 ++++++++
 include/linux/mod_devicetable.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 7830419..e534d1b 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -797,11 +797,19 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
 			else if (dmi->matches[i].exact_match &&
 				 !strcmp(dmi_ident[s], dmi->matches[i].substr))
 				continue;
+		} else if (s == DMI_OEM_STRING) {
+			const struct dmi_device *valid;
+
+			valid = dmi_find_device(DMI_DEV_TYPE_OEM_STRING,
+						dmi->matches[i].substr, NULL);
+			if (valid)
+				continue;
 		}
 
 		/* No match */
 		return false;
 	}
+
 	return true;
 }
 
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index abb6dc2..5739c4c 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -482,6 +482,7 @@ enum dmi_field {
 	DMI_CHASSIS_VERSION,
 	DMI_CHASSIS_SERIAL,
 	DMI_CHASSIS_ASSET_TAG,
+	DMI_OEM_STRING,
 	DMI_STRING_MAX,
 };
 
-- 
2.7.4

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

[Alex Hung]

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.

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 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)) {
+			osi->enable = true;
+			continue;
+		} 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",
+	},
+	{}
+};
+
 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;
-- 
2.7.4

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

[Jean Delvare]

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.

> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  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.

> +		} 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?

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

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

[Andy Shevchenko]

On Mon, 2018-02-05 at 14:14 +0100, Jean Delvare wrote:

> 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.

> > +	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 ?

Playing with OSI string is a bad idea. I wouldn't do anything while
Rafael, or even Len can confirm that is the right thing to do.

For me, AFAIK we need to be bug-to-bug compatible with Windows (at least
on ACPICA side), so, what Windows exactly does on such laptops?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: Monday, February 5, 2018 8:15 AM
> To: Jean Delvare <jdelvare@suse.de>; Alex Hung <alex.hung@canonical.com>
> Cc: rjw@rjwysocki.net; lenb@kernel.org; gregkh@linuxfoundation.org;
> davem@davemloft.net; mika.westerberg@linux.intel.com; f.fainelli@gmail.com;
> dmitry.torokhov@gmail.com; kishon@ti.com; karniksayli1995@gmail.com; linux-
> acpi@vger.kernel.org; Limonciello, Mario <Mario_Limonciello@Dell.com>
> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> 
> On Mon, 2018-02-05 at 14:14 +0100, Jean Delvare wrote:
> 
> > 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.
> 
> > > +	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 ?
> 
> Playing with OSI string is a bad idea. I wouldn't do anything while
> Rafael, or even Len can confirm that is the right thing to do.
> 
> For me, AFAIK we need to be bug-to-bug compatible with Windows (at least
> on ACPICA side), so, what Windows exactly does on such laptops?
> 

The issue that's being worked around isn't an ACPICA interpreter issue, but it's 
a graphics device configuration issue.

Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
don't.  It leads to system hangs on the Linux side.

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

[Dmitry Torokhov]

On Mon, Feb 05, 2018 at 05:36:18PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> > Sent: Monday, February 5, 2018 8:15 AM
> > To: Jean Delvare <jdelvare@suse.de>; Alex Hung <alex.hung@canonical.com>
> > Cc: rjw@rjwysocki.net; lenb@kernel.org; gregkh@linuxfoundation.org;
> > davem@davemloft.net; mika.westerberg@linux.intel.com; f.fainelli@gmail.com;
> > dmitry.torokhov@gmail.com; kishon@ti.com; karniksayli1995@gmail.com; linux-
> > acpi@vger.kernel.org; Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> > 
> > On Mon, 2018-02-05 at 14:14 +0100, Jean Delvare wrote:
> > 
> > > 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.
> > 
> > > > +	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 ?
> > 
> > Playing with OSI string is a bad idea. I wouldn't do anything while
> > Rafael, or even Len can confirm that is the right thing to do.
> > 
> > For me, AFAIK we need to be bug-to-bug compatible with Windows (at least
> > on ACPICA side), so, what Windows exactly does on such laptops?
> > 
> 
> The issue that's being worked around isn't an ACPICA interpreter issue, but it's 
> a graphics device configuration issue.
> 
> Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
> don't.  It leads to system hangs on the Linux side.

Can we adjust Linux drivers to do the right thing? Or is it regarding
the binary NVIDIA blob?

Thanks.

-- 
Dmitry

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

[Mario.Limonciello]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Monday, February 5, 2018 4:46 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: andriy.shevchenko@linux.intel.com; jdelvare@suse.de;
> alex.hung@canonical.com; rjw@rjwysocki.net; lenb@kernel.org;
> gregkh@linuxfoundation.org; davem@davemloft.net;
> mika.westerberg@linux.intel.com; f.fainelli@gmail.com; kishon@ti.com;
> karniksayli1995@gmail.com; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> 
> On Mon, Feb 05, 2018 at 05:36:18PM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> > > Sent: Monday, February 5, 2018 8:15 AM
> > > To: Jean Delvare <jdelvare@suse.de>; Alex Hung <alex.hung@canonical.com>
> > > Cc: rjw@rjwysocki.net; lenb@kernel.org; gregkh@linuxfoundation.org;
> > > davem@davemloft.net; mika.westerberg@linux.intel.com;
> f.fainelli@gmail.com;
> > > dmitry.torokhov@gmail.com; kishon@ti.com; karniksayli1995@gmail.com;
> linux-
> > > acpi@vger.kernel.org; Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> > >
> > > On Mon, 2018-02-05 at 14:14 +0100, Jean Delvare wrote:
> > >
> > > > 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.
> > >
> > > > > +	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 ?
> > >
> > > Playing with OSI string is a bad idea. I wouldn't do anything while
> > > Rafael, or even Len can confirm that is the right thing to do.
> > >
> > > For me, AFAIK we need to be bug-to-bug compatible with Windows (at least
> > > on ACPICA side), so, what Windows exactly does on such laptops?
> > >
> >
> > The issue that's being worked around isn't an ACPICA interpreter issue, but it's
> > a graphics device configuration issue.
> >
> > Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
> > don't.  It leads to system hangs on the Linux side.
> 
> Can we adjust Linux drivers to do the right thing? Or is it regarding
> the binary NVIDIA blob?
> 

Neither Nouveau nor the NVIDIA blob have support for RTD3.

Last I heard it's waiting on NVIDIA releasing something Nouveau
needs.  So.. Eventually?  For now it's better to not hang though.

That's part of why we wanted to enable this via a transient OSI string,
to let this be removed by Linux whenever the driver does grow support.

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

[Andy Shevchenko]

On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:

> > > > Playing with OSI string is a bad idea. I wouldn't do anything
> > > > while
> > > > Rafael, or even Len can confirm that is the right thing to do.
> > > > 
> > > > For me, AFAIK we need to be bug-to-bug compatible with Windows
> > > > (at least
> > > > on ACPICA side), so, what Windows exactly does on such laptops?
> > > > 
> > > 
> > > The issue that's being worked around isn't an ACPICA interpreter
> > > issue, but it's
> > > a graphics device configuration issue.

Then clearly nothing to do with OSI strings here, right?

> > > Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
> > > don't.  It leads to system hangs on the Linux side.
> > 
> > Can we adjust Linux drivers to do the right thing?

+100

> >  Or is it regarding
> > the binary NVIDIA blob?

nVidia vs. Linux again? :-)

> 
> Neither Nouveau nor the NVIDIA blob have support for RTD3.
> 
> Last I heard it's waiting on NVIDIA releasing something Nouveau
> needs.  So.. Eventually?  For now it's better to not hang though.

Hmm... While you are talking sense, the patch itself looks like an ugly
hack.

> That's part of why we wanted to enable this via a transient OSI
> string,
> to let this be removed by Linux whenever the driver does grow support.

So, means "never" then? (Assume a bit of irony here)

I don't know how it feels for maintainers, for me it's quite unlikely to
go (at least in this shape).

I'm sorry I can't be much constructive here, I heard Len once about OSI
huge abuse by almost every party. I would rather let him speak on the
matter.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: Tuesday, February 6, 2018 7:46 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>;
> dmitry.torokhov@gmail.com
> Cc: jdelvare@suse.de; alex.hung@canonical.com; rjw@rjwysocki.net;
> lenb@kernel.org; gregkh@linuxfoundation.org; davem@davemloft.net;
> mika.westerberg@linux.intel.com; f.fainelli@gmail.com; kishon@ti.com;
> karniksayli1995@gmail.com; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> 
> On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:
> 
> > > > > Playing with OSI string is a bad idea. I wouldn't do anything
> > > > > while
> > > > > Rafael, or even Len can confirm that is the right thing to do.
> > > > >
> > > > > For me, AFAIK we need to be bug-to-bug compatible with Windows
> > > > > (at least
> > > > > on ACPICA side), so, what Windows exactly does on such laptops?
> > > > >
> > > >
> > > > The issue that's being worked around isn't an ACPICA interpreter
> > > > issue, but it's
> > > > a graphics device configuration issue.
> 
> Then clearly nothing to do with OSI strings here, right?

The usage of OSI here is: do you support the feature "Linux-Dell-Video"
to Linux kernel.  If Linux kernel responds yes then firmware will turn off 
RTD3 functionality for the discrete GPU.

> 
> > > > Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
> > > > don't.  It leads to system hangs on the Linux side.
> > >
> > > Can we adjust Linux drivers to do the right thing?
> 
> +100
> 
> > >  Or is it regarding
> > > the binary NVIDIA blob?
> 
> nVidia vs. Linux again? :-)
> 
> >
> > Neither Nouveau nor the NVIDIA blob have support for RTD3.
> >
> > Last I heard it's waiting on NVIDIA releasing something Nouveau
> > needs.  So.. Eventually?  For now it's better to not hang though.
> 
> Hmm... While you are talking sense, the patch itself looks like an ugly
> hack.

I don't disagree it's a workaround for a driver deficiency.

> 
> > That's part of why we wanted to enable this via a transient OSI
> > string,
> > to let this be removed by Linux whenever the driver does grow support.
> 
> So, means "never" then? (Assume a bit of irony here)
> 
> I don't know how it feels for maintainers, for me it's quite unlikely to
> go (at least in this shape).

I'm not going to be able to force NVIDIA to fix the blob or to give what's
necessary to enable Nouvaeu for this function.  What we can do is prevent
someone's system from hanging because of this situation.

> 
> I'm sorry I can't be much constructive here, I heard Len once about OSI
> huge abuse by almost every party. I would rather let him speak on the
> matter.

I know that OSI has been abused in the past, but that's exactly why this
has been implemented as seen in this patch series.

It's done specifically to fit within exactly what the Linux kernel ideally
looks for in a custom OSI feature string.

* It has an OS Prefix (Linux-)
* It's an OEM specific string (-Dell-)
* It's a feature specific string (-Video)
*It's specific to a single platform (not a blanket match to Dell Inc)
*It only modifies one feature in ASL

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

[Alex Hung]

On Tue, Feb 6, 2018 at 8:24 AM,  <Mario.Limonciello@dell.com> wrote:
>> -----Original Message-----
>> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
>> Sent: Tuesday, February 6, 2018 7:46 AM
>> To: Limonciello, Mario <Mario_Limonciello@Dell.com>;
>> dmitry.torokhov@gmail.com
>> Cc: jdelvare@suse.de; alex.hung@canonical.com; rjw@rjwysocki.net;
>> lenb@kernel.org; gregkh@linuxfoundation.org; davem@davemloft.net;
>> mika.westerberg@linux.intel.com; f.fainelli@gmail.com; kishon@ti.com;
>> karniksayli1995@gmail.com; linux-acpi@vger.kernel.org
>> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
>>
>> On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:
>>
>> > > > > Playing with OSI string is a bad idea. I wouldn't do anything
>> > > > > while
>> > > > > Rafael, or even Len can confirm that is the right thing to do.
>> > > > >
>> > > > > For me, AFAIK we need to be bug-to-bug compatible with Windows
>> > > > > (at least
>> > > > > on ACPICA side), so, what Windows exactly does on such laptops?
>> > > > >
>> > > >
>> > > > The issue that's being worked around isn't an ACPICA interpreter
>> > > > issue, but it's
>> > > > a graphics device configuration issue.
>>
>> Then clearly nothing to do with OSI strings here, right?
>
> The usage of OSI here is: do you support the feature "Linux-Dell-Video"
> to Linux kernel.  If Linux kernel responds yes then firmware will turn off
> RTD3 functionality for the discrete GPU.
>
>>
>> > > > Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
>> > > > don't.  It leads to system hangs on the Linux side.
>> > >
>> > > Can we adjust Linux drivers to do the right thing?
>>
>> +100
>>
>> > >  Or is it regarding
>> > > the binary NVIDIA blob?
>>
>> nVidia vs. Linux again? :-)
>>
>> >
>> > Neither Nouveau nor the NVIDIA blob have support for RTD3.
>> >
>> > Last I heard it's waiting on NVIDIA releasing something Nouveau
>> > needs.  So.. Eventually?  For now it's better to not hang though.
>>
>> Hmm... While you are talking sense, the patch itself looks like an ugly
>> hack.
>
> I don't disagree it's a workaround for a driver deficiency.
>
>>
>> > That's part of why we wanted to enable this via a transient OSI
>> > string,
>> > to let this be removed by Linux whenever the driver does grow support.
>>
>> So, means "never" then? (Assume a bit of irony here)
>>
>> I don't know how it feels for maintainers, for me it's quite unlikely to
>> go (at least in this shape).

Workarounds are never preferred, but they are needed from time to time.

There are other alternatives such as 1) _REV & acpi_rev_dmi_table, 2)
windows & linux osi strings & acpi_osi_dmi_table.

However, Linux-OEM-my_interface_name strings + OEM strings in DMI are
better choices for couple reasons:

1. Linux-OEM-my_interface_name are more informative than current
available mechanisms.

2. Linux-OEM-my_interface_name is defined in
Documentation/acpi/osi.txt as below:

_OSI("Linux-OEM-my_interface_name")
where 'OEM' is needed if this is an OEM-specific hook,
and 'my_interface_name' describes the hook, which could be a
quirk, a bug, or a bug-fix.

3. DMI OEM strings used in this patch are System IDs and they are
unlikely to repeat for years. System IDs are also more precise than
Product names.

4. The workaround is controlled and limited to specific systems as
Mario commented previously.

>
> I'm not going to be able to force NVIDIA to fix the blob or to give what's
> necessary to enable Nouvaeu for this function.  What we can do is prevent
> someone's system from hanging because of this situation.

and we never stopped poking NVIDIA to fix drivers and will never stop
trying. If NVIDIA fixes RTD3, a clean-up patch will be submit to
remove the quirks.

>
>>
>> I'm sorry I can't be much constructive here, I heard Len once about OSI
>> huge abuse by almost every party. I would rather let him speak on the
>> matter.
>
> I know that OSI has been abused in the past, but that's exactly why this
> has been implemented as seen in this patch series.
>
> It's done specifically to fit within exactly what the Linux kernel ideally
> looks for in a custom OSI feature string.
>
> * It has an OS Prefix (Linux-)
> * It's an OEM specific string (-Dell-)
> * It's a feature specific string (-Video)
> *It's specific to a single platform (not a blanket match to Dell Inc)
> *It only modifies one feature in ASL
>
>

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

[Mario.Limonciello]

> -----Original Message-----
> From: Alex Hung [mailto:alex.hung@canonical.com]
> Sent: Wednesday, February 7, 2018 2:39 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Jean Delvare <jdelvare@suse.de>; Rafael J.
> Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> gregkh@linuxfoundation.org; davem@davemloft.net;
> mika.westerberg@linux.intel.com; Florian Fainelli <f.fainelli@gmail.com>;
> kishon@ti.com; sayli karnik <karniksayli1995@gmail.com>; Linux ACPI Mailing List
> <linux-acpi@vger.kernel.org>
> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> 
> On Tue, Feb 6, 2018 at 8:24 AM,  <Mario.Limonciello@dell.com> wrote:
> >> -----Original Message-----
> >> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> >> Sent: Tuesday, February 6, 2018 7:46 AM
> >> To: Limonciello, Mario <Mario_Limonciello@Dell.com>;
> >> dmitry.torokhov@gmail.com
> >> Cc: jdelvare@suse.de; alex.hung@canonical.com; rjw@rjwysocki.net;
> >> lenb@kernel.org; gregkh@linuxfoundation.org; davem@davemloft.net;
> >> mika.westerberg@linux.intel.com; f.fainelli@gmail.com; kishon@ti.com;
> >> karniksayli1995@gmail.com; linux-acpi@vger.kernel.org
> >> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> >>
> >> On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:
> >>
> >> > > > > Playing with OSI string is a bad idea. I wouldn't do anything
> >> > > > > while
> >> > > > > Rafael, or even Len can confirm that is the right thing to do.
> >> > > > >
> >> > > > > For me, AFAIK we need to be bug-to-bug compatible with Windows
> >> > > > > (at least
> >> > > > > on ACPICA side), so, what Windows exactly does on such laptops?
> >> > > > >
> >> > > >
> >> > > > The issue that's being worked around isn't an ACPICA interpreter
> >> > > > issue, but it's
> >> > > > a graphics device configuration issue.
> >>
> >> Then clearly nothing to do with OSI strings here, right?
> >
> > The usage of OSI here is: do you support the feature "Linux-Dell-Video"
> > to Linux kernel.  If Linux kernel responds yes then firmware will turn off
> > RTD3 functionality for the discrete GPU.
> >
> >>
> >> > > > Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
> >> > > > don't.  It leads to system hangs on the Linux side.
> >> > >
> >> > > Can we adjust Linux drivers to do the right thing?
> >>
> >> +100
> >>
> >> > >  Or is it regarding
> >> > > the binary NVIDIA blob?
> >>
> >> nVidia vs. Linux again? :-)
> >>
> >> >
> >> > Neither Nouveau nor the NVIDIA blob have support for RTD3.
> >> >
> >> > Last I heard it's waiting on NVIDIA releasing something Nouveau
> >> > needs.  So.. Eventually?  For now it's better to not hang though.
> >>
> >> Hmm... While you are talking sense, the patch itself looks like an ugly
> >> hack.
> >
> > I don't disagree it's a workaround for a driver deficiency.
> >
> >>
> >> > That's part of why we wanted to enable this via a transient OSI
> >> > string,
> >> > to let this be removed by Linux whenever the driver does grow support.
> >>
> >> So, means "never" then? (Assume a bit of irony here)
> >>
> >> I don't know how it feels for maintainers, for me it's quite unlikely to
> >> go (at least in this shape).
> 
> Workarounds are never preferred, but they are needed from time to time.
> 
> There are other alternatives such as 1) _REV & acpi_rev_dmi_table, 2)
> windows & linux osi strings & acpi_osi_dmi_table.
> 
> However, Linux-OEM-my_interface_name strings + OEM strings in DMI are
> better choices for couple reasons:
> 
> 1. Linux-OEM-my_interface_name are more informative than current
> available mechanisms.
> 
> 2. Linux-OEM-my_interface_name is defined in
> Documentation/acpi/osi.txt as below:
> 
> _OSI("Linux-OEM-my_interface_name")
> where 'OEM' is needed if this is an OEM-specific hook,
> and 'my_interface_name' describes the hook, which could be a
> quirk, a bug, or a bug-fix.
> 
> 3. DMI OEM strings used in this patch are System IDs and they are
> unlikely to repeat for years. System IDs are also more precise than
> Product names.

Actually for Dell they're guaranteed to always be unique and will 
never repeat on another platform.

> 
> 4. The workaround is controlled and limited to specific systems as
> Mario commented previously.
> 
> >
> > I'm not going to be able to force NVIDIA to fix the blob or to give what's
> > necessary to enable Nouvaeu for this function.  What we can do is prevent
> > someone's system from hanging because of this situation.
> 
> and we never stopped poking NVIDIA to fix drivers and will never stop
> trying. If NVIDIA fixes RTD3, a clean-up patch will be submit to
> remove the quirks.
> 
> >
> >>
> >> I'm sorry I can't be much constructive here, I heard Len once about OSI
> >> huge abuse by almost every party. I would rather let him speak on the
> >> matter.
> >
> > I know that OSI has been abused in the past, but that's exactly why this
> > has been implemented as seen in this patch series.
> >
> > It's done specifically to fit within exactly what the Linux kernel ideally
> > looks for in a custom OSI feature string.
> >
> > * It has an OS Prefix (Linux-)
> > * It's an OEM specific string (-Dell-)
> > * It's a feature specific string (-Video)
> > *It's specific to a single platform (not a blanket match to Dell Inc)
> > *It only modifies one feature in ASL
> >
> >

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

[Rafael J. Wysocki]

On Tue, Feb 6, 2018 at 2:45 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:
>
>> > > > Playing with OSI string is a bad idea. I wouldn't do anything
>> > > > while
>> > > > Rafael, or even Len can confirm that is the right thing to do.
>> > > >
>> > > > For me, AFAIK we need to be bug-to-bug compatible with Windows
>> > > > (at least
>> > > > on ACPICA side), so, what Windows exactly does on such laptops?
>> > > >
>> > >
>> > > The issue that's being worked around isn't an ACPICA interpreter
>> > > issue, but it's
>> > > a graphics device configuration issue.
>
> Then clearly nothing to do with OSI strings here, right?
>
>> > > Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
>> > > don't.  It leads to system hangs on the Linux side.
>> >
>> > Can we adjust Linux drivers to do the right thing?
>
> +100

IMO you should only say so if *you* are willing to do the work and
have all of the requisite means for that.

>> >  Or is it regarding
>> > the binary NVIDIA blob?
>
> nVidia vs. Linux again? :-)
>
>>
>> Neither Nouveau nor the NVIDIA blob have support for RTD3.
>>
>> Last I heard it's waiting on NVIDIA releasing something Nouveau
>> needs.  So.. Eventually?  For now it's better to not hang though.
>
> Hmm... While you are talking sense, the patch itself looks like an ugly
> hack.
>
>> That's part of why we wanted to enable this via a transient OSI
>> string,
>> to let this be removed by Linux whenever the driver does grow support.
>
> So, means "never" then? (Assume a bit of irony here)
>
> I don't know how it feels for maintainers, for me it's quite unlikely to
> go (at least in this shape).
>
> I'm sorry I can't be much constructive here, I heard Len once about OSI
> huge abuse by almost every party. I would rather let him speak on the
> matter.

One thing is the merit and another thing is the patch itself.

On the merit side, with all due respect, this isn't about fixing the
drivers in question.  They aren't broken.

RTD3 (or generally runtime PM) support in Linux has been regarded as
optional so far and it is not even realistic to change this policy
ATM.  The lack of it is a matter of energy-efficiency, but not a
matter of basic correctness, as far as Linux is concerned.

If some platform firmware doesn't work correctly with an OS that
doesn't do RTD3 all over and crashes when Linux runs on it for this
reason, than that platform is simply not compatible with Linux as it
stands.

Granted, we can say "Too bad, Linux is not going to run on this
platform any time soon" (but frankly I don't see how that would
benefit anyone) or we can make the kernel give a little hint to the
platform firmware to make it behave in a Linux-compatible way.

So if we can agree that this has merit (and I personally think that it
does), let's try to make the implementation cleaner, if possible.

Thanks,
Rafael

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

[Lukas Wunner]

On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:
> Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
> don't.  It leads to system hangs on the Linux side.

So I'm on a Mac and thus indifferent to this issue, but I happen
to know a thing or two about hybrid graphics and I'm wondering
what the claim above is supposed to mean.

RTD3, that's runtime D3, right?  Because nouveau does runtime suspend
to D3cold and has been doing so since 2013.

The commit message is extremely terse as to what exact problem
the commit is trying to solve, and I haven't seen anything more
specific in this thread other than handwaving. ("waiting on NVIDIA
releasing something Nouveau needs" -- what exactly?)

So please state clearly what the problem is and why solving it
this way is the best or only solution.

Thanks,

Lukas

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

[Rafael J. Wysocki]

On Sun, Feb 11, 2018 at 2:45 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:
>> Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
>> don't.  It leads to system hangs on the Linux side.
>
> So I'm on a Mac and thus indifferent to this issue, but I happen
> to know a thing or two about hybrid graphics and I'm wondering
> what the claim above is supposed to mean.
>
> RTD3, that's runtime D3, right?  Because nouveau does runtime suspend
> to D3cold and has been doing so since 2013.
>
> The commit message is extremely terse as to what exact problem
> the commit is trying to solve, and I haven't seen anything more
> specific in this thread other than handwaving. ("waiting on NVIDIA
> releasing something Nouveau needs" -- what exactly?)
>
> So please state clearly what the problem is and why solving it
> this way is the best or only solution.

If that's not clear, I also would like to see a response to this
request before making any decisions here.

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

[Mario.Limonciello]

> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J.
> Wysocki
> Sent: Monday, February 12, 2018 3:50 AM
> To: Lukas Wunner <lukas@wunner.de>; Alex Hung <alex.hung@canonical.com>
> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; Rafael J. Wysocki
> <rafael@kernel.org>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> Dmitry Torokhov <dmitry.torokhov@gmail.com>; Jean Delvare
> <jdelvare@suse.de>; Len Brown <lenb@kernel.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; David Miller <davem@davemloft.net>; Mika
> Westerberg <mika.westerberg@linux.intel.com>; Florian Fainelli
> <f.fainelli@gmail.com>; Kishon Vijay Abraham I <kishon@ti.com>;
> karniksayli1995@gmail.com; ACPI Devel Maling List <linux-acpi@vger.kernel.org>
> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> 
> On Sun, Feb 11, 2018 at 2:45 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:
> >> Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
> >> don't.  It leads to system hangs on the Linux side.
> >
> > So I'm on a Mac and thus indifferent to this issue, but I happen
> > to know a thing or two about hybrid graphics and I'm wondering
> > what the claim above is supposed to mean.
> >
> > RTD3, that's runtime D3, right?  Because nouveau does runtime suspend
> > to D3cold and has been doing so since 2013.
> >
> > The commit message is extremely terse as to what exact problem
> > the commit is trying to solve, and I haven't seen anything more
> > specific in this thread other than handwaving. ("waiting on NVIDIA
> > releasing something Nouveau needs" -- what exactly?)
> >
> > So please state clearly what the problem is and why solving it
> > this way is the best or only solution.
> 
> If that's not clear, I also would like to see a response to this
> request before making any decisions here.

It's a lack of proper D3hot support and GC6 support in Nouveau.

As for why this is the best way to solve it?
This has been a problem for many generations and Dell has had
various different heuristics for detecting to turn off RTD3 on NV GPU
to avoid exercising it.

The patches submitted reflect a sustainable way to resolve the
problem rather than the OEM and Linux kernel playing hide and seek
to make the hardware work well in the general purpose scenario.

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

[Rafael J. Wysocki]

On Mon, Feb 12, 2018 at 9:29 PM,  <Mario.Limonciello@dell.com> wrote:
>> -----Original Message-----
>> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J.
>> Wysocki
>> Sent: Monday, February 12, 2018 3:50 AM
>> To: Lukas Wunner <lukas@wunner.de>; Alex Hung <alex.hung@canonical.com>
>> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; Rafael J. Wysocki
>> <rafael@kernel.org>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
>> Dmitry Torokhov <dmitry.torokhov@gmail.com>; Jean Delvare
>> <jdelvare@suse.de>; Len Brown <lenb@kernel.org>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>; David Miller <davem@davemloft.net>; Mika
>> Westerberg <mika.westerberg@linux.intel.com>; Florian Fainelli
>> <f.fainelli@gmail.com>; Kishon Vijay Abraham I <kishon@ti.com>;
>> karniksayli1995@gmail.com; ACPI Devel Maling List <linux-acpi@vger.kernel.org>
>> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
>>
>> On Sun, Feb 11, 2018 at 2:45 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> > On Tue, 2018-02-06 at 00:45 +0000, Mario.Limonciello@dell.com wrote:
>> >> Windows expects to use RTD3 on the NVIDIA GPU but Linux drivers
>> >> don't.  It leads to system hangs on the Linux side.
>> >
>> > So I'm on a Mac and thus indifferent to this issue, but I happen
>> > to know a thing or two about hybrid graphics and I'm wondering
>> > what the claim above is supposed to mean.
>> >
>> > RTD3, that's runtime D3, right?  Because nouveau does runtime suspend
>> > to D3cold and has been doing so since 2013.
>> >
>> > The commit message is extremely terse as to what exact problem
>> > the commit is trying to solve, and I haven't seen anything more
>> > specific in this thread other than handwaving. ("waiting on NVIDIA
>> > releasing something Nouveau needs" -- what exactly?)
>> >
>> > So please state clearly what the problem is and why solving it
>> > this way is the best or only solution.
>>
>> If that's not clear, I also would like to see a response to this
>> request before making any decisions here.
>
> It's a lack of proper D3hot support and GC6 support in Nouveau.

Thanks, but that is still a bit enigmatic.

What exactly do you mean by "proper D3hot support", in particular?

> As for why this is the best way to solve it?
> This has been a problem for many generations and Dell has had
> various different heuristics for detecting to turn off RTD3 on NV GPU
> to avoid exercising it.
>
> The patches submitted reflect a sustainable way to resolve the
> problem rather than the OEM and Linux kernel playing hide and seek
> to make the hardware work well in the general purpose scenario.

Well, I have some reservations.

While the idea of using _OSI to let the platform firmware find out
what the kernel can do is generally fine by me, the implementation
here isn't really.

In the first place, the _OSI "feature" has to be defined clearly and
in such a way that the kernel can say right away whether or not it is
"supported".  That doesn't seem to be the case here.

Second (and what follows from the above), the kernel should not need
any quirks on its side at all to give an answer.  It should be able to
say "yes" or "no" regardless of the platform it runs on if the
firmware asks the question.

So something like "Do you support native PCI power management?" would
be fine, because native PCI power management is defined by the PCI
standard and it should be clear what supporting it means and it
doesn't depend on what platform the kernel runs on.

Thanks,
Rafael

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

[Mario.Limonciello]

> >> request before making any decisions here.
> >
> > It's a lack of proper D3hot support and GC6 support in Nouveau.
> 
> Thanks, but that is still a bit enigmatic.
> 
> What exactly do you mean by "proper D3hot support", in particular?
> 

I don't believe Nouveau or NVIDIA has support for D3hot at all from 
what I've heard.  I've not dug further into this.
Maybe Canonical guys will have some more detail.

> Well, I have some reservations.
> 
> While the idea of using _OSI to let the platform firmware find out
> what the kernel can do is generally fine by me, the implementation
> here isn't really.
> 
> In the first place, the _OSI "feature" has to be defined clearly and
> in such a way that the kernel can say right away whether or not it is
> "supported".  That doesn't seem to be the case here.
> 
> Second (and what follows from the above), the kernel should not need
> any quirks on its side at all to give an answer.  It should be able to
> say "yes" or "no" regardless of the platform it runs on if the
> firmware asks the question.
> 
> So something like "Do you support native PCI power management?" would
> be fine, because native PCI power management is defined by the PCI
> standard and it should be clear what supporting it means and it
> doesn't depend on what platform the kernel runs on.
> 

The problem is this is in conflict with the documentation.
As quoted:

```
_OSI("Linux-OEM-my_interface_name")
where 'OEM' is needed if this is an OEM-specific hook,
and 'my_interface_name' describes the hook, which could be a
quirk, a bug, or a bug-fix.
```

Quite literally from an OEM perspective this is a quirk.  The affected
platforms as configured by default won't work properly with Linux.

We apply a code deviation if the OSPM responds yes to that OSI string.

It's entirely possible that the Linux kernel could not store a quirk table
to match the affected platforms and instead give a blanket simple fast 
"Yes" answer to "Linux-Dell-Video", but I think that is no better than
_OSI("Linux").  This is at least an attestation from the OEM perspective
that we have only changed one thing by this string.

The alternative (and what has been done in the past) was the ACPI
_OSI rev hack marking those platforms for quirks and other things
before that and.  I really don't want to go down that road again.

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

[Alex Hung]

On Mon, Feb 12, 2018 at 3:14 PM,  <Mario.Limonciello@dell.com> wrote:
>> >> request before making any decisions here.
>> >
>> > It's a lack of proper D3hot support and GC6 support in Nouveau.
>>
>> Thanks, but that is still a bit enigmatic.
>>
>> What exactly do you mean by "proper D3hot support", in particular?
>>
>
> I don't believe Nouveau or NVIDIA has support for D3hot at all from
> what I've heard.  I've not dug further into this.
> Maybe Canonical guys will have some more detail.

I do not have more information than current NV GPU driver doesn't
support RTD3 now but NV would support RTD3 from next new GPU chip.

If this is true, we may not need this _OSI or any workaround in next generation.

>
>> Well, I have some reservations.
>>
>> While the idea of using _OSI to let the platform firmware find out
>> what the kernel can do is generally fine by me, the implementation
>> here isn't really.
>>
>> In the first place, the _OSI "feature" has to be defined clearly and
>> in such a way that the kernel can say right away whether or not it is
>> "supported".  That doesn't seem to be the case here.
>>
>> Second (and what follows from the above), the kernel should not need
>> any quirks on its side at all to give an answer.  It should be able to
>> say "yes" or "no" regardless of the platform it runs on if the
>> firmware asks the question.
>>
>> So something like "Do you support native PCI power management?" would
>> be fine, because native PCI power management is defined by the PCI
>> standard and it should be clear what supporting it means and it
>> doesn't depend on what platform the kernel runs on.
>>
>
> The problem is this is in conflict with the documentation.
> As quoted:
>
> ```
> _OSI("Linux-OEM-my_interface_name")
> where 'OEM' is needed if this is an OEM-specific hook,
> and 'my_interface_name' describes the hook, which could be a
> quirk, a bug, or a bug-fix.
> ```
>
> Quite literally from an OEM perspective this is a quirk.  The affected
> platforms as configured by default won't work properly with Linux.
>
> We apply a code deviation if the OSPM responds yes to that OSI string.
>
> It's entirely possible that the Linux kernel could not store a quirk table
> to match the affected platforms and instead give a blanket simple fast
> "Yes" answer to "Linux-Dell-Video", but I think that is no better than
> _OSI("Linux").  This is at least an attestation from the OEM perspective
> that we have only changed one thing by this string.
>
> The alternative (and what has been done in the past) was the ACPI
> _OSI rev hack marking those platforms for quirks and other things
> before that and.  I really don't want to go down that road again.



-- 
Cheers,
Alex Hung

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

[Mika Westerberg]

On Mon, Feb 12, 2018 at 11:14:42PM +0000, Mario.Limonciello@dell.com wrote:
> > So something like "Do you support native PCI power management?" would
> > be fine, because native PCI power management is defined by the PCI
> > standard and it should be clear what supporting it means and it
> > doesn't depend on what platform the kernel runs on.
> > 
> 
> The problem is this is in conflict with the documentation.
> As quoted:
> 
> ```
> _OSI("Linux-OEM-my_interface_name")
> where 'OEM' is needed if this is an OEM-specific hook,
> and 'my_interface_name' describes the hook, which could be a
> quirk, a bug, or a bug-fix.
> ```
> 
> Quite literally from an OEM perspective this is a quirk.  The affected
> platforms as configured by default won't work properly with Linux.
> 
> We apply a code deviation if the OSPM responds yes to that OSI string.
> 
> It's entirely possible that the Linux kernel could not store a quirk table
> to match the affected platforms and instead give a blanket simple fast 
> "Yes" answer to "Linux-Dell-Video", but I think that is no better than
> _OSI("Linux").  This is at least an attestation from the OEM perspective
> that we have only changed one thing by this string.
> 
> The alternative (and what has been done in the past) was the ACPI
> _OSI rev hack marking those platforms for quirks and other things
> before that and.  I really don't want to go down that road again.

I have not followed this thread too closely so I might be missing
something but why do we need to do this in the first place? Does this
work in Windows and if yes, why we can't do the the same in Linux
without any sort of hacks and/or quirks?

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

[Rafael J. Wysocki]

On Tue, Feb 13, 2018 at 8:32 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Feb 12, 2018 at 11:14:42PM +0000, Mario.Limonciello@dell.com wrote:
>> > So something like "Do you support native PCI power management?" would
>> > be fine, because native PCI power management is defined by the PCI
>> > standard and it should be clear what supporting it means and it
>> > doesn't depend on what platform the kernel runs on.
>> >
>>
>> The problem is this is in conflict with the documentation.
>> As quoted:
>>
>> ```
>> _OSI("Linux-OEM-my_interface_name")
>> where 'OEM' is needed if this is an OEM-specific hook,
>> and 'my_interface_name' describes the hook, which could be a
>> quirk, a bug, or a bug-fix.
>> ```
>>
>> Quite literally from an OEM perspective this is a quirk.  The affected
>> platforms as configured by default won't work properly with Linux.
>>
>> We apply a code deviation if the OSPM responds yes to that OSI string.
>>
>> It's entirely possible that the Linux kernel could not store a quirk table
>> to match the affected platforms and instead give a blanket simple fast
>> "Yes" answer to "Linux-Dell-Video", but I think that is no better than
>> _OSI("Linux").  This is at least an attestation from the OEM perspective
>> that we have only changed one thing by this string.
>>
>> The alternative (and what has been done in the past) was the ACPI
>> _OSI rev hack marking those platforms for quirks and other things
>> before that and.  I really don't want to go down that road again.
>
> I have not followed this thread too closely so I might be missing
> something but why do we need to do this in the first place?

As a temporary (presumably) workaround, basically. :-)

> Does this work in Windows and if yes, why we can't do the the same in Linux
> without any sort of hacks and/or quirks?

Of course it works on Windows.

The underlying issue is that the platform firmware expects Linux to
behave like Windows, presumably because Linux says "yes" to
_OSI("Windows <something>"), and it fails to work, because Linux
doesn't behave as expected by it.

Theoretically, it should be possible to make Linux behave like Windows
in that particular respect, but (a) there may be missing pieces that
we have no access to (like some secret documentation or similar) and
(b) it is unclear how much time that would take even if everything was
known.

However, the platforms in question are (or shortly will be) shipping
and Linux quite promptly doesn't work with them.

So the idea is to make a the firmware ask the kernel for a hint on
whether or not it should adjust its behavior for a particular
difference in behavior between Linux and Windows.

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

[Mika Westerberg]

On Wed, Feb 14, 2018 at 10:06:22AM +0100, Rafael J. Wysocki wrote:
> > Does this work in Windows and if yes, why we can't do the the same in Linux
> > without any sort of hacks and/or quirks?
> 
> Of course it works on Windows.
>
> The underlying issue is that the platform firmware expects Linux to
> behave like Windows, presumably because Linux says "yes" to
> _OSI("Windows <something>"), and it fails to work, because Linux
> doesn't behave as expected by it.
> 
> Theoretically, it should be possible to make Linux behave like Windows
> in that particular respect, but (a) there may be missing pieces that
> we have no access to (like some secret documentation or similar) and
> (b) it is unclear how much time that would take even if everything was
> known.
> 
> However, the platforms in question are (or shortly will be) shipping
> and Linux quite promptly doesn't work with them.
> 
> So the idea is to make a the firmware ask the kernel for a hint on
> whether or not it should adjust its behavior for a particular
> difference in behavior between Linux and Windows.

Makes sense. Thanks for the explanation!

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

[Rafael J. Wysocki]

On Tue, Feb 13, 2018 at 12:14 AM,  <Mario.Limonciello@dell.com> wrote:
>> >> request before making any decisions here.
>> >
>> > It's a lack of proper D3hot support and GC6 support in Nouveau.
>>
>> Thanks, but that is still a bit enigmatic.
>>
>> What exactly do you mean by "proper D3hot support", in particular?
>>
>
> I don't believe Nouveau or NVIDIA has support for D3hot at all from
> what I've heard.  I've not dug further into this.

But Lukas is saying that Nouveau actually supports runtime PM, so I'm
not sure what exactly the problem is with respect to this particular
driver.

> Maybe Canonical guys will have some more detail.

OK

>> Well, I have some reservations.
>>
>> While the idea of using _OSI to let the platform firmware find out
>> what the kernel can do is generally fine by me, the implementation
>> here isn't really.
>>
>> In the first place, the _OSI "feature" has to be defined clearly and
>> in such a way that the kernel can say right away whether or not it is
>> "supported".  That doesn't seem to be the case here.
>>
>> Second (and what follows from the above), the kernel should not need
>> any quirks on its side at all to give an answer.  It should be able to
>> say "yes" or "no" regardless of the platform it runs on if the
>> firmware asks the question.
>>
>> So something like "Do you support native PCI power management?" would
>> be fine, because native PCI power management is defined by the PCI
>> standard and it should be clear what supporting it means and it
>> doesn't depend on what platform the kernel runs on.
>>
>
> The problem is this is in conflict with the documentation.
> As quoted:
>
> ```
> _OSI("Linux-OEM-my_interface_name")
> where 'OEM' is needed if this is an OEM-specific hook,
> and 'my_interface_name' describes the hook, which could be a
> quirk, a bug, or a bug-fix.
> ```
>

I don't see the conflict, sorry.

The "OEM-specific hook" is the "feature" here.  It is OEM-specific,
but the kernel still should be able to say "Yes, do that" or "No,
don't do that" without looking at the platform it is running on.

> Quite literally from an OEM perspective this is a quirk.  The affected
> platforms as configured by default won't work properly with Linux.
>
> We apply a code deviation if the OSPM responds yes to that OSI string.

Right.

> It's entirely possible that the Linux kernel could not store a quirk table
> to match the affected platforms and instead give a blanket simple fast
> "Yes" answer to "Linux-Dell-Video", but I think that is no better than
> _OSI("Linux").

First of all, _OSI("Linux") is not well-defined, because it
potentially covers everything ever done or not done by Linux,
regardless of the kernel version.  This just plain doesn't work.

"Linux-Dell-Video" could be defined precisely enough to actually work,
but IMO "Video" is too generic for an _OSI "feature" name.

Something like "Linux-Dell-quirk-this-specific-video-issue" can be made work.

> This is at least an attestation from the OEM perspective
> that we have only changed one thing by this string.

I'm not sure what you mean.

The firmware should only do
_OSI("Linux-Dell-quirk-this-specific-video-issue") if the response to
that makes a difference to it.  It knows what the platform is and it
does the _OSI() thing if the platform may be affected by the quirk
(that is, when necessary).  If the kernel sees that, it has to assume
that the platform may be affected.  Double checking in DMI is
pointless unless there are platforms abusing the _OSI(), but then the
abuse is not the kernel's problem really.

The whole point is that at one point the kernel can stop saying "yes"
to "Linux-Dell-quirk-this-specific-video-issue" if the quirk is not
necessary any more in this particular kernel version.

> The alternative (and what has been done in the past) was the ACPI
> _OSI rev hack marking those platforms for quirks and other things
> before that and.  I really don't want to go down that road again.

Right, that doesn't work just like _OSI("Linux").

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

[Lukas Wunner]

On Tue, Feb 13, 2018 at 10:18:01AM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 13, 2018 at 12:14 AM,  <Mario.Limonciello@dell.com> wrote:
> >> >> request before making any decisions here.
> >> >
> >> > It's a lack of proper D3hot support and GC6 support in Nouveau.
> >>
> >> Thanks, but that is still a bit enigmatic.
> >>
> >> What exactly do you mean by "proper D3hot support", in particular?
> >>
> >
> > I don't believe Nouveau or NVIDIA has support for D3hot at all from
> > what I've heard.  I've not dug further into this.
> 
> But Lukas is saying that Nouveau actually supports runtime PM, so I'm
> not sure what exactly the problem is with respect to this particular
> driver.

Just providing some background info in the hope that it might be useful:

nouveau runtime suspends to D3cold on Nvidia Optimus hybrid graphics
laptops, not on any other machines (e.g. desktop).  The code is in
drivers/gpu/drm/nouveau/nouveau_acpi.c.  It detects presence of Optimus
by looking for a specific _DSM.

On older machines, power was cut using that _DSM.  Newer machines instead
specify _PR3 resources for the root port above the GPU, so power is cut
once the GPU and the root port above it have runtime suspended.  nouveau
detects and supports both mechanisms, as well as the ancient "HybridPower"
machines that preceded Optimus.  Adding Peter Wu to cc, who is the most
knowledgable person I know for this.

Mario mentions GC6, doing a bit of googling reveals this has to do with
power sequencing:
https://www.manualslib.com/manual/1273261/Clevo-N850hk1.html?page=67#manual

It looks like a number of GPIO pins need to be toggled in just the right
order with just the right timing for the GPU to suspend and resume
properly.  Now, normally this should all be handled by the firmware,
but apparently there's some OS cooperation necessary but people may
not be at liberty to talk about it or haven't been told everything
by Nvidia.

AFAIK the proprietary Nvidia driver doesn't support Optimus at all.
That might be the real motivation for the patch.

Thanks,

Lukas

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

[Rafael J. Wysocki]

On Tue, Feb 13, 2018 at 10:55 AM, Lukas Wunner <lukas@wunner.de> wrote:
> On Tue, Feb 13, 2018 at 10:18:01AM +0100, Rafael J. Wysocki wrote:
>> On Tue, Feb 13, 2018 at 12:14 AM,  <Mario.Limonciello@dell.com> wrote:
>> >> >> request before making any decisions here.
>> >> >
>> >> > It's a lack of proper D3hot support and GC6 support in Nouveau.
>> >>
>> >> Thanks, but that is still a bit enigmatic.
>> >>
>> >> What exactly do you mean by "proper D3hot support", in particular?
>> >>
>> >
>> > I don't believe Nouveau or NVIDIA has support for D3hot at all from
>> > what I've heard.  I've not dug further into this.
>>
>> But Lukas is saying that Nouveau actually supports runtime PM, so I'm
>> not sure what exactly the problem is with respect to this particular
>> driver.
>
> Just providing some background info in the hope that it might be useful:
>
> nouveau runtime suspends to D3cold on Nvidia Optimus hybrid graphics
> laptops, not on any other machines (e.g. desktop).  The code is in
> drivers/gpu/drm/nouveau/nouveau_acpi.c.  It detects presence of Optimus
> by looking for a specific _DSM.
>
> On older machines, power was cut using that _DSM.  Newer machines instead
> specify _PR3 resources for the root port above the GPU, so power is cut
> once the GPU and the root port above it have runtime suspended.  nouveau
> detects and supports both mechanisms, as well as the ancient "HybridPower"
> machines that preceded Optimus.  Adding Peter Wu to cc, who is the most
> knowledgable person I know for this.
>
> Mario mentions GC6, doing a bit of googling reveals this has to do with
> power sequencing:
> https://www.manualslib.com/manual/1273261/Clevo-N850hk1.html?page=67#manual
>
> It looks like a number of GPIO pins need to be toggled in just the right
> order with just the right timing for the GPU to suspend and resume
> properly.  Now, normally this should all be handled by the firmware,
> but apparently there's some OS cooperation necessary but people may
> not be at liberty to talk about it or haven't been told everything
> by Nvidia.
>
> AFAIK the proprietary Nvidia driver doesn't support Optimus at all.
> That might be the real motivation for the patch.

OK, that helps, thanks!

It actually agrees with my last reply to Mika.

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

[Mario.Limonciello]

> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J.
> Wysocki
> Sent: Tuesday, February 13, 2018 3:18 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Lukas Wunner <lukas@wunner.de>;
> Alex Hung <alex.hung@canonical.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Jean Delvare <jdelvare@suse.de>; Len Brown
> <lenb@kernel.org>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; David
> Miller <davem@davemloft.net>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Florian Fainelli <f.fainelli@gmail.com>;
> Kishon Vijay Abraham I <kishon@ti.com>; sayli karnik
> <karniksayli1995@gmail.com>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>
> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
> 
> On Tue, Feb 13, 2018 at 12:14 AM,  <Mario.Limonciello@dell.com> wrote:
> >> >> request before making any decisions here.
> >> >
> >> > It's a lack of proper D3hot support and GC6 support in Nouveau.
> >>
> >> Thanks, but that is still a bit enigmatic.
> >>
> >> What exactly do you mean by "proper D3hot support", in particular?
> >>
> >
> > I don't believe Nouveau or NVIDIA has support for D3hot at all from
> > what I've heard.  I've not dug further into this.
> 
> But Lukas is saying that Nouveau actually supports runtime PM, so I'm
> not sure what exactly the problem is with respect to this particular
> driver.
> 
> > Maybe Canonical guys will have some more detail.
> 
> OK
> 
> >> Well, I have some reservations.
> >>
> >> While the idea of using _OSI to let the platform firmware find out
> >> what the kernel can do is generally fine by me, the implementation
> >> here isn't really.
> >>
> >> In the first place, the _OSI "feature" has to be defined clearly and
> >> in such a way that the kernel can say right away whether or not it is
> >> "supported".  That doesn't seem to be the case here.
> >>
> >> Second (and what follows from the above), the kernel should not need
> >> any quirks on its side at all to give an answer.  It should be able to
> >> say "yes" or "no" regardless of the platform it runs on if the
> >> firmware asks the question.
> >>
> >> So something like "Do you support native PCI power management?" would
> >> be fine, because native PCI power management is defined by the PCI
> >> standard and it should be clear what supporting it means and it
> >> doesn't depend on what platform the kernel runs on.
> >>
> >
> > The problem is this is in conflict with the documentation.
> > As quoted:
> >
> > ```
> > _OSI("Linux-OEM-my_interface_name")
> > where 'OEM' is needed if this is an OEM-specific hook,
> > and 'my_interface_name' describes the hook, which could be a
> > quirk, a bug, or a bug-fix.
> > ```
> >
> 
> I don't see the conflict, sorry.
> 
> The "OEM-specific hook" is the "feature" here.  It is OEM-specific,
> but the kernel still should be able to say "Yes, do that" or "No,
> don't do that" without looking at the platform it is running on.
> 
> > Quite literally from an OEM perspective this is a quirk.  The affected
> > platforms as configured by default won't work properly with Linux.
> >
> > We apply a code deviation if the OSPM responds yes to that OSI string.
> 
> Right.
> 
> > It's entirely possible that the Linux kernel could not store a quirk table
> > to match the affected platforms and instead give a blanket simple fast
> > "Yes" answer to "Linux-Dell-Video", but I think that is no better than
> > _OSI("Linux").
> 
> First of all, _OSI("Linux") is not well-defined, because it
> potentially covers everything ever done or not done by Linux,
> regardless of the kernel version.  This just plain doesn't work.
> 
> "Linux-Dell-Video" could be defined precisely enough to actually work,
> but IMO "Video" is too generic for an _OSI "feature" name.
> 
> Something like "Linux-Dell-quirk-this-specific-video-issue" can be made work.

Would:
"Linux-Dell-NVIDIA-PowerManagement"
Be more sufficient?

If not, can you please advise what would be more acceptable?

We'll have to see if there is still time to cut this in instead of Linux-Dell-Video.
or "Linux-Dell-Video" is already in stable BIOS for any of these platforms.

> 
> > This is at least an attestation from the OEM perspective
> > that we have only changed one thing by this string.
> 
> I'm not sure what you mean.
> 
> The firmware should only do
> _OSI("Linux-Dell-quirk-this-specific-video-issue") if the response to
> that makes a difference to it.  It knows what the platform is and it
> does the _OSI() thing if the platform may be affected by the quirk
> (that is, when necessary).  If the kernel sees that, it has to assume
> that the platform may be affected.  Double checking in DMI is
> pointless unless there are platforms abusing the _OSI(), but then the
> abuse is not the kernel's problem really.

The intent from the approach Alex submitted was specifically to avoid
unintentional (but potential) abuse.  Unchecked the new OSI strings can
easily turn into copy and paste for all the problems that come up and
start to behave like _OSI(Linux).  We can do our best to train everyone
that touches BIOS code but it comes from many partners and mistakes
can and will happen.

By needing to add the OEM string to the table when the systems are
tested they are tested with Linux this ensures that when a deviated
OSI string is added creates a checkpoint for the changes to be 
audited to make sure they're only changing what they should and
understood.

If you still feel that is adding undue/unnecessary process, Alex can 
drop that part of this.

I'd still like see patch 1/2 resubmitted with the proposed fixes 
however for use elsewhere in the kernel when systems need to
be detected reliably.

> 
> The whole point is that at one point the kernel can stop saying "yes"
> to "Linux-Dell-quirk-this-specific-video-issue" if the quirk is not
> necessary any more in this particular kernel version.

But then it depends on how specific the quirk actually is.

We can't possibly predict all the problems that will come up so each
quirk OSI string is going to be created as they come up.

What if one day Linux kernel supports D3hot and GC6 NV there is 
some new  technology that replaces GC6 that would otherwise 
fall under the same *-PowerManagement purview and OSI string 
proposed?  Now you need to either find all the systems that used
the string previously and quirk them or quirk the new system.

So that's the other reason why to me it is safer to build the list of
every system that needs it.  Document it in the git commit or a
comment, and if one aspect of the workaround can be dropped
in the future take those systems out of the list.

I'd like to illustrate another hypothetical example:
A lot of our machines have a DSP that goes unused in Linux.
What if we concluded that's actually a significant power drain to 
have it spun up but unused  in Linux and decide to make a:
"Linux-Dell-DisableDSP"

Later a driver comes along that can support the DSP on some
newer systems but not the older ones.  So the newer ones probably
should drop the response of yes to that OSI string if it's still in BIOS,
but how else would you avoid regressing older systems?

I guess the quirk OSI string could be
"Linux-Dell-DisableDSP-KBL-Generation"

But you see what I mean about how it's hard to predict this in
advance and better to whitelist by system?

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

[Rafael J. Wysocki]

On Wed, Feb 14, 2018 at 7:47 PM,  <Mario.Limonciello@dell.com> wrote:
>> -----Original Message-----
>> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J.
>> Wysocki
>> Sent: Tuesday, February 13, 2018 3:18 AM
>> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
>> Cc: Rafael J. Wysocki <rafael@kernel.org>; Lukas Wunner <lukas@wunner.de>;
>> Alex Hung <alex.hung@canonical.com>; Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com>; Dmitry Torokhov
>> <dmitry.torokhov@gmail.com>; Jean Delvare <jdelvare@suse.de>; Len Brown
>> <lenb@kernel.org>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; David
>> Miller <davem@davemloft.net>; Mika Westerberg
>> <mika.westerberg@linux.intel.com>; Florian Fainelli <f.fainelli@gmail.com>;
>> Kishon Vijay Abraham I <kishon@ti.com>; sayli karnik
>> <karniksayli1995@gmail.com>; ACPI Devel Maling List <linux-
>> acpi@vger.kernel.org>
>> Subject: Re: [PATCH 2/2] ACPI / osi: add DMI quirk for Dell systems
>>
>> On Tue, Feb 13, 2018 at 12:14 AM,  <Mario.Limonciello@dell.com> wrote:
>> >> >> request before making any decisions here.
>> >> >
>> >> > It's a lack of proper D3hot support and GC6 support in Nouveau.
>> >>
>> >> Thanks, but that is still a bit enigmatic.
>> >>
>> >> What exactly do you mean by "proper D3hot support", in particular?
>> >>
>> >
>> > I don't believe Nouveau or NVIDIA has support for D3hot at all from
>> > what I've heard.  I've not dug further into this.
>>
>> But Lukas is saying that Nouveau actually supports runtime PM, so I'm
>> not sure what exactly the problem is with respect to this particular
>> driver.
>>
>> > Maybe Canonical guys will have some more detail.
>>
>> OK
>>
>> >> Well, I have some reservations.
>> >>
>> >> While the idea of using _OSI to let the platform firmware find out
>> >> what the kernel can do is generally fine by me, the implementation
>> >> here isn't really.
>> >>
>> >> In the first place, the _OSI "feature" has to be defined clearly and
>> >> in such a way that the kernel can say right away whether or not it is
>> >> "supported".  That doesn't seem to be the case here.
>> >>
>> >> Second (and what follows from the above), the kernel should not need
>> >> any quirks on its side at all to give an answer.  It should be able to
>> >> say "yes" or "no" regardless of the platform it runs on if the
>> >> firmware asks the question.
>> >>
>> >> So something like "Do you support native PCI power management?" would
>> >> be fine, because native PCI power management is defined by the PCI
>> >> standard and it should be clear what supporting it means and it
>> >> doesn't depend on what platform the kernel runs on.
>> >>
>> >
>> > The problem is this is in conflict with the documentation.
>> > As quoted:
>> >
>> > ```
>> > _OSI("Linux-OEM-my_interface_name")
>> > where 'OEM' is needed if this is an OEM-specific hook,
>> > and 'my_interface_name' describes the hook, which could be a
>> > quirk, a bug, or a bug-fix.
>> > ```
>> >
>>
>> I don't see the conflict, sorry.
>>
>> The "OEM-specific hook" is the "feature" here.  It is OEM-specific,
>> but the kernel still should be able to say "Yes, do that" or "No,
>> don't do that" without looking at the platform it is running on.
>>
>> > Quite literally from an OEM perspective this is a quirk.  The affected
>> > platforms as configured by default won't work properly with Linux.
>> >
>> > We apply a code deviation if the OSPM responds yes to that OSI string.
>>
>> Right.
>>
>> > It's entirely possible that the Linux kernel could not store a quirk table
>> > to match the affected platforms and instead give a blanket simple fast
>> > "Yes" answer to "Linux-Dell-Video", but I think that is no better than
>> > _OSI("Linux").
>>
>> First of all, _OSI("Linux") is not well-defined, because it
>> potentially covers everything ever done or not done by Linux,
>> regardless of the kernel version.  This just plain doesn't work.
>>
>> "Linux-Dell-Video" could be defined precisely enough to actually work,
>> but IMO "Video" is too generic for an _OSI "feature" name.
>>
>> Something like "Linux-Dell-quirk-this-specific-video-issue" can be made work.
>
> Would:
> "Linux-Dell-NVIDIA-PowerManagement"
> Be more sufficient?

Yes, it would be better IMO.

> If not, can you please advise what would be more acceptable?
>
> We'll have to see if there is still time to cut this in instead of Linux-Dell-Video.
> or "Linux-Dell-Video" is already in stable BIOS for any of these platforms.

As I said, you can define "Linux-Dell-Video" to mean "the PM issue
with NVidia graphics", but overall the name is confusingly generic.

In any case the meaning of the string has to be precise enough for
kernel developers to know when and why it is still necessary to give a
positive response to it.

>>
>> > This is at least an attestation from the OEM perspective
>> > that we have only changed one thing by this string.
>>
>> I'm not sure what you mean.
>>
>> The firmware should only do
>> _OSI("Linux-Dell-quirk-this-specific-video-issue") if the response to
>> that makes a difference to it.  It knows what the platform is and it
>> does the _OSI() thing if the platform may be affected by the quirk
>> (that is, when necessary).  If the kernel sees that, it has to assume
>> that the platform may be affected.  Double checking in DMI is
>> pointless unless there are platforms abusing the _OSI(), but then the
>> abuse is not the kernel's problem really.
>
> The intent from the approach Alex submitted was specifically to avoid
> unintentional (but potential) abuse.  Unchecked the new OSI strings can
> easily turn into copy and paste for all the problems that come up and
> start to behave like _OSI(Linux).  We can do our best to train everyone
> that touches BIOS code but it comes from many partners and mistakes
> can and will happen.
>
> By needing to add the OEM string to the table when the systems are
> tested they are tested with Linux this ensures that when a deviated
> OSI string is added creates a checkpoint for the changes to be
> audited to make sure they're only changing what they should and
> understood.
>
> If you still feel that is adding undue/unnecessary process, Alex can
> drop that part of this.

Yes, please.  At least make it a separate patch with clearly specified
purpose.  The way it was posted caused people to think that this was
necessary part of the approach.  Also, as it stands it very much looks
like an attempt to avoid having to actually define the meaning of the
_OSI() string.

In any case, the meaning of the _OSI() string must be well-defined in
the first place.  I won't let it in otherwise.  Then, if anyone abuses
it in a way that will lead to problems when the kernel stops
responding to it positively, we can blacklist the affected platforms
at that time.

> I'd still like see patch 1/2 resubmitted with the proposed fixes
> however for use elsewhere in the kernel when systems need to
> be detected reliably.

Not without a user, though.

>>
>> The whole point is that at one point the kernel can stop saying "yes"
>> to "Linux-Dell-quirk-this-specific-video-issue" if the quirk is not
>> necessary any more in this particular kernel version.
>
> But then it depends on how specific the quirk actually is.

So that's why I'm saying that it must be well-defined.

> We can't possibly predict all the problems that will come up so each
> quirk OSI string is going to be created as they come up.
>
> What if one day Linux kernel supports D3hot and GC6 NV there is
> some new  technology that replaces GC6 that would otherwise
> fall under the same *-PowerManagement purview and OSI string
> proposed?  Now you need to either find all the systems that used
> the string previously and quirk them or quirk the new system.

If that's a new issue, technically different from the previous one, a
prudent thing to do would be to define a new _OSI() string and use
that on the new system.

> So that's the other reason why to me it is safer to build the list of
> every system that needs it.  Document it in the git commit or a
> comment, and if one aspect of the workaround can be dropped
> in the future take those systems out of the list.
>
> I'd like to illustrate another hypothetical example:
> A lot of our machines have a DSP that goes unused in Linux.
> What if we concluded that's actually a significant power drain to
> have it spun up but unused  in Linux and decide to make a:
> "Linux-Dell-DisableDSP"
>
> Later a driver comes along that can support the DSP on some
> newer systems but not the older ones.  So the newer ones probably
> should drop the response of yes to that OSI string if it's still in BIOS,
> but how else would you avoid regressing older systems?

I would define an new _OSI() string to use instead of the previous one
for the new generation.

> I guess the quirk OSI string could be
> "Linux-Dell-DisableDSP-KBL-Generation"
>
> But you see what I mean about how it's hard to predict this in
> advance and better to whitelist by system?

Yes, it is hard to predict things in general, but if your _OSI()
strings are each defined to cover a single specific issue, that
shouldn't be a problem.

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

[Mario.Limonciello]

> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@suse.de]
> Sent: Monday, February 5, 2018 7:15 AM
> To: Alex Hung <alex.hung@canonical.com>
> Cc: rjw@rjwysocki.net; lenb@kernel.org; gregkh@linuxfoundation.org;
> davem@davemloft.net; mika.westerberg@linux.intel.com;
> andriy.shevchenko@linux.intel.com; f.fainelli@gmail.com;
> dmitry.torokhov@gmail.com; kishon@ti.com; karniksayli1995@gmail.com; linux-
> acpi@vger.kernel.org; Limonciello, Mario <Mario_Limonciello@Dell.com>
> 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@canonical.com>
> > ---
> >  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

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

[Jean Delvare]

On Mon, 5 Feb 2018 17:24:43 +0000, Mario.Limonciello@dell.com wrote:
> > > +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".

I understand what you say, but can't see how this relates to my
concerns above.

-- 
Jean Delvare
SUSE L3 Support

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

[Alex Hung]

On Mon, Feb 5, 2018 at 5:14 AM, Jean Delvare <jdelvare@suse.de> wrote:
> 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.
>
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>  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 ?

Thanks for pointing this out.

This can be fixed by placing
"dmi_check_system(acpi_oem_osi_dmi_table)" call in
"early_acpi_osi_init" to fix this, as "osi_setup" used by the kernel
parameter acpi_osi is executed after early_acpi_osi_init. It is also
where other quirks are initialised too.

>
>> +                     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.

Thanks. "break" should be used here.

>
>> +             } 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?
>
> Choosing a sort rule and sticking to it would make it easier to add
> entries later with not risk of duplicates.

I was just as surprised, but they are indeed two different systems, as
pointed out by Mario.
>
>> +
>>  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



-- 
Cheers,
Alex Hung

Re: [PATCH 1/2] firmware: dmi_scan: add DMI_OEM_STRING support to dmi_matches

[Jean Delvare]

Hi Alex,

On Wed, 31 Jan 2018 00:40:04 -0800, Alex Hung wrote:
> OEM strings are defined by each OEM and they contain customized and
> useful OEM information. Supporting it provides more flexible uses of
> the dmi_matches function.
> 
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  drivers/firmware/dmi_scan.c     | 8 ++++++++
>  include/linux/mod_devicetable.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 7830419..e534d1b 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -797,11 +797,19 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
>  			else if (dmi->matches[i].exact_match &&
>  				 !strcmp(dmi_ident[s], dmi->matches[i].substr))
>  				continue;
> +		} else if (s == DMI_OEM_STRING) {
> +			const struct dmi_device *valid;
> +
> +			valid = dmi_find_device(DMI_DEV_TYPE_OEM_STRING,
> +						dmi->matches[i].substr, NULL);
> +			if (valid)
> +				continue;
>  		}
>  
>  		/* No match */
>  		return false;
>  	}
> +
>  	return true;
>  }
>  
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index abb6dc2..5739c4c 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -482,6 +482,7 @@ enum dmi_field {
>  	DMI_CHASSIS_VERSION,
>  	DMI_CHASSIS_SERIAL,
>  	DMI_CHASSIS_ASSET_TAG,
> +	DMI_OEM_STRING,
>  	DMI_STRING_MAX,
>  };
>  

This is kind of a hack, because there are more than one OEM string, so
they don't fit in dmi_ident[], but I see the value. However, reserving
one entry for them in dmi_ident[], which you will never use, is
potentially confusing, so it would have to be documented.

Did you consider adding DMI_OEM_STRING after DMI_STRING_MAX? It would
avoid the memory waste (small but still) and shouldn't be a problem if
you test this specific case early enough in dmi_matches()?

It should also be documented that only exact matches are supported for
DMI_OEM_STRING (dmi_strmatch.exact_match is ignored and considered true
always.)

Lastly you will have to rebase your patch on top of Linus' latest tree
and in particular:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8cf4e6a04f734e831c2ac7f405071d1cde690ba8

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] firmware: dmi_scan: add DMI_OEM_STRING support to dmi_matches

[Alex Hung]

On Mon, Feb 5, 2018 at 2:26 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Alex,
>
> On Wed, 31 Jan 2018 00:40:04 -0800, Alex Hung wrote:
>> OEM strings are defined by each OEM and they contain customized and
>> useful OEM information. Supporting it provides more flexible uses of
>> the dmi_matches function.
>>
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>  drivers/firmware/dmi_scan.c     | 8 ++++++++
>>  include/linux/mod_devicetable.h | 1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index 7830419..e534d1b 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -797,11 +797,19 @@ static bool dmi_matches(const struct dmi_system_id *dmi)
>>                       else if (dmi->matches[i].exact_match &&
>>                                !strcmp(dmi_ident[s], dmi->matches[i].substr))
>>                               continue;
>> +             } else if (s == DMI_OEM_STRING) {
>> +                     const struct dmi_device *valid;
>> +
>> +                     valid = dmi_find_device(DMI_DEV_TYPE_OEM_STRING,
>> +                                             dmi->matches[i].substr, NULL);
>> +                     if (valid)
>> +                             continue;
>>               }
>>
>>               /* No match */
>>               return false;
>>       }
>> +
>>       return true;
>>  }
>>
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index abb6dc2..5739c4c 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -482,6 +482,7 @@ enum dmi_field {
>>       DMI_CHASSIS_VERSION,
>>       DMI_CHASSIS_SERIAL,
>>       DMI_CHASSIS_ASSET_TAG,
>> +     DMI_OEM_STRING,
>>       DMI_STRING_MAX,
>>  };
>>
>
> This is kind of a hack, because there are more than one OEM string, so
> they don't fit in dmi_ident[], but I see the value. However, reserving
> one entry for them in dmi_ident[], which you will never use, is
> potentially confusing, so it would have to be documented.
>
> Did you consider adding DMI_OEM_STRING after DMI_STRING_MAX? It would
> avoid the memory waste (small but still) and shouldn't be a problem if
> you test this specific case early enough in dmi_matches()?
>
> It should also be documented that only exact matches are supported for
> DMI_OEM_STRING (dmi_strmatch.exact_match is ignored and considered true
> always.)

It is a good idea. I will refine, test and submit a V2.

>
> Lastly you will have to rebase your patch on top of Linus' latest tree
> and in particular:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8cf4e6a04f734e831c2ac7f405071d1cde690ba8

Will do

>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support