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

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

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

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

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

-- 
 i.