[PATCH v4 1/3] platform/x86/amd/pmf: Add BIOS_INPUTS_MAX macro to replace hardcoded array size

[PATCH v4 1/3] platform/x86/amd/pmf: Add BIOS_INPUTS_MAX macro to replace hardcoded array size

[Shyam Sundar S K]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 2049 bytes --]

Define a new macro BIOS_INPUTS_MAX, to represent the maximum number of
BIOS input values. Replace hardcoded array sizes in relevant structures
with this macro to improve readability and maintainability.

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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v4:
 - No Change

v3:
 - No change

v2:
 - New patch spinned from v1
 - Add new BIOS_INPUTS_MAX macro and replace hardcoded values

 drivers/platform/x86/amd/pmf/pmf.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index bd19f2a6bc78..2145df4128cd 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -119,6 +119,7 @@ struct cookie_header {
 
 #define APTS_MAX_STATES		16
 #define CUSTOM_BIOS_INPUT_BITS	GENMASK(16, 7)
+#define BIOS_INPUTS_MAX		10
 
 typedef void (*apmf_event_handler_t)(acpi_handle handle, u32 event, void *data);
 
@@ -204,7 +205,7 @@ struct apmf_sbios_req_v1 {
 	u8 skin_temp_apu;
 	u8 skin_temp_hs2;
 	u8 enable_cnqf;
-	u32 custom_policy[10];
+	u32 custom_policy[BIOS_INPUTS_MAX];
 } __packed;
 
 struct apmf_sbios_req_v2 {
@@ -216,7 +217,7 @@ struct apmf_sbios_req_v2 {
 	u32 stt_min_limit;
 	u8 skin_temp_apu;
 	u8 skin_temp_hs2;
-	u32 custom_policy[10];
+	u32 custom_policy[BIOS_INPUTS_MAX];
 } __packed;
 
 struct apmf_fan_idx {
@@ -355,7 +356,7 @@ enum power_modes_v2 {
 };
 
 struct pmf_bios_inputs_prev {
-	u32 custom_bios_inputs[10];
+	u32 custom_bios_inputs[BIOS_INPUTS_MAX];
 };
 
 struct amd_pmf_dev {
@@ -451,7 +452,7 @@ struct os_power_slider {
 struct amd_pmf_notify_smart_pc_update {
 	u16 size;
 	u32 pending_req;
-	u32 custom_bios[10];
+	u32 custom_bios[BIOS_INPUTS_MAX];
 } __packed;
 
 struct fan_table_control {
-- 
2.34.1

[PATCH v4 2/3] platform/x86/amd/pmf: Use devm_mutex_init() for mutex initialization

[Shyam Sundar S K]

Replace mutex_init() with the devm_mutex_init(), to ensure proper mutex
cleanup during probe failure and driver removal.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
v4:
 - New patch spinned off from v3

 drivers/platform/x86/amd/pmf/core.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index bc544a4a5266..2ec4cb92e34f 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -465,9 +465,17 @@ static int amd_pmf_probe(struct platform_device *pdev)
 	if (!dev->regbase)
 		return -ENOMEM;
 
-	mutex_init(&dev->lock);
-	mutex_init(&dev->update_mutex);
-	mutex_init(&dev->cb_mutex);
+	err = devm_mutex_init(dev->dev, &dev->lock);
+	if (err)
+		return err;
+
+	err = devm_mutex_init(dev->dev, &dev->update_mutex);
+	if (err)
+		return err;
+
+	err = devm_mutex_init(dev->dev, &dev->cb_mutex);
+	if (err)
+		return err;
 
 	apmf_acpi_init(dev);
 	platform_set_drvdata(pdev, dev);
@@ -491,9 +499,6 @@ static void amd_pmf_remove(struct platform_device *pdev)
 		amd_pmf_notify_sbios_heartbeat_event_v2(dev, ON_UNLOAD);
 	apmf_acpi_deinit(dev);
 	amd_pmf_dbgfs_unregister(dev);
-	mutex_destroy(&dev->lock);
-	mutex_destroy(&dev->update_mutex);
-	mutex_destroy(&dev->cb_mutex);
 }
 
 static const struct attribute_group *amd_pmf_driver_groups[] = {
-- 
2.34.1

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

[Shyam Sundar S K]

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

Re: [PATCH v4 2/3] platform/x86/amd/pmf: Use devm_mutex_init() for mutex initialization

[Mario Limonciello]

On 11/19/25 2:58 AM, Shyam Sundar S K wrote:
> Replace mutex_init() with the devm_mutex_init(), to ensure proper mutex
> cleanup during probe failure and driver removal.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>> ---
> v4:
>   - New patch spinned off from v3
> 
>   drivers/platform/x86/amd/pmf/core.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index bc544a4a5266..2ec4cb92e34f 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -465,9 +465,17 @@ static int amd_pmf_probe(struct platform_device *pdev)
>   	if (!dev->regbase)
>   		return -ENOMEM;
>   
> -	mutex_init(&dev->lock);
> -	mutex_init(&dev->update_mutex);
> -	mutex_init(&dev->cb_mutex);
> +	err = devm_mutex_init(dev->dev, &dev->lock);
> +	if (err)
> +		return err;
> +
> +	err = devm_mutex_init(dev->dev, &dev->update_mutex);
> +	if (err)
> +		return err;
> +
> +	err = devm_mutex_init(dev->dev, &dev->cb_mutex);
> +	if (err)
> +		return err;
>   
>   	apmf_acpi_init(dev);
>   	platform_set_drvdata(pdev, dev);
> @@ -491,9 +499,6 @@ static void amd_pmf_remove(struct platform_device *pdev)
>   		amd_pmf_notify_sbios_heartbeat_event_v2(dev, ON_UNLOAD);
>   	apmf_acpi_deinit(dev);
>   	amd_pmf_dbgfs_unregister(dev);
> -	mutex_destroy(&dev->lock);
> -	mutex_destroy(&dev->update_mutex);
> -	mutex_destroy(&dev->cb_mutex);
>   }
>   
>   static const struct attribute_group *amd_pmf_driver_groups[] = {

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

[Ilpo Järvinen]

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?

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?

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.

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

[Shyam Sundar S K]

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.

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.

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.

> 
> 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, preventing ring stalls.
The dev_warn() only triggers when the buffer overflows, not from
stalled consumption.

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

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

[Ilpo Järvinen]

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

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

[Shyam Sundar S K]

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