Message-ID: <ed041865-c5aa-1312-e401-8cf333ac0820@linux.intel.com>
Date: Mon, 8 Sep 2025 14:03:45 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Xi Pardee <xi.pardee@...ux.intel.com>
cc: irenic.rajneesh@...il.com, david.e.box@...ux.intel.com, 
    Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 5/6] platform/x86:intel/pmc: Show device and function
 number

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@...ux.intel.com>
> > > ---
> > >   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?

> > > +			*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?

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

-- 
 i.

Powered by blists - more mailing lists