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 <[email protected]>
---
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
--
2.25.1
On Fri, Jul 23, 2021 at 10:25 PM Michael Bottini
<[email protected]> 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.
That is directly against the spec.
> 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.
I'm not sure. By the spec, the OS need not evaluate _DSD for a given
device more than once.
> Signed-off-by: Michael Bottini <[email protected]>
> ---
> 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);
EXPORT_SyMBOL_GPL(), please.
> 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
> --
> 2.25.1
>
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 <[email protected]>
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.
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
>
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 <[email protected]>
>
> 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
>>
On Wed, 2021-07-28 at 11:10 +0200, Hans de Goede wrote:
> 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
> > > <[email protected]>
> >
> > 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
We get that reevaluating the _DSD would be against spec. We have taken
this back to the firmware team as a bug and asked for a fix or
different solution. Thanks.
>
>
>
>
> > > ---
> > > 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
> > >
>