Re: [PATCH 1/2] acpi: Add acpi_init_properties to ACPI driver code

[Hans de Goede <hdegoede@redhat.com>]

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