Re: [PATCH v4 3/3] platform/x86/amd/pmf: Use ring buffer to store custom BIOS input values

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

Hi Ilpo,

On 11/28/2025 03:48, Ilpo Järvinen wrote:
> On Fri, 28 Nov 2025, Shyam Sundar S K wrote:
>> On 11/27/2025 17:39, Ilpo Järvinen wrote:
>>> On Wed, 19 Nov 2025, Shyam Sundar S K wrote:
>>>
>>>> Custom BIOS input values can be updated by multiple sources, such as power
>>>> mode changes and sensor events, each triggering a custom BIOS input event.
>>>> When these events occur in rapid succession, new data may overwrite
>>>> previous values before they are processed, resulting in lost updates.
>>>>
>>>> To address this, introduce a fixed-size, power-of-two ring buffer to
>>>> capture every custom BIOS input event, storing both the pending request
>>>> and its associated input values. Access to the ring buffer is synchronized
>>>> using a mutex.
>>>>
>>>> The previous use of memset() to clear the pending request structure after
>>>> each event is removed, as each BIOS input value is now copied into the
>>>> buffer as a snapshot. Consumers now process entries directly from the ring
>>>> buffer, making explicit clearing of the pending request structure
>>>> unnecessary.
>>>>
>>>> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>> Tested-by: Yijun Shen <Yijun.Shen@Dell.com>
>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>> v4:
>>>>  - Do not store local copy of the ring buffer
>>>>  - use devm_mutex_init()
>>>>
>>>> v3:
>>>>  - include headers wherever missing
>>>>  - use dev_warn() instead of dev_WARN_ONCE()
>>>>  - remove generic struct names
>>>>  - enhance ringbuffer mechanism to handle common path
>>>>  - other cosmetic remarks
>>>>
>>>> v2:
>>>>  - Add dev_WARN_ONCE()
>>>>  - Change variable name rb_mutex to cbi_mutex
>>>>  - Move tail increment logic above pending request check
>>>>
>>>>  drivers/platform/x86/amd/pmf/acpi.c   | 40 +++++++++++++++++++++++++++
>>>>  drivers/platform/x86/amd/pmf/core.c   |  5 ++++
>>>>  drivers/platform/x86/amd/pmf/pmf.h    | 21 ++++++++++++++
>>>>  drivers/platform/x86/amd/pmf/spc.c    | 32 +++++++++++----------
>>>>  drivers/platform/x86/amd/pmf/tee-if.c |  2 ++
>>>>  5 files changed, 86 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>>> index 13c4fec2c7ef..3d94b03cf794 100644
>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>> @@ -9,6 +9,9 @@
>>>>   */
>>>>  
>>>>  #include <linux/acpi.h>
>>>> +#include <linux/array_size.h>
>>>> +#include <linux/cleanup.h>
>>>> +#include <linux/dev_printk.h>
>>>>  #include "pmf.h"
>>>>  
>>>>  #define APMF_CQL_NOTIFICATION  2
>>>> @@ -331,6 +334,39 @@ int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req
>>>>  									 req, sizeof(*req));
>>>>  }
>>>>  
>>>> +/* Store custom BIOS inputs data in ring buffer */
>>>> +static void amd_pmf_custom_bios_inputs_rb(struct amd_pmf_dev *pmf_dev)
>>>> +{
>>>> +	struct pmf_cbi_ring_buffer *rb = &pmf_dev->cbi_buf;
>>>> +	int i;
>>>> +
>>>> +	guard(mutex)(&pmf_dev->cbi_mutex);
>>>> +
>>>> +	switch (pmf_dev->cpu_id) {
>>>> +	case AMD_CPU_ID_PS:
>>>> +		for (i = 0; i < ARRAY_SIZE(custom_bios_inputs_v1); i++)
>>>> +			rb->data[rb->head].val[i] = pmf_dev->req1.custom_policy[i];
>>>> +		rb->data[rb->head].preq = pmf_dev->req1.pending_req;
>>>> +		break;
>>>> +	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>>>> +	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>>>> +		for (i = 0; i < ARRAY_SIZE(custom_bios_inputs); i++)
>>>> +			rb->data[rb->head].val[i] = pmf_dev->req.custom_policy[i];
>>>> +		rb->data[rb->head].preq = pmf_dev->req.pending_req;
>>>> +		break;
>>>> +	default:
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (CIRC_SPACE(rb->head, rb->tail, CUSTOM_BIOS_INPUT_RING_ENTRIES) == 0) {
>>>> +		/* Rare case: ensures the newest BIOS input value is kept */
>>>> +		dev_warn(pmf_dev->dev, "Overwriting BIOS input value, data may be lost\n");
>>>> +		rb->tail = (rb->tail + 1) & (CUSTOM_BIOS_INPUT_RING_ENTRIES - 1);
>>>> +	}
>>>> +
>>>> +	rb->head = (rb->head + 1) & (CUSTOM_BIOS_INPUT_RING_ENTRIES - 1);
>>>> +}
>>>> +
>>>>  static void amd_pmf_handle_early_preq(struct amd_pmf_dev *pdev)
>>>>  {
>>>>  	if (!pdev->cb_flag)
>>>> @@ -356,6 +392,8 @@ static void apmf_event_handler_v2(acpi_handle handle, u32 event, void *data)
>>>>  	dev_dbg(pmf_dev->dev, "Pending request (preq): 0x%x\n", pmf_dev->req.pending_req);
>>>>  
>>>>  	amd_pmf_handle_early_preq(pmf_dev);
>>>> +
>>>> +	amd_pmf_custom_bios_inputs_rb(pmf_dev);
>>>>  }
>>>>  
>>>>  static void apmf_event_handler_v1(acpi_handle handle, u32 event, void *data)
>>>> @@ -374,6 +412,8 @@ static void apmf_event_handler_v1(acpi_handle handle, u32 event, void *data)
>>>>  	dev_dbg(pmf_dev->dev, "Pending request (preq1): 0x%x\n", pmf_dev->req1.pending_req);
>>>>  
>>>>  	amd_pmf_handle_early_preq(pmf_dev);
>>>> +
>>>> +	amd_pmf_custom_bios_inputs_rb(pmf_dev);
>>>>  }
>>>>  
>>>>  static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>>>> index 2ec4cb92e34f..71421a5d7afd 100644
>>>> --- a/drivers/platform/x86/amd/pmf/core.c
>>>> +++ b/drivers/platform/x86/amd/pmf/core.c
>>>> @@ -11,6 +11,7 @@
>>>>  #include <linux/debugfs.h>
>>>>  #include <linux/iopoll.h>
>>>>  #include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>>  #include <linux/pci.h>
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/power_supply.h>
>>>> @@ -477,6 +478,10 @@ static int amd_pmf_probe(struct platform_device *pdev)
>>>>  	if (err)
>>>>  		return err;
>>>>  
>>>> +	err = devm_mutex_init(dev->dev, &dev->cbi_mutex);
>>>> +	if (err)
>>>> +		return err;
>>>> +
>>>>  	apmf_acpi_init(dev);
>>>>  	platform_set_drvdata(pdev, dev);
>>>>  	amd_pmf_dbgfs_register(dev);
>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>> index 2145df4128cd..5a18b3604b6e 100644
>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>> @@ -12,7 +12,9 @@
>>>>  #define PMF_H
>>>>  
>>>>  #include <linux/acpi.h>
>>>> +#include <linux/circ_buf.h>
>>>>  #include <linux/input.h>
>>>> +#include <linux/mutex_types.h>
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/platform_profile.h>
>>>>  
>>>> @@ -120,6 +122,7 @@ struct cookie_header {
>>>>  #define APTS_MAX_STATES		16
>>>>  #define CUSTOM_BIOS_INPUT_BITS	GENMASK(16, 7)
>>>>  #define BIOS_INPUTS_MAX		10
>>>> +#define CUSTOM_BIOS_INPUT_RING_ENTRIES	64	/* Must be power of two for CIRC_* macros */
>>>>  
>>>>  typedef void (*apmf_event_handler_t)(acpi_handle handle, u32 event, void *data);
>>>>  
>>>> @@ -359,6 +362,22 @@ struct pmf_bios_inputs_prev {
>>>>  	u32 custom_bios_inputs[BIOS_INPUTS_MAX];
>>>>  };
>>>>  
>>>> +/**
>>>> + * struct pmf_bios_input_entry - Snapshot of custom BIOS input event
>>>> + * @val: Array of custom BIOS input values
>>>> + * @preq: Pending request value associated with this event
>>>> + */
>>>> +struct pmf_bios_input_entry {
>>>> +	u32 val[BIOS_INPUTS_MAX];
>>>> +	u32 preq;
>>>> +};
>>>> +
>>>> +struct pmf_cbi_ring_buffer {
>>>> +	struct pmf_bios_input_entry data[CUSTOM_BIOS_INPUT_RING_ENTRIES];
>>>> +	int head;
>>>> +	int tail;
>>>> +};
>>>> +
>>>>  struct amd_pmf_dev {
>>>>  	void __iomem *regbase;
>>>>  	void __iomem *smu_virt_addr;
>>>> @@ -407,6 +426,8 @@ struct amd_pmf_dev {
>>>>  	struct apmf_sbios_req_v1 req1;
>>>>  	struct pmf_bios_inputs_prev cb_prev; /* To preserve custom BIOS inputs */
>>>>  	bool cb_flag;			     /* To handle first custom BIOS input */
>>>> +	struct pmf_cbi_ring_buffer cbi_buf;
>>>> +	struct mutex cbi_mutex;		     /* Protects ring buffer access */
>>>>  };
>>>>  
>>>>  struct apmf_sps_prop_granular_v2 {
>>>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
>>>> index 85192c7536b8..34fff41b86fe 100644
>>>> --- a/drivers/platform/x86/amd/pmf/spc.c
>>>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>>>> @@ -11,6 +11,7 @@
>>>>  
>>>>  #include <acpi/button.h>
>>>>  #include <linux/amd-pmf-io.h>
>>>> +#include <linux/cleanup.h>
>>>>  #include <linux/power_supply.h>
>>>>  #include <linux/units.h>
>>>>  #include "pmf.h"
>>>> @@ -132,30 +133,37 @@ static void amd_pmf_set_ta_custom_bios_input(struct ta_pmf_enact_table *in, int
>>>>  	}
>>>>  }
>>>>  
>>>> -static void amd_pmf_update_bios_inputs(struct amd_pmf_dev *pdev, u32 pending_req,
>>>> +static void amd_pmf_update_bios_inputs(struct amd_pmf_dev *pdev, struct pmf_bios_input_entry *data,
>>>>  				       const struct amd_pmf_pb_bitmap *inputs,
>>>> -				       const u32 *custom_policy, struct ta_pmf_enact_table *in)
>>>> +				       struct ta_pmf_enact_table *in)
>>>>  {
>>>>  	unsigned int i;
>>>>  
>>>>  	for (i = 0; i < ARRAY_SIZE(custom_bios_inputs); i++) {
>>>> -		if (!(pending_req & inputs[i].bit_mask))
>>>> +		if (!(data->preq & inputs[i].bit_mask))
>>>>  			continue;
>>>> -		amd_pmf_set_ta_custom_bios_input(in, i, custom_policy[i]);
>>>> -		pdev->cb_prev.custom_bios_inputs[i] = custom_policy[i];
>>>> -		dev_dbg(pdev->dev, "Custom BIOS Input[%d]: %u\n", i, custom_policy[i]);
>>>> +		amd_pmf_set_ta_custom_bios_input(in, i, data->val[i]);
>>>> +		pdev->cb_prev.custom_bios_inputs[i] = data->val[i];
>>>> +		dev_dbg(pdev->dev, "Custom BIOS Input[%d]: %u\n", i, data->val[i]);
>>>>  	}
>>>>  }
>>>>  
>>>>  static void amd_pmf_get_custom_bios_inputs(struct amd_pmf_dev *pdev,
>>>>  					   struct ta_pmf_enact_table *in)
>>>>  {
>>>> +	struct pmf_cbi_ring_buffer *rb = &pdev->cbi_buf;
>>>>  	unsigned int i;
>>>>  
>>>> +	guard(mutex)(&pdev->cbi_mutex);
>>>> +
>>>>  	for (i = 0; i < ARRAY_SIZE(custom_bios_inputs); i++)
>>>>  		amd_pmf_set_ta_custom_bios_input(in, i, pdev->cb_prev.custom_bios_inputs[i]);
>>>>  
>>>> -	if (!(pdev->req.pending_req || pdev->req1.pending_req))
>>>> +	if (CIRC_CNT(rb->head, rb->tail, CUSTOM_BIOS_INPUT_RING_ENTRIES) == 0)
>>>> +		return;	/* return if ring buffer is empty */
>>>> +
>>>> +	/* If no active custom BIOS input pending request, do not consume further work */
>>>> +	if (!rb->data[rb->tail].preq)
>>>>  		return;
>>>
>>> I'm left usure if "do not consume further work" comment really means that 
>>> the entry is supposed to not get removed from the ring, which stalls the 
>>> ring forever?
>>
>> No. The amd_pmf_populate_ta_inputs() function runs periodically and
>> calls amd_pmf_get_custom_bios_inputs(). The preq check prevents
>> unnecessary processing when the ring buffer is empty or contains no
>> valid pending requests.
> 
> At this point, the ring buffer IS NOT EMPTY as you just checked for it 
> above this check! So no, this preq check cannot ever find ring buffer 
> empty.
> 
>> Regarding the ring stall:
>> Entries are added to the ring buffer only when a valid ACPI event
>> triggers the event handler. Once an entry with preq set is queued, the
>> consumer processes it and increments the tail.
> 
> Who is consuming them? This function? And it keeps returning if the entry 
> at rb->tail has zero preq.
> 
>> The check acts as a guard when the periodic function runs but no new
>> events are available - either the ring buffer is empty or the current
>> entry has no active pending request.
> 
> But the entry is left in place so the next invocation finds "no active 
> pending requests", and the entry is again left in place, and so on.
> 
>>> If that's the wanted behavior, does that imply overwrite dev_warn() above 
>>> can spam the logs from that point on as the ring can fill up without 
>>> anything consuming entries from it?
>>
>> Entries with valid preq are always consumed,
> 
> True, but I'm talking about the opposite case, that is, when the 
> rb->tail entry has an entry with zero preq!
> 
>> preventing ring stalls.
> 
> This returns when !preq so I don't know what would prevent the stalling. 
> Please explain (but please think twice whether the explanation you're 
> going to give is sound!).
> 
>> The dev_warn() only triggers when the buffer overflows, not from
>> stalled consumption.
> 
> Stalled consumer and queuing more and more entries to the circular buffer 
> will eventually lead to overflow, no matter what.
> 
> The overflow would likely actually destall it by overwriting the entry 
> with zero preq but that'd just hide to logic bug you have here.
> 
> 
> I think what you should do is to consume the entry even when it has !preq
> (that is, advance rb->tail also in that case; I'm not talking about other 
> processing you do but getting rid of the "nothing to do" entry at the 
> rb->tail).

ah! great feedback. Let me fix these bugs and resend.

Thanks,
Shyam

> 
>>> I've taken first two patches into review-ilpo-next.
>>>
>>>>  	if (!pdev->smart_pc_enabled)
>>>> @@ -165,20 +173,16 @@ static void amd_pmf_get_custom_bios_inputs(struct amd_pmf_dev *pdev,
>>>>  	case PMF_IF_V1:
>>>>  		if (!is_apmf_bios_input_notifications_supported(pdev))
>>>>  			return;
>>>> -		amd_pmf_update_bios_inputs(pdev, pdev->req1.pending_req, custom_bios_inputs_v1,
>>>> -					   pdev->req1.custom_policy, in);
>>>> +		amd_pmf_update_bios_inputs(pdev, &rb->data[rb->tail], custom_bios_inputs_v1, in);
>>>>  		break;
>>>>  	case PMF_IF_V2:
>>>> -		amd_pmf_update_bios_inputs(pdev, pdev->req.pending_req, custom_bios_inputs,
>>>> -					   pdev->req.custom_policy, in);
>>>> +		amd_pmf_update_bios_inputs(pdev, &rb->data[rb->tail], custom_bios_inputs, in);
>>>>  		break;
>>>>  	default:
>>>>  		break;
>>>>  	}
>>>>  
>>>> -	/* Clear pending requests after handling */
>>>> -	memset(&pdev->req, 0, sizeof(pdev->req));
>>>> -	memset(&pdev->req1, 0, sizeof(pdev->req1));
>>>> +	rb->tail = (rb->tail + 1) & (CUSTOM_BIOS_INPUT_RING_ENTRIES - 1);
>>>>  }
>>>>  
>>>>  static void amd_pmf_get_c0_residency(u16 *core_res, size_t size, struct ta_pmf_enact_table *in)
>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> index 6e8116bef4f6..add742e33e1e 100644
>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> @@ -579,6 +579,8 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>>>  		status = ret == TA_PMF_TYPE_SUCCESS;
>>>>  		if (status) {
>>>>  			dev->cb_flag = true;
>>>> +			dev->cbi_buf.head = 0;
>>>> +			dev->cbi_buf.tail = 0;
>>>>  			break;
>>>>  		}
>>>>  		amd_pmf_tee_deinit(dev);
>>>>
>>>
>>
>