Message-ID: <dfa61f8c-c49b-4c29-9999-cdff97fbb43f@linux.intel.com>
Date: Thu, 11 Sep 2025 11:32:57 -0700
From: Xi Pardee <xi.pardee@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...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

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@...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?
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

>

Powered by blists - more mailing lists