Re: [PATCH v2 2/9] platform/x86/amd/pmf: Fix the custom bios input handling mechanism

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

Hi Ilpo,

On 6/30/2025 18:46, Ilpo Järvinen wrote:
> On Tue, 17 Jun 2025, Shyam Sundar S K wrote:
> 
>> Originally, the 'amd_pmf_get_custom_bios_inputs()' function was written
>> under the assumption that the BIOS would only send a single pending
>> request for the driver to process. However, following OEM enablement, it
>> became clear that multiple pending requests for custom BIOS inputs might
>> be sent at the same time, a scenario that the current code logic does not
>> support when it comes to handling multiple custom BIOS inputs.
>>
>> To address this, the code logic needs to be improved to not only manage
>> multiple simultaneous custom BIOS inputs but also to ensure it is scalable
>> for future additional inputs.
>>
>> Cc: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmf/pmf.h | 12 ++++++------
>>  drivers/platform/x86/amd/pmf/spc.c | 15 ++++++---------
>>  2 files changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 1a5a8d70c360..696b170255e0 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -623,14 +623,14 @@ enum ta_slider {
>>  	TA_MAX,
>>  };
>>  
>> -enum apmf_smartpc_custom_bios_inputs {
>> -	APMF_SMARTPC_CUSTOM_BIOS_INPUT1,
>> -	APMF_SMARTPC_CUSTOM_BIOS_INPUT2,
>> +struct amd_pmf_pb_bitmap {
>> +	const char *name;
>> +	u32 bit_mask;
>>  };
>>  
>> -enum apmf_preq_smartpc {
>> -	NOTIFY_CUSTOM_BIOS_INPUT1 = 5,
>> -	NOTIFY_CUSTOM_BIOS_INPUT2,
>> +static const struct amd_pmf_pb_bitmap custom_bios_inputs[] __used = {
>> +	{"NOTIFY_CUSTOM_BIOS_INPUT1",     BIT(5)},
>> +	{"NOTIFY_CUSTOM_BIOS_INPUT2",     BIT(6)},
>>  };
>>  
>>  enum platform_type {
>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
>> index 1d90f9382024..e72c11aba31d 100644
>> --- a/drivers/platform/x86/amd/pmf/spc.c
>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>> @@ -101,18 +101,15 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *
>>  static void amd_pmf_get_custom_bios_inputs(struct amd_pmf_dev *pdev,
>>  					   struct ta_pmf_enact_table *in)
>>  {
>> +	u32 *bios_inputs[] = {&in->ev_info.bios_input1, &in->ev_info.bios_input2};
>> +	int i;
>> +
>>  	if (!pdev->req.pending_req)
>>  		return;
>>  
>> -	switch (pdev->req.pending_req) {
>> -	case BIT(NOTIFY_CUSTOM_BIOS_INPUT1):
>> -		in->ev_info.bios_input1 = pdev->req.custom_policy[APMF_SMARTPC_CUSTOM_BIOS_INPUT1];
>> -		break;
>> -	case BIT(NOTIFY_CUSTOM_BIOS_INPUT2):
>> -		in->ev_info.bios_input2 = pdev->req.custom_policy[APMF_SMARTPC_CUSTOM_BIOS_INPUT2];
>> -		break;
>> -	default:
>> -		dev_dbg(pdev->dev, "Invalid preq for BIOS input: 0x%x\n", pdev->req.pending_req);
>> +	for (i = 0; i < ARRAY_SIZE(custom_bios_inputs); i++) {
>> +		if (pdev->req.pending_req & custom_bios_inputs[i].bit_mask)
>> +			*bios_inputs[i] = pdev->req.custom_policy[i];
> 
> Hi,
> 
> After seeing this version, I think there's better way to solve the 
> discontinous layout problem.
> 
> Could you instead add a helper amd_pmf_set_ta_bios_input() or something 
> along those lines. With that, you don't need to add that local array at 
> all. I'd also convert bios_input1/2 to array so both are arrays (I 
> understand you cannot change the layout and merge the arrays). The 
> helper can then pick which of the arrays it should set to based on the 
> index.
> 
> And briefly mention in the helper's function comment, that we need to do 
> this because the layout does not have continuous area for bios inputs.
> 
> That seems simpler approach than this local array approach.
> 


Thank you for the feedback and apologies for the long delay in sending
a new version. I have sent out v3, please have a look.

Thanks,
Shyam