Re: [PATCH v3 5/6] platform/x86:intel/pmc: Show device and function number

From: Xi Pardee

Date: Thu Sep 11 2025 - 14:33:06 EST

Hi Ilpo,

Thanks for the review. I will fix the patch based on your comments and send another version separately.

On 9/8/2025 4:03 AM, Ilpo Järvinen wrote:
On Sun, 7 Sep 2025, Xi Pardee wrote:

Hi Ilpo,

Thanks for the review.

On 8/28/2025 6:56 AM, Ilpo Järvinen wrote:
On Fri, 15 Aug 2025, Xi Pardee wrote:

Add support to show device and function number for S0ix blockers. This
feature depends on S0ix blocker substate requirement table and BDF
association table. This feature is available for platforms starting from
Pather Lake.

Only a subset of S0ix blockers has device and function number associated
to it. Get the availability information from the substate requirement
table. Get the device and function number mapping information for each
S0ix blocker from the BDF association table.

Signed-off-by: Xi Pardee <xi.pardee@xxxxxxxxxxxxxxx>
---
drivers/platform/x86/intel/pmc/core.c | 182 +++++++++++++++++++++++++-
drivers/platform/x86/intel/pmc/core.h | 23 +++-
2 files changed, 203 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c
b/drivers/platform/x86/intel/pmc/core.c
index a0b948a875a5a..69ee40cbb8b8a 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c

+ const char *name = NULL;
+ struct list_head *cur;
+ struct bdf_entry *bdf;
+ struct pmc *pmc;
+
+ pmc = pmcdev->pmcs[pmcidx];
+ if (!pmc)
+ continue;
+
+ list_for_each(cur, pmc->bdf_list) {
+ bdf = list_entry(cur, struct bdf_entry, node);
+ if (bdf->name != name) {
+ seq_printf(s, "pmc%d: %30s | %15x | %15x |\n",
pmcidx,
%u
Will change it in next version.
I don't remember if I mentioned it earlier but if you're going to address
the review comment fully. There's no need to "ack" them like this. I
trust you make the changes you don't contest.

By doing so, we can both save time by only focusing on the points which
are contested or need further discussion. :-)


+ if (!maps[*r_idx][*i_idx].name)
+ (*r_idx)++;
+
+ // Iteration reaches the end of the maps
+ if (*r_idx >= arr_size)
+ return NULL;
+
+ for (; *r_idx < arr_size; (*r_idx)++) {
+ const char *ip_name;
Can't you put this to the innermost block?
Will move it in next version.
+ if (reset)
Why you need this?
The purpose of this function is to return the name of the NEXT s0ix blocker
with BDF information.

r_idx and i_idx are used to keep track of the current position of the
iteration, therefore i_idx could not be reset to 0 at the first run of the
inner for loop. After the first run of inner for loop reset should be set to
true so in next run of the outer for loop i_idx could be reset to 0 (which
mean the iteration reaches the next s0ix blocker map).
But why you cannot reset i_idx after the inner for () loop and drop
this reset variable entirely?
Yes, I will do that in next version.

+ *i_idx = 0;
+
+ for (; maps[*r_idx][*i_idx].name; reset = TRUE, (*i_idx)++) {
true

This is hard enough to understand even without that "for (;". Would
probably be better to use while () instead.
Will change to while loop in next version.
+ if (!maps[*r_idx][*i_idx].blk)
+ continue;
+
+ bool exist = **lpm_req_regs & BIT(BDF_EXIST_BIT);
+ (*lpm_req_regs)++;
+ if (exist) {
+ ip_name = maps[*r_idx][*i_idx].name;
+ (*i_idx)++;
+ return ip_name;
+ }
+ }
+ }
+ return NULL;
+}
TBH, this entire function is horrible mess, two nested iterators as
pointers, etc.

I'm very far from following all what going on here.

I suppose I've not seen this patch previously?
To achieve the NEXT name of the s0ix blocker with BDF information we need to
iterate through two (set of) maps in parallel. The s0ix_blocker_maps provide
names of all s0ix blockers and the lpm_req_regs map shows that which s0ix
blocker has associated BDF information.

if maps[*r_idx][*i_idx].blk is set, that means it is a s0ix blocker. For each
s0ix blocker, if **lpm_req_regs & BIT(BDF_EXIST_BIT) is set, that means this
blocker has associated BDF information. Pointers are used to keep track of the
current position of the two (set of) maps so the function provides the NEXT
name of the s0ix blocker with associated BDF info.

I will probably switch to use a temporary data structure, such as list, to
store all s0ix blockers with BDF info and then iterate through this list in
pmc_core_process_bdf() call instead. That will make the logic easier to follow
and maintain. I will also add more comments to next version of this patch.
My out-of-band suggestion was to convert i_idx into a correctly typed
pointer as it's the last-level array, you only need to do two things for
the pointer:

- set it to start of the next array when r_idx increases.
- increment the pointer with ++.
+static int pmc_core_process_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
u32 data,
+ unsigned int *r_idx, unsigned int *i_idx, u32
**lpm_req_regs,
+ const char **name)
+{
+ unsigned int i;
+
+ if (!data)
+ return 0;
+
+ if (!*name)
+ return -EINVAL;
+
+ for (i = BDF_FUN_LOW_BIT; i <= BDF_FUN_HIGH_BIT; i++) {
I think you can iterate 0 ... __fls(FIELD_MAX()).
Each 16 bit represents one device and the associated function numbers for one
s0ix blocker.

Bit 4-0 indicates the device number.

Bit 12-5 indicates function numbers.

Bit 15 indicates if the next 16 bit is associated to the same s0ix blocker as
the current word.

Between bit 12 and bit 5, each bit position represents one function number.
Bit 5 represents function 0 and bit 12 represents function 7. I will add more
comments in the next version.

Will change to use __fls(FIELD_MAX()) in next version.
Yes, these are fields which are to be defined with GENMASK()/BIT(). Then
this code just has to figure out how to deal with that change and my
suggestion was to use fls construct. If you find better approach, those
can be used as well but my point is that this iteration should be sourced
from the GENMASK_U16(12, 5).

+ name = pmc_core_get_next_bdf_ip_name(pmc, &r_idx, &i_idx,
&lpm_reg_regs);
+ if (!name)
+ return -EINVAL;
+
+ pmc->bdf_list = devm_kzalloc(&pmcdev->pdev->dev, sizeof(struct
list_head), GFP_KERNEL);
Should use sizeof(*xx).

But why you need to allocate the list head and not have it in place
within the pmc's struct?
The memory for bdf_list is only allocated when the bdf information is
available to achieve.

intel_pmc_core driver can check if the memory is allocated to decide whether
to create a file in debugfs for bdf in pmc_core_dbgfs_register().
But can't you use empty list check for that?
I can use the empty list check in pmc_core_dbgfs_register() instead.

+ if (!pmc->bdf_list)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(pmc->bdf_list);
+
+ for (; sample_id < max_sample_id; sample_id++) {
+ u32 data;
+
+ ret = pmt_telem_read32(ep, sample_id, &data, 1);
+ if (ret) {
+ dev_err(&pmcdev->pdev->dev,
+ "couldn't read bdf: %d\n", ret);
One line.
Will change it in next version.
+ return ret;
+ }
+
+ if (sample_id == header_id) {
+ size = (data & GENMASK(BDF_SIZE_HIGH_BIT,
BDF_SIZE_LOW_BIT))
+ >> BDF_SIZE_LOW_BIT;
Define the field and use FIELD_GET().
Will change it in next version.
+ header_id += size + 1;
No, I just cannot understand what's going on here, it's hopeless. Always
when I think I've finally understood what its all about you throw a curve
ball like this.
There is one header line (32 bit) between each type of s0ix blocker in the bdf
association table. The bit 23-26 in header line indicates the size of each
section of one specific type of s0ix blocker in this table.

header_id is used  to keep track of the id of each header so we will process
the header line differently from the other lines.

I will add more detailed comment in next version.
I suspect naming the fields with defines and using FIELD_GET() will
already get you far.

Perhaps BDF_SIZE (=what remains when you take those custom coded bit field
postfix out of the current naming) should be renamed into something like
BDF_SECTION_SIZE for better clarity.

Will use FIELD_GET() to get the information in next version.

Thanks for reviewing the patch.

Xi