| Message ID | 20210723202157.2425-1-michael.a.bottini@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [1/2] acpi: Add acpi_init_properties to ACPI driver code | expand |
On Fri, Jul 23, 2021 at 10:25 PM Michael Bottini <michael.a.bottini@linux.intel.com> 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 <michael.a.bottini@linux.intel.com> > --- > 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 <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. 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 <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 >>
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 > > > <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 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 > > > >
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
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> --- drivers/acpi/property.c | 1 + include/linux/acpi.h | 6 ++++++ 2 files changed, 7 insertions(+)