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

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

[Michael Bottini]

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(+)

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

[PATCH 2/2] platform/x86/intel/pmc: Add PSON residency counter

[Michael Bottini]

Tiger Lake devices have the capability to track the duration
of time that their Power Supply Units (PSUs) are turned off during S0ix.
This patch adds a debugfs file `pson_residency_usec` to provide
access to this counter.

In order to determine whether the device is capable of PSON,
use acpi_init_properties() to reevaluate _DSD.

Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
---
 drivers/platform/x86/intel/pmc/core.c | 46 +++++++++++++++++++++++++++
 drivers/platform/x86/intel/pmc/core.h |  7 ++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index 7c4bf7d22fd5..6cf06aecf368 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -595,6 +595,8 @@ static const struct pmc_reg_map tgl_reg_map = {
 	.lpm_sts = tgl_lpm_maps,
 	.lpm_status_offset = TGL_LPM_STATUS_OFFSET,
 	.lpm_live_status_offset = TGL_LPM_LIVE_STATUS_OFFSET,
+	.pson_residency_offset = TGL_PSON_RESIDENCY_OFFSET,
+	.pson_residency_counter_step = TGL_PSON_RES_COUNTER_STEP,
 	.etr3_offset = ETR3_OFFSET,
 };
 
@@ -1084,6 +1086,20 @@ static int pmc_core_dev_state_get(void *data, u64 *val)
 
 DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu\n");
 
+static int pmc_core_pson_residency_get(void *data, u64 *val)
+{
+	struct pmc_dev *pmcdev = data;
+	const struct pmc_reg_map *map = pmcdev->map;
+	u32 value;
+
+	value = pmc_core_reg_read(pmcdev, map->pson_residency_offset);
+	*val = (u64)value * pmcdev->map->pson_residency_counter_step;
+
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_pson_residency, pmc_core_pson_residency_get, NULL, "%llu\n");
+
 static int pmc_core_check_read_lock_bit(struct pmc_dev *pmcdev)
 {
 	u32 value;
@@ -1788,6 +1804,30 @@ static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
 	}
 }
 
+static bool pmc_core_is_pson_residency_enabled(struct pmc_dev *pmcdev)
+{
+	struct platform_device *pdev = pmcdev->pdev;
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status status;
+	u8 val;
+
+	if (!adev)
+		return false;
+
+	acpi_init_properties(adev);
+	status = acpi_evaluate_object(adev->handle, "PSOP", NULL, NULL);
+
+	if (ACPI_FAILURE(status))
+		return false;
+
+	if (fwnode_property_read_u8(acpi_fwnode_handle(adev),
+				    "intel-cec-pson-switching-enabled-in-s0",
+				    &val))
+		return false;
+
+	return val == 1;
+}
+
 static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 {
 	debugfs_remove_recursive(pmcdev->dbgfs_dir);
@@ -1856,6 +1896,11 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 				    pmcdev->dbgfs_dir, pmcdev,
 				    &pmc_core_substate_req_regs_fops);
 	}
+
+	if (pmcdev->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
+		debugfs_create_file("pson_residency_usec", 0444,
+				    pmcdev->dbgfs_dir, pmcdev, &pmc_core_pson_residency);
+	}
 }
 
 static const struct x86_cpu_id intel_pmc_core_ids[] = {
@@ -1944,6 +1989,7 @@ static int pmc_core_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, pmcdev);
+	pmcdev->pdev = pdev;
 
 	cpu_id = x86_match_cpu(intel_pmc_core_ids);
 	if (!cpu_id)
diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index 333e25981e8e..822d77f49861 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -214,6 +214,10 @@ enum ppfear_regs {
 #define TGL_LPM_PRI_OFFSET			0x1C7C
 #define TGL_LPM_NUM_MAPS			6
 
+/* Tigerlake PSON residency register */
+#define TGL_PSON_RESIDENCY_OFFSET		0x18f8
+#define TGL_PSON_RES_COUNTER_STEP		0x7A
+
 /* Extended Test Mode Register 3 (CNL and later) */
 #define ETR3_OFFSET				0x1048
 #define ETR3_CF9GR				BIT(20)
@@ -301,6 +305,8 @@ struct pmc_reg_map {
 	const u32 lpm_residency_offset;
 	const u32 lpm_status_offset;
 	const u32 lpm_live_status_offset;
+	const u32 pson_residency_offset;
+	const u32 pson_residency_counter_step;
 	const u32 etr3_offset;
 };
 
@@ -337,6 +343,7 @@ struct pmc_dev {
 	int num_lpm_modes;
 	int lpm_en_modes[LPM_MAX_NUM_MODES];
 	u32 *lpm_req_regs;
+	struct platform_device *pdev;
 };
 
 #define pmc_for_each_mode(i, mode, pmcdev)		\
-- 
2.25.1

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

[Rafael J. Wysocki]

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
>

Re: [PATCH 2/2] platform/x86/intel/pmc: Add PSON residency counter

[David E. Box]

On Sat, 2021-07-24 at 11:29 +0300, Andy Shevchenko wrote:
> 
> 
> On Friday, July 23, 2021, Michael Bottini
> <michael.a.bottini@linux.intel.com> wrote:
> > Tiger Lake devices have the capability to track the duration
> > of time that their Power Supply Units (PSUs) are turned off during
> > S0ix.
> > This patch adds a debugfs file `pson_residency_usec` to provide
> > access to this counter.
> > 
> > In order to determine whether the device is capable of PSON,
> > use acpi_init_properties() to reevaluate _DSD.
> > 
> > 
> 
> It’s direct abuse of ACPI specification as I read it:
> 
> “_DSD must return the same data each time it is evaluated. Firmware
> should not expect it to be evaluated every time (in case it is
> implemented as a method).”
> 
> 
> NAK to the series.

Okay, we'll check with the BIOS folks. They are setting this property
from _STA. They may not expect OSPM to reevaluate _DSD. But they may
have expected that OSPM doesn't attempt to read _DSD until after _STA
is executed.

David

> 
> 
> 
> 
>  
> > Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel/pmc/core.c | 46
> > +++++++++++++++++++++++++++
> >  drivers/platform/x86/intel/pmc/core.h |  7 ++++
> >  2 files changed, 53 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel/pmc/core.c
> > b/drivers/platform/x86/intel/pmc/core.c
> > index 7c4bf7d22fd5..6cf06aecf368 100644
> > --- a/drivers/platform/x86/intel/pmc/core.c
> > +++ b/drivers/platform/x86/intel/pmc/core.c
> > @@ -595,6 +595,8 @@ static const struct pmc_reg_map tgl_reg_map = {
> >         .lpm_sts = tgl_lpm_maps,
> >         .lpm_status_offset = TGL_LPM_STATUS_OFFSET,
> >         .lpm_live_status_offset = TGL_LPM_LIVE_STATUS_OFFSET,
> > +       .pson_residency_offset = TGL_PSON_RESIDENCY_OFFSET,
> > +       .pson_residency_counter_step = TGL_PSON_RES_COUNTER_STEP,
> >         .etr3_offset = ETR3_OFFSET,
> >  };
> > 
> > @@ -1084,6 +1086,20 @@ static int pmc_core_dev_state_get(void
> > *data,
> > u64 *val)
> > 
> >  DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state,
> > pmc_core_dev_state_get,
> > NULL, "%llu\n");
> > 
> > +static int pmc_core_pson_residency_get(void *data, u64 *val)
> > +{
> > +       struct pmc_dev *pmcdev = data;
> > +       const struct pmc_reg_map *map = pmcdev->map;
> > +       u32 value;
> > +
> > +       value = pmc_core_reg_read(pmcdev, map-
> > > pson_residency_offset);
> > +       *val = (u64)value * pmcdev->map-
> > >pson_residency_counter_step;
> > +
> > +       return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_pson_residency,
> > pmc_core_pson_residency_get, NULL, "%llu\n");
> > +
> >  static int pmc_core_check_read_lock_bit(struct pmc_dev *pmcdev)
> >  {
> >         u32 value;
> > @@ -1788,6 +1804,30 @@ static void
> > pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
> >         }
> >  }
> > 
> > +static bool pmc_core_is_pson_residency_enabled(struct pmc_dev
> > *pmcdev)
> > +{
> > +       struct platform_device *pdev = pmcdev->pdev;
> > +       struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> > +       acpi_status status;
> > +       u8 val;
> > +
> > +       if (!adev)
> > +               return false;
> > +
> > +       acpi_init_properties(adev);
> > +       status = acpi_evaluate_object(adev->handle, "PSOP", NULL,
> > NULL);
> > +
> > +       if (ACPI_FAILURE(status))
> > +               return false;
> > +
> > +       if (fwnode_property_read_u8(acpi_fwnode_handle(adev),
> > +                                   "intel-cec-pson-switching-
> > enabled-in-s0",
> > +                                   &val))
> > +               return false;
> > +
> > +       return val == 1;
> > +}
> > +
> >  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> >  {
> >         debugfs_remove_recursive(pmcdev->dbgfs_dir);
> > @@ -1856,6 +1896,11 @@ static void pmc_core_dbgfs_register(struct
> > pmc_dev *pmcdev)
> >                                     pmcdev->dbgfs_dir, pmcdev,
> >                                    
> > &pmc_core_substate_req_regs_fops);
> >         }
> > +
> > +       if (pmcdev->map->pson_residency_offset &&
> > pmc_core_is_pson_residency_enabled(pmcdev)) {
> > +               debugfs_create_file("pson_residency_usec", 0444,
> > +                                   pmcdev->dbgfs_dir, pmcdev,
> > &pmc_core_pson_residency);
> > +       }
> >  }
> > 
> >  static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > @@ -1944,6 +1989,7 @@ static int pmc_core_probe(struct
> > platform_device *pdev)
> >                 return -ENOMEM;
> > 
> >         platform_set_drvdata(pdev, pmcdev);
> > +       pmcdev->pdev = pdev;
> > 
> >         cpu_id = x86_match_cpu(intel_pmc_core_ids);
> >         if (!cpu_id)
> > diff --git a/drivers/platform/x86/intel/pmc/core.h
> > b/drivers/platform/x86/intel/pmc/core.h
> > index 333e25981e8e..822d77f49861 100644
> > --- a/drivers/platform/x86/intel/pmc/core.h
> > +++ b/drivers/platform/x86/intel/pmc/core.h
> > @@ -214,6 +214,10 @@ enum ppfear_regs {
> >  #define TGL_LPM_PRI_OFFSET                     0x1C7C
> >  #define TGL_LPM_NUM_MAPS                       6
> > 
> > +/* Tigerlake PSON residency register */
> > +#define TGL_PSON_RESIDENCY_OFFSET              0x18f8
> > +#define TGL_PSON_RES_COUNTER_STEP              0x7A
> > +
> >  /* Extended Test Mode Register 3 (CNL and later) */
> >  #define ETR3_OFFSET                            0x1048
> >  #define ETR3_CF9GR                             BIT(20)
> > @@ -301,6 +305,8 @@ struct pmc_reg_map {
> >         const u32 lpm_residency_offset;
> >         const u32 lpm_status_offset;
> >         const u32 lpm_live_status_offset;
> > +       const u32 pson_residency_offset;
> > +       const u32 pson_residency_counter_step;
> >         const u32 etr3_offset;
> >  };
> > 
> > @@ -337,6 +343,7 @@ struct pmc_dev {
> >         int num_lpm_modes;
> >         int lpm_en_modes[LPM_MAX_NUM_MODES];
> >         u32 *lpm_req_regs;
> > +       struct platform_device *pdev;
> >  };
> > 
> >  #define pmc_for_each_mode(i, mode, pmcdev)             \
> > -- 
> > 2.25.1
> > 
> > 
> 
> 

Re: [PATCH 2/2] platform/x86/intel/pmc: Add PSON residency counter

[Andy Shevchenko]

On Sat, Jul 24, 2021 at 10:49 PM David E. Box
<david.e.box@linux.intel.com> wrote:
> On Sat, 2021-07-24 at 11:29 +0300, Andy Shevchenko wrote:
> > On Friday, July 23, 2021, Michael Bottini
> > <michael.a.bottini@linux.intel.com> wrote:
> > > Tiger Lake devices have the capability to track the duration
> > > of time that their Power Supply Units (PSUs) are turned off during
> > > S0ix.
> > > This patch adds a debugfs file `pson_residency_usec` to provide
> > > access to this counter.
> > >
> > > In order to determine whether the device is capable of PSON,
> > > use acpi_init_properties() to reevaluate _DSD.
> > >
> > >
> >
> > It’s direct abuse of ACPI specification as I read it:
> >
> > “_DSD must return the same data each time it is evaluated. Firmware
> > should not expect it to be evaluated every time (in case it is
> > implemented as a method).”
> >
> >
> > NAK to the series.
>
> Okay, we'll check with the BIOS folks. They are setting this property
> from _STA. They may not expect OSPM to reevaluate _DSD.

Does it matter?

> But they may
> have expected that OSPM doesn't attempt to read _DSD until after _STA
> is executed.

They may study a bit of ACPI specification perhaps?
Or if there is indeed an issue with the specification language (so
native-speaking people misinterpret above) then ECR to ASWG should be
submitted?

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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
> 

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

[Hans de Goede]

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
>>

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

[David E. Box]

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
> > > 
>