Re: [PATCH v5 RESEND 6/9] platform/x86/amd/pmf: Add custom BIOS input support for AMD_CPU_ID_PS

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

Hi Ilpo,

On 9/11/2025 12:29, Ilpo Järvinen wrote:
> On Wed, 10 Sep 2025, Shyam Sundar S K wrote:
>> On 9/10/2025 16:01, Ilpo Järvinen wrote:
>>> On Mon, 1 Sep 2025, Shyam Sundar S K wrote:
>>>
>>>> The PMF ACPI Specification (APMF) has been revised to version 1.3 to allow
>>>> for additional custom BIOS inputs, enabling OEMs to have more precise
>>>> thermal management of the system. This update includes adding support to
>>>> the driver using the new data structure received from the BIOS through the
>>>> existing APMF interfaces.
>>>>
>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Tested-by: Yijun Shen <Yijun.Shen@Dell.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>>  drivers/platform/x86/amd/pmf/acpi.c | 58 ++++++++++++++++++++++++++---
>>>>  drivers/platform/x86/amd/pmf/pmf.h  | 22 +++++++++++
>>>>  drivers/platform/x86/amd/pmf/spc.c  | 36 +++++++++++++++---
>>>>  3 files changed, 105 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>>> index 4982311ac045..41c34c26ceec 100644
>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>> @@ -320,6 +320,11 @@ int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v
>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS, req, sizeof(*req));
>>>>  }
>>>>  
>>>> +int apmf_get_sbios_requests_v1(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v1 *req)
>>>> +{
>>>> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS, req, sizeof(*req));
>>>> +}
>>>> +
>>>>  int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
>>>>  {
>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>>>> @@ -338,6 +343,18 @@ static void apmf_event_handler_v2(acpi_handle handle, u32 event, void *data)
>>>>  		dev_err(pmf_dev->dev, "Failed to get v2 SBIOS requests: %d\n", ret);
>>>>  }
>>>>  
>>>> +static void apmf_event_handler_v1(acpi_handle handle, u32 event, void *data)
>>>> +{
>>>> +	struct amd_pmf_dev *pmf_dev = data;
>>>> +	int ret;
>>>> +
>>>> +	guard(mutex)(&pmf_dev->cb_mutex);
>>>> +
>>>> +	ret = apmf_get_sbios_requests_v1(pmf_dev, &pmf_dev->req1);
>>>> +	if (ret)
>>>> +		dev_err(pmf_dev->dev, "Failed to get v1 SBIOS requests: %d\n", ret);
>>>> +}
>>>> +
>>>>  static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>>>  {
>>>>  	struct amd_pmf_dev *pmf_dev = data;
>>>> @@ -427,6 +444,11 @@ int apmf_get_dyn_slider_def_dc(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_
>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_DYN_SLIDER_DC, data, sizeof(*data));
>>>>  }
>>>>  
>>>> +static apmf_event_handler_t apmf_event_handlers[] = {
>>>> +	[PMF_IF_V1] = apmf_event_handler_v1,
>>>> +	[PMF_IF_V2] = apmf_event_handler_v2,
>>>> +};
>>>> +
>>>>  int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>>>>  {
>>>>  	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>>> @@ -446,13 +468,26 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>>>>  		apmf_event_handler(ahandle, 0, pmf_dev);
>>>>  	}
>>>>  
>>>> -	if (pmf_dev->smart_pc_enabled && pmf_dev->pmf_if_version == PMF_IF_V2) {
>>>> +	if (!pmf_dev->smart_pc_enabled)
>>>> +		return -EINVAL;
>>>
>>> Hi,
>>>
>>> Is this change okay? Previously this function returned 0 in this case.
>>>
>>
>> Yes - this change is okay and was introduced to address your v4
>> remarks w.r.t to code optimization.
>>
>> This function still returns 0 upon success but this additional check
>> is to make sure we don't enter the underlying code block for smart pc
>> if the feature is not enabled.
> 
> Code flow within this function is fine but this doesn't answer my main 
> concern related to the returned value. Is it okay for this function to 
> return -EINVAL if smart pc feature is not enabled? Previously this 
> function returned 0 also if smart pc was not enabled.
> 
> (Maybe you wanted to say it's okay to change the returned value from 0 to 
> -EINVAL but it's notindicated by your words, thus reiterating the 
> question.)
> 

Yes. I think its okay to return -EINVAL in this case if the smart pc
feature is not enabled.

Thanks,
Shyam