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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4233 bytes --]

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

-- 
 i.