| Message ID | 20250115152600.26482-2-huaisheng.ye@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl/core/regs: Refactor out functions to count regblocks of given type | expand |
Some minor comments that Dave can fixup: Huaisheng Ye wrote: > In commit d717d7f3df18494baafd9595fb4bcb9c380d7389, cxl_count_regblock was When referencing a commit the typical form is d717d7f3df18 ("cxl: Add functions to get an instance of / count regblocks of a given type") ...however in this case the commit history does not matter. Typically commit history is useful for fixes to identify what kernels are suspectible to the problem. In this case it is just a cleanup. Also, for what this patch is I think the commit message can afford to be more concise and just say: --- cxl_find_regblock_instance() counts the number of instances of a register block as a side effect of searching through all available register blocks. cxl_count_regblock() throws away that work and recounts all the register blocks by asking cxl_find_regblock_instance() to redo work it has already done until it finally returns an error, that is needlessly wasteful. Let cxl_count_regblock() leverage the counting that cxl_find_regblock_instance() already does by passing in a sentinel value (CXL_INSTANCES_COUNT) that triggers the count to be returned. --- Otherwise, patch looks good. Reviewed-by: Dan Williams <dan.j.williams@intel.com>
From: Williams, Dan J <dan.j.williams@intel.com> Sent: Wednesday, January 22, 2025 5:14 PM > To: Ye, Huaisheng <huaisheng.ye@intel.com>; Jonathan.Cameron@huawei.com; > Williams, Dan J <dan.j.williams@intel.com>; Jiang, Dave > <dave.jiang@intel.com>; ming.li@zohomail.com > Cc: Jia, Pei P <pei.p.jia@intel.com>; linux-cxl@vger.kernel.org; Ye, > Huaisheng <huaisheng.ye@intel.com> > Subject: Re: [PATCH v5 1/1] cxl/core/regs: Refactor out functions to count > regblocks of given type > > Some minor comments that Dave can fixup: > > Huaisheng Ye wrote: > > In commit d717d7f3df18494baafd9595fb4bcb9c380d7389, cxl_count_regblock > was > > When referencing a commit the typical form is > > d717d7f3df18 ("cxl: Add functions to get an instance of / count regblocks > of a given type") > > ...however in this case the commit history does not matter. Typically > commit history is useful for fixes to identify what kernels are > suspectible to the problem. In this case it is just a cleanup. > > Also, for what this patch is I think the commit message can afford to be > more concise and just say: > > --- > cxl_find_regblock_instance() counts the number of instances of a register > block as a side effect of searching through all available register blocks. > cxl_count_regblock() throws away that work and recounts all the register > blocks by asking cxl_find_regblock_instance() to redo work it has > already done until it finally returns an error, that is needlessly > wasteful. > > Let cxl_count_regblock() leverage the counting that > cxl_find_regblock_instance() already does by passing in a sentinel value > (CXL_INSTANCES_COUNT) that triggers the count to be returned. > --- > > Otherwise, patch looks good. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> Many thanks for the guidance, I gain a lot of benefit from that. Best Regards, Huaisheng Ye
On 1/22/25 2:14 AM, Dan Williams wrote: > Some minor comments that Dave can fixup: > > Huaisheng Ye wrote: >> In commit d717d7f3df18494baafd9595fb4bcb9c380d7389, cxl_count_regblock was > > When referencing a commit the typical form is > > d717d7f3df18 ("cxl: Add functions to get an instance of / count regblocks of a given type") > > ...however in this case the commit history does not matter. Typically > commit history is useful for fixes to identify what kernels are > suspectible to the problem. In this case it is just a cleanup. > > Also, for what this patch is I think the commit message can afford to be > more concise and just say: > > --- > cxl_find_regblock_instance() counts the number of instances of a register > block as a side effect of searching through all available register blocks. > cxl_count_regblock() throws away that work and recounts all the register > blocks by asking cxl_find_regblock_instance() to redo work it has > already done until it finally returns an error, that is needlessly wasteful. > > Let cxl_count_regblock() leverage the counting that > cxl_find_regblock_instance() already does by passing in a sentinel value > (CXL_INSTANCES_COUNT) that triggers the count to be returned. > --- > > Otherwise, patch looks good. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> Applied with the suggested commit log replacement
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index 59cb35b40c7e..77e42cd68bba 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -289,20 +289,16 @@ static bool cxl_decode_regblock(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi, return true; } -/** - * cxl_find_regblock_instance() - Locate a register block by type / index - * @pdev: The CXL PCI device to enumerate. - * @type: Register Block Indicator id - * @map: Enumeration output, clobbered on error - * @index: Index into which particular instance of a regblock wanted in the - * order found in register locator DVSEC. - * - * Return: 0 if register block enumerated, negative error code otherwise +/* + * __cxl_find_regblock_instance() - Locate a register block or count instances by type / index + * Use CXL_INSTANCES_COUNT for @index if counting instances. * - * A CXL DVSEC may point to one or more register blocks, search for them - * by @type and @index. + * __cxl_find_regblock_instance() may return: + * 0 - if register block enumerated. + * >= 0 - if counting instances. + * < 0 - error code otherwise. */ -int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, +static int __cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, struct cxl_register_map *map, int index) { u32 regloc_size, regblocks; @@ -342,8 +338,30 @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, } map->resource = CXL_RESOURCE_NONE; + if (index == CXL_INSTANCES_COUNT) + return instance; + return -ENODEV; } + +/** + * cxl_find_regblock_instance() - Locate a register block by type / index + * @pdev: The CXL PCI device to enumerate. + * @type: Register Block Indicator id + * @map: Enumeration output, clobbered on error + * @index: Index into which particular instance of a regblock wanted in the + * order found in register locator DVSEC. + * + * Return: 0 if register block enumerated, negative error code otherwise + * + * A CXL DVSEC may point to one or more register blocks, search for them + * by @type and @index. + */ +int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, + struct cxl_register_map *map, unsigned int index) +{ + return __cxl_find_regblock_instance(pdev, type, map, index); +} EXPORT_SYMBOL_NS_GPL(cxl_find_regblock_instance, "CXL"); /** @@ -360,7 +378,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_find_regblock_instance, "CXL"); int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, struct cxl_register_map *map) { - return cxl_find_regblock_instance(pdev, type, map, 0); + return __cxl_find_regblock_instance(pdev, type, map, 0); } EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, "CXL"); @@ -371,19 +389,13 @@ EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, "CXL"); * * Some regblocks may be repeated. Count how many instances. * - * Return: count of matching regblocks. + * Return: non-negative count of matching regblocks, negative error code otherwise. */ int cxl_count_regblock(struct pci_dev *pdev, enum cxl_regloc_type type) { struct cxl_register_map map; - int rc, count = 0; - while (1) { - rc = cxl_find_regblock_instance(pdev, type, &map, count); - if (rc) - return count; - count++; - } + return __cxl_find_regblock_instance(pdev, type, &map, CXL_INSTANCES_COUNT); } EXPORT_SYMBOL_NS_GPL(cxl_count_regblock, "CXL"); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index f6015f24ad38..7fc456d5b917 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -302,10 +302,11 @@ int cxl_map_device_regs(const struct cxl_register_map *map, struct cxl_device_regs *regs); int cxl_map_pmu_regs(struct cxl_register_map *map, struct cxl_pmu_regs *regs); +#define CXL_INSTANCES_COUNT -1 enum cxl_regloc_type; int cxl_count_regblock(struct pci_dev *pdev, enum cxl_regloc_type type); int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, - struct cxl_register_map *map, int index); + struct cxl_register_map *map, unsigned int index); int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, struct cxl_register_map *map); int cxl_setup_regs(struct cxl_register_map *map); diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 6d94ff4a4f1a..a96e54c6259e 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -907,7 +907,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) struct cxl_dev_state *cxlds; struct cxl_register_map map; struct cxl_memdev *cxlmd; - int i, rc, pmu_count; + int rc, pmu_count; + unsigned int i; bool irq_avail; /* @@ -1009,6 +1010,9 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) return rc; pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU); + if (pmu_count < 0) + return pmu_count; + for (i = 0; i < pmu_count; i++) { struct cxl_pmu_regs pmu_regs;