Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/2] acpi: Add acpi_init_properties to ACPI driver code
To:     Michael Bottini <michael.a.bottini@linux.intel.com>,
        rjw@rjwysocki.net, lenb@kernel.org, irenic.rajneesh@gmail.com,
        david.e.box@linux.intel.com, mgross@linux.intel.com
Cc:     linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
        platform-driver-x86@vger.kernel.org
References: <20210723202157.2425-1-michael.a.bottini@linux.intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <d8e4f0f3-7282-50d4-16ac-2f67b210373c@redhat.com>
Date:   Wed, 28 Jul 2021 11:08:01 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210723202157.2425-1-michael.a.bottini@linux.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>