Re: [RFC 2/2] ACPI: OSI: Deprecate some abused _OSI strings

[Daniel Dadap <ddadap@nvidia.com>]

On 8/19/22 9:25 AM, Mario Limonciello wrote:
> The Linux-Lenovo-NV-HDMI-Audio and Linux-HPI-Hybrid-Graphics have
> been seen in the wild being abused by other vendors.  If these use
> cases are still needed for modern laptops, they should be done via
> kernel drivers instead.
>
> As we can't have nice things, mark these strings to only be applied
> to laptops from 2022 or earlier.  This should avoid breaking any
> older laptops.  In the future if the kernel drivers need to call
> Linux-only ASL for any reason, it could be a custom _DSM used only
> for Linux or something similar. This approach allows kernel developers
> to control whether to stop calling the ASL when the deficiency by
> the kernel is resolved.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   Documentation/firmware-guide/acpi/osi.rst | 24 ++++++++++-------------
>   drivers/acpi/osi.c                        | 22 +++++++++++++++------
>   2 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/firmware-guide/acpi/osi.rst b/Documentation/firmware-guide/acpi/osi.rst
> index 05869c0045d7..392b982741fe 100644
> --- a/Documentation/firmware-guide/acpi/osi.rst
> +++ b/Documentation/firmware-guide/acpi/osi.rst
> @@ -41,26 +41,22 @@ But it is likely that they will all eventually be added.
>   What should an OEM do if they want to support Linux and Windows
>   using the same BIOS image?  Often they need to do something different
>   for Linux to deal with how Linux is different from Windows.
> -Here the BIOS should ask exactly what it wants to know:
>   
> +In this case, the OEM should create custom ASL to be executed by the
> +Linux kernel and changes to Linux kernel drivers to execute this custom
> +ASL.  The easiest way to accomplish this is to introduce a device specific
> +method (_DSM) that is called from the Linux kernel.
> +
> +In the past the kernel used to support something like:
>   _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.
>   
> -In addition, the OEM should send a patch to upstream Linux
> -via the linux-acpi@vger.kernel.org mailing list.  When that patch
> -is checked into Linux, the OS will answer "YES" when the BIOS
> -on the OEM's system uses _OSI to ask if the interface is supported
> -by the OS.  Linux distributors can back-port that patch for Linux
> -pre-installs, and it will be included by all distributions that
> -re-base to upstream.  If the distribution can not update the kernel binary,
> -they can also add an acpi_osi=Linux-OEM-my_interface_name
> -cmdline parameter to the boot loader, as needed.
> -
> -If the string refers to a feature where the upstream kernel
> -eventually grows support, a patch should be sent to remove
> -the string when that support is added to the kernel.
> +However this was discovered to be abused by other BIOS vendors to change
> +completely unrelated code on completely unrelated systems.  As such it's
> +been deprecated and any old hooks will not be activated on systems from
> +2023 or later.
>   
>   That was easy.  Read on, to find out how to do it wrong.
>   
> diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
> index c2f6b2f553d9..18c339c3f277 100644
> --- a/drivers/acpi/osi.c
> +++ b/drivers/acpi/osi.c
> @@ -25,6 +25,7 @@
>   struct acpi_osi_entry {
>   	char string[OSI_STRING_LENGTH_MAX];
>   	bool enable;
> +	unsigned int max_bios_year;
>   };
>   
>   static struct acpi_osi_config {
> @@ -40,25 +41,29 @@ static struct acpi_osi_config {
>   static struct acpi_osi_config osi_config;
>   static struct acpi_osi_entry
>   osi_setup_entries[OSI_STRING_ENTRIES_MAX] __initdata = {
> -	{"Module Device", true},
> -	{"Processor Device", true},
> -	{"3.0 _SCP Extensions", true},
> -	{"Processor Aggregator Device", true},
> +	{"Module Device", true, 0},
> +	{"Processor Device", true, 0},
> +	{"3.0 _SCP Extensions", true, 0},
> +	{"Processor Aggregator Device", true, 0},


Since you're going to have to update this whole table anyway, maybe it 
would be more self-documenting if you used named initializers? e.g.:

{ .string = "Module Device", .enable = true, .max_bios_year = 0 },

For that matter, do we really need to explicitly set max_bios_year in 
entries that don't use it? They're all zero-initialized anyway due to 
the static declaration.


>   	/*
>   	 * Linux-Lenovo-NV-HDMI-Audio is used by BIOS to power on NVidia's HDMI
>   	 * audio device which is turned off for power-saving in Windows OS.
>   	 * This power management feature observed on some Lenovo Thinkpad
>   	 * systems which will not be able to output audio via HDMI without
>   	 * a BIOS workaround.
> +	 *
> +	 * This _OSI string is only applied to systems from 2022 or earlier.
>   	 */
> -	{"Linux-Lenovo-NV-HDMI-Audio", true},
> +	{"Linux-Lenovo-NV-HDMI-Audio", true, 2022},


Oof. I remember this one. Setting it to this year seems a bit generous, 
as IIUC systems haven't shipped with this feature in a few years, so 
there shouldn't be anything legitimately needing this key in recent 
years, but if it's been found being abused in the wild as you say, I 
suppose it's safer to not break things, even if they're being naughty.


>   	/*
>   	 * Linux-HPI-Hybrid-Graphics is used by BIOS to enable dGPU to
>   	 * output video directly to external monitors on HP Inc. mobile
>   	 * workstations as Nvidia and AMD VGA drivers provide limited
>   	 * hybrid graphics supports.
> +	 *
> +	 * This _OSI string is only applied to systems from 2022 or earlier.
>   	 */
> -	{"Linux-HPI-Hybrid-Graphics", true},
> +	{"Linux-HPI-Hybrid-Graphics", true, 2022},
>   };
>   
>   static u32 acpi_osi_handler(acpi_string interface, u32 supported)
> @@ -122,9 +127,11 @@ void __init acpi_osi_setup(char *str)
>   		osi = &osi_setup_entries[i];
>   		if (!strcmp(osi->string, str)) {
>   			osi->enable = enable;
> +			osi->max_bios_year = 0;
>   			break;
>   		} else if (osi->string[0] == '\0') {
>   			osi->enable = enable;
> +			osi->max_bios_year = 0;
>   			strncpy(osi->string, str, OSI_STRING_LENGTH_MAX);
>   			break;
>   		}
> @@ -225,6 +232,9 @@ static void __init acpi_osi_setup_late(void)
>   		str = osi->string;
>   		if (*str == '\0')
>   			break;
> +		if (osi->max_bios_year &&
> +		    dmi_get_bios_year() > osi->max_bios_year)
> +			continue;
>   		if (osi->enable) {
>   			status = acpi_install_interface(str);
>   			if (ACPI_SUCCESS(status))