linux-kernel - Re: [PATCH 1/2] acpi: Add acpi_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 28 Jul 2021 11:10:21 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Michael Bottini <michael.a.bottini@...ux.intel.com>,
        rjw@...ysocki.net, lenb@...nel.org, irenic.rajneesh@...il.com,
        david.e.box@...ux.intel.com, mgross@...ux.intel.com
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 1/2] acpi: Add acpi_init_properties to ACPI driver code

Hi,

On 7/28/21 11:08 AM, Hans de Goede wrote:
> Hi,
> 
> On 7/23/21 10:21 PM, Michael Bottini wrote:
>> Some products in the field, like Intel Rocket Lake systems, contain
>> AML code that can modify _DSD properties after they have been
>> evaluated by ACPI init code. Therefore, there is a need for drivers
>> to be able to reevaluate _DSDs so that the updated property values can
>> be read. Export acpi_init_properties() for this purpose.
>>
>> Signed-off-by: Michael Bottini <michael.a.bottini@...ux.intel.com>
> 
> My first instinct here is this is a firmware bug and we should
> go out of our way here to not support this and to instead apply
> pressure on the vendor to get the firmware fixed.
> 
> Let me explain, the standard use of _DSD is to allow embedding
> open-firmware/devicetree style properties inside ACPI nodes.
> 
> devicetree files, unlike AML contain static information, which
> is parsed once and only once.
> 
> Allowing AML code to dynamically change _DSD results pretty
> much breaks this entire model.
> 
> So I might be shooting from the hip a bit here:
> "no, just no". IOW nack.

I should have read the rest of the thread first I guess.

I see that Andy and Rafael are saying the same thing.

So here we have 3 people who all 3 are somewhat experts in ACPI
saying no to this. So yes please talk to the BIOS team as you
indicated elsewhere in the thread.

Regards,

Hans




>> ---
>>  drivers/acpi/property.c | 1 +
>>  include/linux/acpi.h    | 6 ++++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
>> index e312ebaed8db..2c1f8cf1a8f0 100644
>> --- a/drivers/acpi/property.c
>> +++ b/drivers/acpi/property.c
>> @@ -432,6 +432,7 @@ void acpi_init_properties(struct acpi_device *adev)
>>  	if (!adev->data.pointer)
>>  		acpi_extract_apple_properties(adev);
>>  }
>> +EXPORT_SYMBOL(acpi_init_properties);
>>  
>>  static void acpi_destroy_nondev_subnodes(struct list_head *list)
>>  {
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 72e4f7fd268c..57defc3bc9b9 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -716,6 +716,8 @@ static inline u64 acpi_arch_get_root_pointer(void)
>>  
>>  int acpi_get_local_address(acpi_handle handle, u32 *addr);
>>  
>> +void acpi_init_properties(struct acpi_device *adev);
>> +
>>  #else	/* !CONFIG_ACPI */
>>  
>>  #define acpi_disabled 1
>> @@ -976,6 +978,10 @@ static inline int acpi_get_local_address(acpi_handle handle, u32 *addr)
>>  	return -ENODEV;
>>  }
>>  
>> +static inline void acpi_init_properties(struct acpi_device *adev)
>> +{
>> +}
>> +
>>  #endif	/* !CONFIG_ACPI */
>>  
>>  #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
>>