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@linux.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
@@ -1017,6 +1017,38 @@ const struct file_operations pmc_core_substate_req_regs_fops = {
 	.release	= single_release,
 };
 
+static int pmc_core_bdf_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	unsigned int pmcidx;
+
+	seq_printf(s, "%36s | %15s | %15s |\n", "Element", "Device Number", "Function Number");
+	for (pmcidx = 0; pmcidx < ARRAY_SIZE(pmcdev->pmcs); pmcidx++) {
+		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,
+					   bdf->name, bdf->dev_num, bdf->fun_num);
+				name = bdf->name;
+			} else {
+				seq_printf(s, "%54x | %15x |\n",
+					   bdf->dev_num, bdf->fun_num);
+			}
+		}
+	}
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_bdf);
+
 static unsigned int pmc_core_get_crystal_freq(void)
 {
 	unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;
@@ -1418,6 +1450,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info
 				    pmc_dev_info->sub_req_show);
 	}
 
+	if (primary_pmc->bdf_list) {
+		debugfs_create_file("bdf", 0444, pmcdev->dbgfs_dir, pmcdev, &pmc_core_bdf_fops);
+	}
+
 	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
 		debugfs_create_file("pson_residency_usec", 0444,
 				    pmcdev->dbgfs_dir, primary_pmc, &pmc_core_pson_residency);
@@ -1521,7 +1557,7 @@ int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct tel
 	return ret;
 }
 
-int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+static int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
 				 struct telem_endpoint *ep)
 {
 	u32 num_blocker, sample_id;
@@ -1551,6 +1587,150 @@ int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
 	return 0;
 }
 
+static const char *pmc_core_get_next_bdf_ip_name(struct pmc *pmc, unsigned int *r_idx,
+						 unsigned int *i_idx, u32 **lpm_req_regs)
+{
+	const struct pmc_bit_map **maps;
+	unsigned int arr_size;
+	bool reset = FALSE;
+
+	maps = pmc->map->s0ix_blocker_maps;
+	arr_size = pmc_core_lpm_get_arr_size(maps);
+
+	// Iteration reaches the end of the bitmap array
+	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;
+
+		if (reset)
+			*i_idx = 0;
+
+		for (; maps[*r_idx][*i_idx].name; reset = TRUE, (*i_idx)++) {
+			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;
+}
+
+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++) {
+		struct bdf_entry *b_entry;
+		u32 function_data;
+
+		function_data = (data & BIT(i));
+		if (function_data) {
+			b_entry = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*b_entry), GFP_KERNEL);
+			if (!b_entry)
+				return -ENOMEM;
+			b_entry->dev_num = data & GENMASK(BDF_DEV_HIGH_BIT, BDF_DEV_LOW_BIT);
+			b_entry->fun_num = i - BDF_FUN_LOW_BIT;
+			b_entry->name = *name;
+			list_add_tail(&b_entry->node, pmc->bdf_list);
+		}
+	}
+
+	if (!(data & BIT(BDF_REQ_BIT)))
+		*name = pmc_core_get_next_bdf_ip_name(pmc, r_idx, i_idx, lpm_req_regs);
+
+	return 0;
+}
+
+static int pmc_core_pmt_get_bdf(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep)
+{
+	unsigned int sample_id, max_sample_id, header_id, size, r_idx, i_idx;
+	struct bdf_entry *entry;
+	u32 *lpm_reg_regs;
+	const char *name;
+	int ret;
+
+	header_id = pmc->map->bdf_offset;
+	sample_id = header_id;
+	max_sample_id = sample_id + pmc->map->bdf_table_size;
+	lpm_reg_regs = pmc->lpm_req_regs;
+	r_idx = 0;
+	i_idx = 0;
+
+	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);
+	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);
+			return ret;
+		}
+
+		if (sample_id == header_id) {
+			size = (data & GENMASK(BDF_SIZE_HIGH_BIT, BDF_SIZE_LOW_BIT))
+			       >> BDF_SIZE_LOW_BIT;
+			header_id += size + 1;
+			continue;
+		}
+
+		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
+		if (ret)
+			return ret;
+		data = data >> BDF_SIZE;
+		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
+		if (ret)
+			return ret;
+	}
+
+	list_for_each_entry(entry, pmc->bdf_list, node) {
+		dev_dbg(&pmcdev->pdev->dev, "bdf info: name %s, dev_num %x, fun_num %x",
+			entry->name, entry->dev_num, entry->fun_num);
+	}
+	return 0;
+}
+
+int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
+				 struct telem_endpoint *ep)
+{
+	int ret;
+
+	ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
+	if (ret)
+		return ret;
+
+	return pmc_core_pmt_get_bdf(pmcdev, pmc, ep);
+}
+
 static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
 {
 	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index bfe8fba808063..6ff2d171dc2ba 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -317,6 +317,24 @@ enum ppfear_regs {
 #define PMC_DEVID_MTL_IOEP	0x7ecf
 #define PMC_DEVID_MTL_IOEM	0x7ebf
 
+/* BDF offset */
+#define BDF_EXIST_BIT		3
+#define BDF_SIZE_HIGH_BIT	23
+#define BDF_SIZE_LOW_BIT	16
+#define BDF_DEV_HIGH_BIT	4
+#define BDF_DEV_LOW_BIT		0
+#define BDF_FUN_HIGH_BIT	12
+#define BDF_FUN_LOW_BIT		5
+#define BDF_REQ_BIT		15
+#define BDF_SIZE		16
+
+struct bdf_entry {
+	struct list_head node;
+	const char *name;
+	u32 dev_num;
+	u32 fun_num;
+};
+
 extern const char *pmc_lpm_modes[];
 
 struct pmc_bit_map {
@@ -373,6 +391,8 @@ struct pmc_reg_map {
 	const u32 s0ix_blocker_offset;
 	const u32 num_s0ix_blocker;
 	const u32 blocker_req_offset;
+	const u32 bdf_offset;
+	const u32 bdf_table_size;
 	/* Low Power Mode registers */
 	const int lpm_num_maps;
 	const int lpm_num_modes;
@@ -418,6 +438,7 @@ struct pmc {
 	const struct pmc_reg_map *map;
 	u32 *lpm_req_regs;
 	u32 ltr_ign;
+	struct list_head *bdf_list;
 };
 
 /**
@@ -540,7 +561,7 @@ extern struct pmc_dev_info ptl_pmc_dev;
 void cnl_suspend(struct pmc_dev *pmcdev);
 int cnl_resume(struct pmc_dev *pmcdev);
 int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
-int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
+int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
 				 struct telem_endpoint *ep);
 
 extern const struct file_operations pmc_core_substate_req_regs_fops;
-- 
2.43.0

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@linux.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
> @@ -1017,6 +1017,38 @@ const struct file_operations pmc_core_substate_req_regs_fops = {
>  	.release	= single_release,
>  };
>  
> +static int pmc_core_bdf_show(struct seq_file *s, void *unused)
> +{
> +	struct pmc_dev *pmcdev = s->private;
> +	unsigned int pmcidx;
> +
> +	seq_printf(s, "%36s | %15s | %15s |\n", "Element", "Device Number", "Function Number");
> +	for (pmcidx = 0; pmcidx < ARRAY_SIZE(pmcdev->pmcs); pmcidx++) {

Change this to pmc_idx and lets make it the only form that are added from 
this point on.

The other ones should be converted to it eventually, I once had a cleanup 
patch to rename them but IIRC I dropped it to not conflict with some 
feature worked. Maybe you can fit a rename change into some series so I 
won't end up conflicting your feature work :-).

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

> +					   bdf->name, bdf->dev_num, bdf->fun_num);
> +				name = bdf->name;
> +			} else {
> +				seq_printf(s, "%54x | %15x |\n",
> +					   bdf->dev_num, bdf->fun_num);
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pmc_core_bdf);
> +
>  static unsigned int pmc_core_get_crystal_freq(void)
>  {
>  	unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;
> @@ -1418,6 +1450,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info
>  				    pmc_dev_info->sub_req_show);
>  	}
>  
> +	if (primary_pmc->bdf_list) {
> +		debugfs_create_file("bdf", 0444, pmcdev->dbgfs_dir, pmcdev, &pmc_core_bdf_fops);
> +	}

Unnecessary braces.

> +
>  	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
>  		debugfs_create_file("pson_residency_usec", 0444,
>  				    pmcdev->dbgfs_dir, primary_pmc, &pmc_core_pson_residency);
> @@ -1521,7 +1557,7 @@ int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct tel
>  	return ret;
>  }
>  
> -int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
> +static int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
>  				 struct telem_endpoint *ep)
>  {
>  	u32 num_blocker, sample_id;
> @@ -1551,6 +1587,150 @@ int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
>  	return 0;
>  }
>  
> +static const char *pmc_core_get_next_bdf_ip_name(struct pmc *pmc, unsigned int *r_idx,
> +						 unsigned int *i_idx, u32 **lpm_req_regs)
> +{
> +	const struct pmc_bit_map **maps;
> +	unsigned int arr_size;
> +	bool reset = FALSE;

FALSE is a define in some obscure header (which you probably didn't 
include intentionally anyway ;-)).

Please use false.

> +
> +	maps = pmc->map->s0ix_blocker_maps;
> +	arr_size = pmc_core_lpm_get_arr_size(maps);
> +
> +	// Iteration reaches the end of the bitmap array

This driver has used exclusively /* */ comments.

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

> +		if (reset)

Why you need this?

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

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

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

> +		struct bdf_entry *b_entry;
> +		u32 function_data;
> +
> +		function_data = (data & BIT(i));
> +		if (function_data) {

Why the extra variable???

> +			b_entry = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*b_entry), GFP_KERNEL);
> +			if (!b_entry)
> +				return -ENOMEM;
> +			b_entry->dev_num = data & GENMASK(BDF_DEV_HIGH_BIT, BDF_DEV_LOW_BIT);
> +			b_entry->fun_num = i - BDF_FUN_LOW_BIT;

What "fun" stands for? Should it be "func" as is the typical short for 
"function" in BDF?

> +			b_entry->name = *name;
> +			list_add_tail(&b_entry->node, pmc->bdf_list);
> +		}
> +	}
> +
> +	if (!(data & BIT(BDF_REQ_BIT)))
> +		*name = pmc_core_get_next_bdf_ip_name(pmc, r_idx, i_idx, lpm_req_regs);
> +
> +	return 0;
> +}
> +
> +static int pmc_core_pmt_get_bdf(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep)
> +{
> +	unsigned int sample_id, max_sample_id, header_id, size, r_idx, i_idx;
> +	struct bdf_entry *entry;
> +	u32 *lpm_reg_regs;
> +	const char *name;
> +	int ret;
> +
> +	header_id = pmc->map->bdf_offset;
> +	sample_id = header_id;
> +	max_sample_id = sample_id + pmc->map->bdf_table_size;
> +	lpm_reg_regs = pmc->lpm_req_regs;
> +	r_idx = 0;
> +	i_idx = 0;
> +
> +	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?

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

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

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

In case this series is in any kind of hurry. I suggest you send the series 
without this patch and we work out this patch separately on top of the 
applied patches (I expect the patch 1-5 to be fine on next iteration).

> +			continue;
> +		}
> +
> +		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
> +		if (ret)
> +			return ret;
> +		data = data >> BDF_SIZE;
> +		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	list_for_each_entry(entry, pmc->bdf_list, node) {
> +		dev_dbg(&pmcdev->pdev->dev, "bdf info: name %s, dev_num %x, fun_num %x",
> +			entry->name, entry->dev_num, entry->fun_num);
> +	}
> +	return 0;
> +}
> +
> +int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
> +				 struct telem_endpoint *ep)
> +{
> +	int ret;
> +
> +	ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
> +	if (ret)
> +		return ret;
> +
> +	return pmc_core_pmt_get_bdf(pmcdev, pmc, ep);
> +}
> +
>  static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
>  {
>  	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index bfe8fba808063..6ff2d171dc2ba 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -317,6 +317,24 @@ enum ppfear_regs {
>  #define PMC_DEVID_MTL_IOEP	0x7ecf
>  #define PMC_DEVID_MTL_IOEM	0x7ebf
>  
> +/* BDF offset */
> +#define BDF_EXIST_BIT		3
> +#define BDF_SIZE_HIGH_BIT	23
> +#define BDF_SIZE_LOW_BIT	16
> +#define BDF_DEV_HIGH_BIT	4
> +#define BDF_DEV_LOW_BIT		0
> +#define BDF_FUN_HIGH_BIT	12
> +#define BDF_FUN_LOW_BIT		5
> +#define BDF_REQ_BIT		15
> +#define BDF_SIZE		16

Use BIT(), GENMASK() for most right here. All?

> +
> +struct bdf_entry {
> +	struct list_head node;
> +	const char *name;
> +	u32 dev_num;
> +	u32 fun_num;
> +};
> +
>  extern const char *pmc_lpm_modes[];
>  
>  struct pmc_bit_map {
> @@ -373,6 +391,8 @@ struct pmc_reg_map {
>  	const u32 s0ix_blocker_offset;
>  	const u32 num_s0ix_blocker;
>  	const u32 blocker_req_offset;
> +	const u32 bdf_offset;
> +	const u32 bdf_table_size;
>  	/* Low Power Mode registers */
>  	const int lpm_num_maps;
>  	const int lpm_num_modes;
> @@ -418,6 +438,7 @@ struct pmc {
>  	const struct pmc_reg_map *map;
>  	u32 *lpm_req_regs;
>  	u32 ltr_ign;
> +	struct list_head *bdf_list;
>  };
>  
>  /**
> @@ -540,7 +561,7 @@ extern struct pmc_dev_info ptl_pmc_dev;
>  void cnl_suspend(struct pmc_dev *pmcdev);
>  int cnl_resume(struct pmc_dev *pmcdev);
>  int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
> -int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
> +int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
>  				 struct telem_endpoint *ep);
>  
>  extern const struct file_operations pmc_core_substate_req_regs_fops;
> 

-- 
 i.

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@linux.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
>> @@ -1017,6 +1017,38 @@ const struct file_operations pmc_core_substate_req_regs_fops = {
>>   	.release	= single_release,
>>   };
>>   
>> +static int pmc_core_bdf_show(struct seq_file *s, void *unused)
>> +{
>> +	struct pmc_dev *pmcdev = s->private;
>> +	unsigned int pmcidx;
>> +
>> +	seq_printf(s, "%36s | %15s | %15s |\n", "Element", "Device Number", "Function Number");
>> +	for (pmcidx = 0; pmcidx < ARRAY_SIZE(pmcdev->pmcs); pmcidx++) {
> Change this to pmc_idx and lets make it the only form that are added from
> this point on.
>
> The other ones should be converted to it eventually, I once had a cleanup
> patch to rename them but IIRC I dropped it to not conflict with some
> feature worked. Maybe you can fit a rename change into some series so I
> won't end up conflicting your feature work :-).
I will remove this patch from this patch series and send this one 
separately later and include a cleanup patch to rename all pmc index to 
pmc_idx.
>
>> +		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.
>
>> +					   bdf->name, bdf->dev_num, bdf->fun_num);
>> +				name = bdf->name;
>> +			} else {
>> +				seq_printf(s, "%54x | %15x |\n",
>> +					   bdf->dev_num, bdf->fun_num);
>> +			}
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(pmc_core_bdf);
>> +
>>   static unsigned int pmc_core_get_crystal_freq(void)
>>   {
>>   	unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;
>> @@ -1418,6 +1450,10 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev, struct pmc_dev_info
>>   				    pmc_dev_info->sub_req_show);
>>   	}
>>   
>> +	if (primary_pmc->bdf_list) {
>> +		debugfs_create_file("bdf", 0444, pmcdev->dbgfs_dir, pmcdev, &pmc_core_bdf_fops);
>> +	}
> Unnecessary braces.
Will change it in next version.
>
>> +
>>   	if (primary_pmc->map->pson_residency_offset && pmc_core_is_pson_residency_enabled(pmcdev)) {
>>   		debugfs_create_file("pson_residency_usec", 0444,
>>   				    pmcdev->dbgfs_dir, primary_pmc, &pmc_core_pson_residency);
>> @@ -1521,7 +1557,7 @@ int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct tel
>>   	return ret;
>>   }
>>   
>> -int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
>> +static int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
>>   				 struct telem_endpoint *ep)
>>   {
>>   	u32 num_blocker, sample_id;
>> @@ -1551,6 +1587,150 @@ int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
>>   	return 0;
>>   }
>>   
>> +static const char *pmc_core_get_next_bdf_ip_name(struct pmc *pmc, unsigned int *r_idx,
>> +						 unsigned int *i_idx, u32 **lpm_req_regs)
>> +{
>> +	const struct pmc_bit_map **maps;
>> +	unsigned int arr_size;
>> +	bool reset = FALSE;
> FALSE is a define in some obscure header (which you probably didn't
> include intentionally anyway ;-)).
>
> Please use false.
Will change it to false in next version.
>
>> +
>> +	maps = pmc->map->s0ix_blocker_maps;
>> +	arr_size = pmc_core_lpm_get_arr_size(maps);
>> +
>> +	// Iteration reaches the end of the bitmap array
> This driver has used exclusively /* */ comments.
Will change it in next version.
>
>> +	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).

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

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

>
>> +		struct bdf_entry *b_entry;
>> +		u32 function_data;
>> +
>> +		function_data = (data & BIT(i));
>> +		if (function_data) {
> Why the extra variable???
Will remove the function_data variable in next version.
>
>> +			b_entry = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*b_entry), GFP_KERNEL);
>> +			if (!b_entry)
>> +				return -ENOMEM;
>> +			b_entry->dev_num = data & GENMASK(BDF_DEV_HIGH_BIT, BDF_DEV_LOW_BIT);
>> +			b_entry->fun_num = i - BDF_FUN_LOW_BIT;
> What "fun" stands for? Should it be "func" as is the typical short for
> "function" in BDF?
Will change to func_num in next version.
>
>> +			b_entry->name = *name;
>> +			list_add_tail(&b_entry->node, pmc->bdf_list);
>> +		}
>> +	}
>> +
>> +	if (!(data & BIT(BDF_REQ_BIT)))
>> +		*name = pmc_core_get_next_bdf_ip_name(pmc, r_idx, i_idx, lpm_req_regs);
>> +
>> +	return 0;
>> +}
>> +
>> +static int pmc_core_pmt_get_bdf(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep)
>> +{
>> +	unsigned int sample_id, max_sample_id, header_id, size, r_idx, i_idx;
>> +	struct bdf_entry *entry;
>> +	u32 *lpm_reg_regs;
>> +	const char *name;
>> +	int ret;
>> +
>> +	header_id = pmc->map->bdf_offset;
>> +	sample_id = header_id;
>> +	max_sample_id = sample_id + pmc->map->bdf_table_size;
>> +	lpm_reg_regs = pmc->lpm_req_regs;
>> +	r_idx = 0;
>> +	i_idx = 0;
>> +
>> +	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().

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

>
> In case this series is in any kind of hurry. I suggest you send the series
> without this patch and we work out this patch separately on top of the
> applied patches (I expect the patch 1-5 to be fine on next iteration).

I will send this patch separately from the other patches in this series.

>
>> +			continue;
>> +		}
>> +
>> +		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
>> +		if (ret)
>> +			return ret;
>> +		data = data >> BDF_SIZE;
>> +		ret = pmc_core_process_bdf(pmcdev, pmc, data, &r_idx, &i_idx, &lpm_reg_regs, &name);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	list_for_each_entry(entry, pmc->bdf_list, node) {
>> +		dev_dbg(&pmcdev->pdev->dev, "bdf info: name %s, dev_num %x, fun_num %x",
>> +			entry->name, entry->dev_num, entry->fun_num);
>> +	}
>> +	return 0;
>> +}
>> +
>> +int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
>> +				 struct telem_endpoint *ep)
>> +{
>> +	int ret;
>> +
>> +	ret = pmc_core_pmt_get_blk_sub_req(pmcdev, pmc, ep);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return pmc_core_pmt_get_bdf(pmcdev, pmc, ep);
>> +}
>> +
>>   static int pmc_core_get_telem_info(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
>>   {
>>   	struct pci_dev *pcidev __free(pci_dev_put) = NULL;
>> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
>> index bfe8fba808063..6ff2d171dc2ba 100644
>> --- a/drivers/platform/x86/intel/pmc/core.h
>> +++ b/drivers/platform/x86/intel/pmc/core.h
>> @@ -317,6 +317,24 @@ enum ppfear_regs {
>>   #define PMC_DEVID_MTL_IOEP	0x7ecf
>>   #define PMC_DEVID_MTL_IOEM	0x7ebf
>>   
>> +/* BDF offset */
>> +#define BDF_EXIST_BIT		3
>> +#define BDF_SIZE_HIGH_BIT	23
>> +#define BDF_SIZE_LOW_BIT	16
>> +#define BDF_DEV_HIGH_BIT	4
>> +#define BDF_DEV_LOW_BIT		0
>> +#define BDF_FUN_HIGH_BIT	12
>> +#define BDF_FUN_LOW_BIT		5
>> +#define BDF_REQ_BIT		15
>> +#define BDF_SIZE		16
> Use BIT(), GENMASK() for most right here. All?

Will change to use BIT() and GENMASK() in next version.

Thanks for reviewing the patch.

Xi

>
>> +
>> +struct bdf_entry {
>> +	struct list_head node;
>> +	const char *name;
>> +	u32 dev_num;
>> +	u32 fun_num;
>> +};
>> +
>>   extern const char *pmc_lpm_modes[];
>>   
>>   struct pmc_bit_map {
>> @@ -373,6 +391,8 @@ struct pmc_reg_map {
>>   	const u32 s0ix_blocker_offset;
>>   	const u32 num_s0ix_blocker;
>>   	const u32 blocker_req_offset;
>> +	const u32 bdf_offset;
>> +	const u32 bdf_table_size;
>>   	/* Low Power Mode registers */
>>   	const int lpm_num_maps;
>>   	const int lpm_num_modes;
>> @@ -418,6 +438,7 @@ struct pmc {
>>   	const struct pmc_reg_map *map;
>>   	u32 *lpm_req_regs;
>>   	u32 ltr_ign;
>> +	struct list_head *bdf_list;
>>   };
>>   
>>   /**
>> @@ -540,7 +561,7 @@ extern struct pmc_dev_info ptl_pmc_dev;
>>   void cnl_suspend(struct pmc_dev *pmcdev);
>>   int cnl_resume(struct pmc_dev *pmcdev);
>>   int pmc_core_pmt_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc, struct telem_endpoint *ep);
>> -int pmc_core_pmt_get_blk_sub_req(struct pmc_dev *pmcdev, struct pmc *pmc,
>> +int pmc_core_pmt_get_sub_req_bdf(struct pmc_dev *pmcdev, struct pmc *pmc,
>>   				 struct telem_endpoint *ep);
>>   
>>   extern const struct file_operations pmc_core_substate_req_regs_fops;
>>

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

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

>