| Message ID | 20241225083539.2230150-1-huaisheng.ye@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] cxl/core/regs: Refactor out functions to count regblocks of given type | expand |
On 12/25/2024 4:35 PM, Huaisheng Ye wrote: > In commit d717d7f3df18494baafd9595fb4bcb9c380d7389, cxl_count_regblock was > added for counting regblocks of a given RBI (Register Block Identifier). > It is workable, but has two drawbacks that can be improved. > > 1. In order to get the count of instances, cxl_count_regblock has to tentatively > repeat the call of cxl_find_regblock_instance by increasing index from 0. It > will not stop until an error value is returned. Actually, It needs to search for > Register Blocks in dvsec again every time by taking a start from the head. This > is a bit inefficient. > > For example, to determine if PMU1 exists, cxl_find_regblock_instance must check > all Register Blocks by the type and index from RB1 to RB4. If there are more RBs > of the same type in the future, the situation will be even worse. > > 16 81 00 23 PCIe extended Capability Header > 02 c0 1e 98 Header 1 > 00 00 00 08 Header 2 > -------------------------------------- > 00 00 01 00 RB 1 - Offset Low Component > 00 00 00 00 RB 1 - Offset High > -------------------------------------- > 00 00 03 02 RB 2 - Offset Low Device Register > 00 00 00 00 RB 2 - Offset High > -------------------------------------- > 00 01 04 02 RB 3 - Offset Low PMU0 > 00 00 00 00 RB 3 - Offset High > -------------------------------------- > 00 02 04 02 RB 4 - Offset Low PMU1 > 00 00 00 00 RB 4 - Offset High > > RB: Register Block > > 2. cxl_count_regblock blocks the opportunity to get error codes from > cxl_find_regblock_instance. cxl_pci_probe has error code checking for almost > all function calls. This is a good behavior, but cxl_count_regblock couldn't. > > With this patch, only need to add two lines of code in cxl_find_regblock_instance, > which can return the count of regblocks by given RBI in just one call. It is > more effective than before. Besides, the error code could be obtained by the > called function, here is cxl_pci_probe. > > Based on the above reasons, cxl_count_regblock could be considered to remove. > > This patch is tested by ndctl cxl_test and physical CXL expander card > with v6.12. I think the description about how you tested the patch should be included in cover-letter rather than patch's changelog. > > Signed-off-by: Huaisheng Ye <huaisheng.ye@intel.com> > --- > drivers/cxl/core/regs.c | 32 +++++++------------------------- > drivers/cxl/cxl.h | 2 +- > drivers/cxl/pci.c | 7 ++++++- > 3 files changed, 14 insertions(+), 27 deletions(-) > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index e1082e749c69..d6b882a334d5 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -290,17 +290,19 @@ static bool cxl_decode_regblock(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi, > } > > /** > - * cxl_find_regblock_instance() - Locate a register block by type / index > + * cxl_find_regblock_instance() - Locate a register block or count instances 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 > + * Return: 0 if register block enumerated, non-negative if counting instances, > + * negative error code otherwise > * > * A CXL DVSEC may point to one or more register blocks, search for them > * by @type and @index. > + * Use CXL_INSTANCES_COUNT for @index if counting instances by @type. > */ > int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, > struct cxl_register_map *map, int index) > @@ -341,6 +343,9 @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, > } > } > > + if (index == CXL_INSTANCES_COUNT) > + return instance; > + Above return will skip below 'map->resource = CXL_RESOURCE_NONE'. is it expected? > map->resource = CXL_RESOURCE_NONE; > return -ENODEV; > } > @@ -364,29 +369,6 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, > } > EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL); > > -/** > - * cxl_count_regblock() - Count instances of a given regblock type. > - * @pdev: The CXL PCI device to enumerate. > - * @type: Register Block Indicator id > - * > - * Some regblocks may be repeated. Count how many instances. > - * > - * Return: count of matching regblocks. > - */ > -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++; > - } > -} > -EXPORT_SYMBOL_NS_GPL(cxl_count_regblock, CXL); > - > int cxl_map_pmu_regs(struct cxl_register_map *map, struct cxl_pmu_regs *regs) > { > struct device *dev = map->host; > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 5406e3ab3d4a..3fbbd76da819 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -294,8 +294,8 @@ 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); > int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 188412d45e0d..5aa4dfda4a73 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -915,7 +915,12 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > - pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU); > + rc = cxl_find_regblock_instance(pdev, CXL_REGLOC_RBI_PMU, &map, CXL_INSTANCES_COUNT); I think that I can clearly understand what cxl_count_regblock() does from the name of it, not sure if repalcing cxl_count_regblock() with cxl_find_regblock_instance() is a good choice. Let us see if other reviwers have more comments on it. > + if (rc < 0) > + return rc; Before this patch, any error happened in cxl_count_regblock() will cause cxl_count_regblock() returning current PMU instance counter, it will not stop cxl pci probing. But this patch will change this code logic, any error happened during counting PMU instance counter will stop cxl pci probing, is it expected? My understanding is that cxl_pci driver allows PMU initialization failure(in below loops), so I guess that counting PMU instances failure is allowed, just initializing the number of PMU instances returned by cxl_count_regblock() is OK. > + > + pmu_count = rc; > + > for (i = 0; i < pmu_count; i++) { > struct cxl_pmu_regs pmu_regs; >
On 12/25/2024 4:35 PM, Huaisheng Ye wrote: > In commit d717d7f3df18494baafd9595fb4bcb9c380d7389, cxl_count_regblock was > added for counting regblocks of a given RBI (Register Block Identifier). > It is workable, but has two drawbacks that can be improved. > > 1. In order to get the count of instances, cxl_count_regblock has to tentatively > repeat the call of cxl_find_regblock_instance by increasing index from 0. It > will not stop until an error value is returned. Actually, It needs to search for > Register Blocks in dvsec again every time by taking a start from the head. This > is a bit inefficient. > > For example, to determine if PMU1 exists, cxl_find_regblock_instance must check > all Register Blocks by the type and index from RB1 to RB4. If there are more RBs > of the same type in the future, the situation will be even worse. > > 16 81 00 23 PCIe extended Capability Header > 02 c0 1e 98 Header 1 > 00 00 00 08 Header 2 > -------------------------------------- > 00 00 01 00 RB 1 - Offset Low Component > 00 00 00 00 RB 1 - Offset High > -------------------------------------- > 00 00 03 02 RB 2 - Offset Low Device Register > 00 00 00 00 RB 2 - Offset High > -------------------------------------- > 00 01 04 02 RB 3 - Offset Low PMU0 > 00 00 00 00 RB 3 - Offset High > -------------------------------------- > 00 02 04 02 RB 4 - Offset Low PMU1 > 00 00 00 00 RB 4 - Offset High > > RB: Register Block > > 2. cxl_count_regblock blocks the opportunity to get error codes from > cxl_find_regblock_instance. cxl_pci_probe has error code checking for almost > all function calls. This is a good behavior, but cxl_count_regblock couldn't. > > With this patch, only need to add two lines of code in cxl_find_regblock_instance, > which can return the count of regblocks by given RBI in just one call. It is > more effective than before. Besides, the error code could be obtained by the > called function, here is cxl_pci_probe. > > Based on the above reasons, cxl_count_regblock could be considered to remove. > > This patch is tested by ndctl cxl_test and physical CXL expander card > with v6.12. I think the description about how you tested the patch should be included in cover-letter rather than patch's changelog. > > Signed-off-by: Huaisheng Ye <huaisheng.ye@intel.com> > --- > drivers/cxl/core/regs.c | 32 +++++++------------------------- > drivers/cxl/cxl.h | 2 +- > drivers/cxl/pci.c | 7 ++++++- > 3 files changed, 14 insertions(+), 27 deletions(-) > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index e1082e749c69..d6b882a334d5 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -290,17 +290,19 @@ static bool cxl_decode_regblock(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi, > } > > /** > - * cxl_find_regblock_instance() - Locate a register block by type / index > + * cxl_find_regblock_instance() - Locate a register block or count instances 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 > + * Return: 0 if register block enumerated, non-negative if counting instances, > + * negative error code otherwise > * > * A CXL DVSEC may point to one or more register blocks, search for them > * by @type and @index. > + * Use CXL_INSTANCES_COUNT for @index if counting instances by @type. > */ > int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, > struct cxl_register_map *map, int index) > @@ -341,6 +343,9 @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, > } > } > > + if (index == CXL_INSTANCES_COUNT) > + return instance; > + Above return should not skip below 'map->resource = CXL_RESOURCE_NONE'. > map->resource = CXL_RESOURCE_NONE; > return -ENODEV; > } > @@ -364,29 +369,6 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, > } > EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL); > > -/** > - * cxl_count_regblock() - Count instances of a given regblock type. > - * @pdev: The CXL PCI device to enumerate. > - * @type: Register Block Indicator id > - * > - * Some regblocks may be repeated. Count how many instances. > - * > - * Return: count of matching regblocks. > - */ > -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++; > - } > -} > -EXPORT_SYMBOL_NS_GPL(cxl_count_regblock, CXL); > - > int cxl_map_pmu_regs(struct cxl_register_map *map, struct cxl_pmu_regs *regs) > { > struct device *dev = map->host; > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 5406e3ab3d4a..3fbbd76da819 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -294,8 +294,8 @@ 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); > int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 188412d45e0d..5aa4dfda4a73 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -915,7 +915,12 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > - pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU); > + rc = cxl_find_regblock_instance(pdev, CXL_REGLOC_RBI_PMU, &map, CXL_INSTANCES_COUNT); I think that I can clearly understand what cxl_count_regblock() does from the name of it, not sure if repalcing cxl_count_regblock() with cxl_find_regblock_instance() is a good choice. Let us see if other reviwers have more comments on it. > + if (rc < 0) > + return rc; Before this patch, any error happened in cxl_count_regblock() will cause cxl_count_regblock() returning current PMU instance counter, it will not stop cxl pci probing. But this patch will change this code logic, any error happened during counting PMU instance counter will stop cxl pci probing, is it expected? My understanding is that cxl_pci driver allows PMU initialization failure(in below loops), so I guess that counting PMU instances failure is allowed, just initializing the number of PMU instances returned by cxl_count_regblock() is OK. > + > + pmu_count = rc; > + > for (i = 0; i < pmu_count; i++) { > struct cxl_pmu_regs pmu_regs; >
From: Li Ming <ming.li@zohomail.com> Sent: Thursday, December 26, 2024 10:11 AM > > On 12/25/2024 4:35 PM, Huaisheng Ye wrote: > > In commit d717d7f3df18494baafd9595fb4bcb9c380d7389, cxl_count_regblock > > was added for counting regblocks of a given RBI (Register Block > Identifier). > > It is workable, but has two drawbacks that can be improved. > > > > 1. In order to get the count of instances, cxl_count_regblock has to > > tentatively repeat the call of cxl_find_regblock_instance by > > increasing index from 0. It will not stop until an error value is > > returned. Actually, It needs to search for Register Blocks in dvsec > > again every time by taking a start from the head. This is a bit > inefficient. > > > > For example, to determine if PMU1 exists, cxl_find_regblock_instance > > must check all Register Blocks by the type and index from RB1 to RB4. > > If there are more RBs of the same type in the future, the situation will > be even worse. > > > > 16 81 00 23 PCIe extended Capability Header > > 02 c0 1e 98 Header 1 > > 00 00 00 08 Header 2 > > -------------------------------------- > > 00 00 01 00 RB 1 - Offset Low Component > > 00 00 00 00 RB 1 - Offset High > > -------------------------------------- > > 00 00 03 02 RB 2 - Offset Low Device Register > > 00 00 00 00 RB 2 - Offset High > > -------------------------------------- > > 00 01 04 02 RB 3 - Offset Low PMU0 > > 00 00 00 00 RB 3 - Offset High > > -------------------------------------- > > 00 02 04 02 RB 4 - Offset Low PMU1 > > 00 00 00 00 RB 4 - Offset High > > > > RB: Register Block > > > > 2. cxl_count_regblock blocks the opportunity to get error codes from > > cxl_find_regblock_instance. cxl_pci_probe has error code checking for > > almost all function calls. This is a good behavior, but > cxl_count_regblock couldn't. > > > > With this patch, only need to add two lines of code in > > cxl_find_regblock_instance, which can return the count of regblocks by > > given RBI in just one call. It is more effective than before. Besides, > > the error code could be obtained by the called function, here is > cxl_pci_probe. > > > > Based on the above reasons, cxl_count_regblock could be considered to > remove. > > > > This patch is tested by ndctl cxl_test and physical CXL expander card > > with v6.12. > > I think the description about how you tested the patch should be included > in cover-letter rather than patch's changelog. > Hi Ming, Many thanks for your comments. This is a single patch, so I haven't prepared cover-letter, but your suggestion is correct I should provide a more detailed introduction for how to test. I will expand them in next version. 1. Ndctl CXL test suite v80 could pass with this patch applied. $ meson test -C build --suite cxl ninja: Entering directory `/home/work/source/ndctl/build' [1/48] Generating version.h with a custom command 1/11 ndctl:cxl / cxl-topology.sh OK 3.29s 2/11 ndctl:cxl / cxl-region-sysfs.sh OK 2.50s 3/11 ndctl:cxl / cxl-labels.sh OK 1.64s 4/11 ndctl:cxl / cxl-create-region.sh OK 3.29s 5/11 ndctl:cxl / cxl-xor-region.sh OK 1.72s 6/11 ndctl:cxl / cxl-events.sh OK 1.45s 7/11 ndctl:cxl / cxl-sanitize.sh OK 4.47s 8/11 ndctl:cxl / cxl-destroy-region.sh OK 1.84s 9/11 ndctl:cxl / cxl-qos-class.sh OK 2.52s 10/11 ndctl:cxl / cxl-poison.sh OK 2.81s 11/11 ndctl:cxl / cxl-security.sh OK 0.79s Ok: 11 Expected Fail: 0 Fail: 0 Unexpected Pass: 0 Skipped: 0 Timeout: 0 Full log written to /home/work/source/ndctl/build/meson-logs/testlog.txt 2. Test patch with Qemu x4 switch topology: ACPI0017:00 [root0] | HB_0 [port1] / \ RP_0 RP_1 | | USP [port2] / / \ \ DSP DSP DSP DSP | | | | mem1 mem0 mem2 mem3 Every card has 2 PMU RBs, here are the pmu_mem devices. $ ls /sys/bus/cxl/devices/pmu_mem* -lh lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem0.0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:01.0/0000:10:00.0/pmu_mem0.0 lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem0.1 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:01.0/0000:10:00.0/pmu_mem0.1 lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem1.0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:00.0/0000:0f:00.0/pmu_mem1.0 lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem1.1 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:00.0/0000:0f:00.0/pmu_mem1.1 lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem2.0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:02.0/0000:11:00.0/pmu_mem2.0 lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem2.1 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:02.0/0000:11:00.0/pmu_mem2.1 lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem3.0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:03.0/0000:12:00.0/pmu_mem3.0 lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem3.1 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:03.0/0000:12:00.0/pmu_mem3.1 > > > > Signed-off-by: Huaisheng Ye <huaisheng.ye@intel.com> > > --- > > drivers/cxl/core/regs.c | 32 +++++++------------------------- > > drivers/cxl/cxl.h | 2 +- > > drivers/cxl/pci.c | 7 ++++++- > > 3 files changed, 14 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index > > e1082e749c69..d6b882a334d5 100644 > > --- a/drivers/cxl/core/regs.c > > +++ b/drivers/cxl/core/regs.c > > @@ -290,17 +290,19 @@ static bool cxl_decode_regblock(struct pci_dev > > *pdev, u32 reg_lo, u32 reg_hi, } > > > > /** > > - * cxl_find_regblock_instance() - Locate a register block by type / > > index > > + * cxl_find_regblock_instance() - Locate a register block or count > > + instances 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 > > + * Return: 0 if register block enumerated, non-negative if counting > > + instances, > > + * negative error code otherwise > > * > > * A CXL DVSEC may point to one or more register blocks, search for > them > > * by @type and @index. > > + * Use CXL_INSTANCES_COUNT for @index if counting instances by @type. > > */ > > int cxl_find_regblock_instance(struct pci_dev *pdev, enum > cxl_regloc_type type, > > struct cxl_register_map *map, int index) @@ - > 341,6 +343,9 > > @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum > cxl_regloc_type type, > > } > > } > > > > + if (index == CXL_INSTANCES_COUNT) > > + return instance; > > + > > Above return will skip below 'map->resource = CXL_RESOURCE_NONE'. is it > expected? > Yes, when counting regblocks we don't need to return register_map, we only want to know the number of the given RBI. So, skipping 'map->resource = CXL_RESOURCE_NONE' and 'return -ENODEV' is okay. <snip> > > - pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU); > > + rc = cxl_find_regblock_instance(pdev, CXL_REGLOC_RBI_PMU, &map, > > +CXL_INSTANCES_COUNT); > > I think that I can clearly understand what cxl_count_regblock() does from > the name of it, not sure if repalcing cxl_count_regblock() with > cxl_find_regblock_instance() is a good choice. > > Let us see if other reviwers have more comments on it. If you feel that cxl_count_regblock() is more helpful for understanding, it is also possible to keep it. Just call cxl_find_regblock_instance once with CXL_INSTANCES_COUNT could return the count. > > > + if (rc < 0) > > + return rc; > > Before this patch, any error happened in cxl_count_regblock() will cause > cxl_count_regblock() returning current PMU instance counter, it will not > stop cxl pci probing. > > But this patch will change this code logic, any error happened during > counting PMU instance counter will stop cxl pci probing, is it expected? > > My understanding is that cxl_pci driver allows PMU initialization > failure(in below loops), so I guess that counting PMU instances failure is > allowed, just initializing the number of PMU instances returned by > cxl_count_regblock() is OK. I think this patch will improve the original logic. Just like you introduced, there are two types of error could happen in cxl_count_regblock() when counting PMU or other instances. The first one is that regloc could fail to be found, then cxl_find_regblock_instance should return '-ENXIO' to notify the caller. But current cxl_count_regblock() blocks that. I think this is a more serious mistake than no PMU instances. The other one is that no PMU instance or no corresponding index could be found. With this situation, the patch will return 0, that is no difference with original logic. Only when the return value is negative, that means real error has occurred. cxl pci probing will be stopped under such circumstances. I am not opposed to the commit d717d7f3df18, I think it could be improved from the original behavior, that is tentatively repeating the call of cxl_find_regblock_instance by increasing index from 0 to the highest count. With this patch, just one call could get the count of instances. Best Regards, Huaisheng Ye
On 12/26/2024 2:20 PM, Ye, Huaisheng wrote: > From: Li Ming <ming.li@zohomail.com> > Sent: Thursday, December 26, 2024 10:11 AM >> On 12/25/2024 4:35 PM, Huaisheng Ye wrote: >>> In commit d717d7f3df18494baafd9595fb4bcb9c380d7389, cxl_count_regblock >>> was added for counting regblocks of a given RBI (Register Block >> Identifier). >>> It is workable, but has two drawbacks that can be improved. >>> >>> 1. In order to get the count of instances, cxl_count_regblock has to >>> tentatively repeat the call of cxl_find_regblock_instance by >>> increasing index from 0. It will not stop until an error value is >>> returned. Actually, It needs to search for Register Blocks in dvsec >>> again every time by taking a start from the head. This is a bit >> inefficient. >>> For example, to determine if PMU1 exists, cxl_find_regblock_instance >>> must check all Register Blocks by the type and index from RB1 to RB4. >>> If there are more RBs of the same type in the future, the situation will >> be even worse. >>> 16 81 00 23 PCIe extended Capability Header >>> 02 c0 1e 98 Header 1 >>> 00 00 00 08 Header 2 >>> -------------------------------------- >>> 00 00 01 00 RB 1 - Offset Low Component >>> 00 00 00 00 RB 1 - Offset High >>> -------------------------------------- >>> 00 00 03 02 RB 2 - Offset Low Device Register >>> 00 00 00 00 RB 2 - Offset High >>> -------------------------------------- >>> 00 01 04 02 RB 3 - Offset Low PMU0 >>> 00 00 00 00 RB 3 - Offset High >>> -------------------------------------- >>> 00 02 04 02 RB 4 - Offset Low PMU1 >>> 00 00 00 00 RB 4 - Offset High >>> >>> RB: Register Block >>> >>> 2. cxl_count_regblock blocks the opportunity to get error codes from >>> cxl_find_regblock_instance. cxl_pci_probe has error code checking for >>> almost all function calls. This is a good behavior, but >> cxl_count_regblock couldn't. >>> With this patch, only need to add two lines of code in >>> cxl_find_regblock_instance, which can return the count of regblocks by >>> given RBI in just one call. It is more effective than before. Besides, >>> the error code could be obtained by the called function, here is >> cxl_pci_probe. >>> Based on the above reasons, cxl_count_regblock could be considered to >> remove. >>> This patch is tested by ndctl cxl_test and physical CXL expander card >>> with v6.12. >> I think the description about how you tested the patch should be included >> in cover-letter rather than patch's changelog. >> > Hi Ming, > > Many thanks for your comments. > > This is a single patch, so I haven't prepared cover-letter, but your suggestion > is correct I should provide a more detailed introduction for how to test. I will > expand them in next version. > > 1. Ndctl CXL test suite v80 could pass with this patch applied. > > $ meson test -C build --suite cxl > ninja: Entering directory `/home/work/source/ndctl/build' > [1/48] Generating version.h with a custom command > 1/11 ndctl:cxl / cxl-topology.sh OK 3.29s > 2/11 ndctl:cxl / cxl-region-sysfs.sh OK 2.50s > 3/11 ndctl:cxl / cxl-labels.sh OK 1.64s > 4/11 ndctl:cxl / cxl-create-region.sh OK 3.29s > 5/11 ndctl:cxl / cxl-xor-region.sh OK 1.72s > 6/11 ndctl:cxl / cxl-events.sh OK 1.45s > 7/11 ndctl:cxl / cxl-sanitize.sh OK 4.47s > 8/11 ndctl:cxl / cxl-destroy-region.sh OK 1.84s > 9/11 ndctl:cxl / cxl-qos-class.sh OK 2.52s > 10/11 ndctl:cxl / cxl-poison.sh OK 2.81s > 11/11 ndctl:cxl / cxl-security.sh OK 0.79s > > Ok: 11 > Expected Fail: 0 > Fail: 0 > Unexpected Pass: 0 > Skipped: 0 > Timeout: 0 > > Full log written to /home/work/source/ndctl/build/meson-logs/testlog.txt > > > 2. Test patch with Qemu x4 switch topology: > > ACPI0017:00 [root0] > | > HB_0 [port1] > / \ > RP_0 RP_1 > | | > USP [port2] > / / \ \ > DSP DSP DSP DSP > | | | | > mem1 mem0 mem2 mem3 > > Every card has 2 PMU RBs, here are the pmu_mem devices. > > $ ls /sys/bus/cxl/devices/pmu_mem* -lh > lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem0.0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:01.0/0000:10:00.0/pmu_mem0.0 > lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem0.1 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:01.0/0000:10:00.0/pmu_mem0.1 > lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem1.0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:00.0/0000:0f:00.0/pmu_mem1.0 > lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem1.1 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:00.0/0000:0f:00.0/pmu_mem1.1 > lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem2.0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:02.0/0000:11:00.0/pmu_mem2.0 > lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem2.1 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:02.0/0000:11:00.0/pmu_mem2.1 > lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem3.0 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:03.0/0000:12:00.0/pmu_mem3.0 > lrwxrwxrwx. 1 root root 0 Dec 26 12:59 /sys/bus/cxl/devices/pmu_mem3.1 -> ../../../devices/pci0000:0c/0000:0c:00.0/0000:0d:00.0/0000:0e:03.0/0000:12:00.0/pmu_mem3.1 > >>> Signed-off-by: Huaisheng Ye <huaisheng.ye@intel.com> >>> --- >>> drivers/cxl/core/regs.c | 32 +++++++------------------------- >>> drivers/cxl/cxl.h | 2 +- >>> drivers/cxl/pci.c | 7 ++++++- >>> 3 files changed, 14 insertions(+), 27 deletions(-) >>> >>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index >>> e1082e749c69..d6b882a334d5 100644 >>> --- a/drivers/cxl/core/regs.c >>> +++ b/drivers/cxl/core/regs.c >>> @@ -290,17 +290,19 @@ static bool cxl_decode_regblock(struct pci_dev >>> *pdev, u32 reg_lo, u32 reg_hi, } >>> >>> /** >>> - * cxl_find_regblock_instance() - Locate a register block by type / >>> index >>> + * cxl_find_regblock_instance() - Locate a register block or count >>> + instances 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 >>> + * Return: 0 if register block enumerated, non-negative if counting >>> + instances, >>> + * negative error code otherwise >>> * >>> * A CXL DVSEC may point to one or more register blocks, search for >> them >>> * by @type and @index. >>> + * Use CXL_INSTANCES_COUNT for @index if counting instances by @type. >>> */ >>> int cxl_find_regblock_instance(struct pci_dev *pdev, enum >> cxl_regloc_type type, >>> struct cxl_register_map *map, int index) @@ - >> 341,6 +343,9 >>> @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum >> cxl_regloc_type type, >>> } >>> } >>> >>> + if (index == CXL_INSTANCES_COUNT) >>> + return instance; >>> + >> Above return will skip below 'map->resource = CXL_RESOURCE_NONE'. is it >> expected? >> > Yes, when counting regblocks we don't need to return register_map, we only want to know the number of the given RBI. > So, skipping 'map->resource = CXL_RESOURCE_NONE' and 'return -ENODEV' is okay. My understanding is that you should reset 'map->resource' to 'CXL_RESOURCE_NONE' in the case of no available register_map returned. Because the register_map has been used in cxl_decoder_regblock(), reset 'map->resource' to avoid some potential issues on caller side. [snip]
From: Li Ming <ming.li@zohomail.com> Sent: Sunday, December 29, 2024 1:42 PM [snip] > >>> + > >> Above return will skip below 'map->resource = CXL_RESOURCE_NONE'. is > >> it expected? > >> > > Yes, when counting regblocks we don't need to return register_map, we > only want to know the number of the given RBI. > > So, skipping 'map->resource = CXL_RESOURCE_NONE' and 'return -ENODEV' is > okay. > > My understanding is that you should reset 'map->resource' to > 'CXL_RESOURCE_NONE' in the case of no available register_map returned. > Because the register_map has been used in cxl_decoder_regblock(), reset > 'map->resource' to avoid some potential issues on caller side. > > [snip] > Got it, I will reset it in next version. Thanks. Best Regards, Huaisheng Ye
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index e1082e749c69..d6b882a334d5 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -290,17 +290,19 @@ static bool cxl_decode_regblock(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi, } /** - * cxl_find_regblock_instance() - Locate a register block by type / index + * cxl_find_regblock_instance() - Locate a register block or count instances 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 + * Return: 0 if register block enumerated, non-negative if counting instances, + * negative error code otherwise * * A CXL DVSEC may point to one or more register blocks, search for them * by @type and @index. + * Use CXL_INSTANCES_COUNT for @index if counting instances by @type. */ int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, struct cxl_register_map *map, int index) @@ -341,6 +343,9 @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, } } + if (index == CXL_INSTANCES_COUNT) + return instance; + map->resource = CXL_RESOURCE_NONE; return -ENODEV; } @@ -364,29 +369,6 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, } EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL); -/** - * cxl_count_regblock() - Count instances of a given regblock type. - * @pdev: The CXL PCI device to enumerate. - * @type: Register Block Indicator id - * - * Some regblocks may be repeated. Count how many instances. - * - * Return: count of matching regblocks. - */ -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++; - } -} -EXPORT_SYMBOL_NS_GPL(cxl_count_regblock, CXL); - int cxl_map_pmu_regs(struct cxl_register_map *map, struct cxl_pmu_regs *regs) { struct device *dev = map->host; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 5406e3ab3d4a..3fbbd76da819 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -294,8 +294,8 @@ 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); int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 188412d45e0d..5aa4dfda4a73 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -915,7 +915,12 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) return rc; - pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU); + rc = cxl_find_regblock_instance(pdev, CXL_REGLOC_RBI_PMU, &map, CXL_INSTANCES_COUNT); + if (rc < 0) + return rc; + + pmu_count = rc; + for (i = 0; i < pmu_count; i++) { struct cxl_pmu_regs pmu_regs;
In commit d717d7f3df18494baafd9595fb4bcb9c380d7389, cxl_count_regblock was added for counting regblocks of a given RBI (Register Block Identifier). It is workable, but has two drawbacks that can be improved. 1. In order to get the count of instances, cxl_count_regblock has to tentatively repeat the call of cxl_find_regblock_instance by increasing index from 0. It will not stop until an error value is returned. Actually, It needs to search for Register Blocks in dvsec again every time by taking a start from the head. This is a bit inefficient. For example, to determine if PMU1 exists, cxl_find_regblock_instance must check all Register Blocks by the type and index from RB1 to RB4. If there are more RBs of the same type in the future, the situation will be even worse. 16 81 00 23 PCIe extended Capability Header 02 c0 1e 98 Header 1 00 00 00 08 Header 2 -------------------------------------- 00 00 01 00 RB 1 - Offset Low Component 00 00 00 00 RB 1 - Offset High -------------------------------------- 00 00 03 02 RB 2 - Offset Low Device Register 00 00 00 00 RB 2 - Offset High -------------------------------------- 00 01 04 02 RB 3 - Offset Low PMU0 00 00 00 00 RB 3 - Offset High -------------------------------------- 00 02 04 02 RB 4 - Offset Low PMU1 00 00 00 00 RB 4 - Offset High RB: Register Block 2. cxl_count_regblock blocks the opportunity to get error codes from cxl_find_regblock_instance. cxl_pci_probe has error code checking for almost all function calls. This is a good behavior, but cxl_count_regblock couldn't. With this patch, only need to add two lines of code in cxl_find_regblock_instance, which can return the count of regblocks by given RBI in just one call. It is more effective than before. Besides, the error code could be obtained by the called function, here is cxl_pci_probe. Based on the above reasons, cxl_count_regblock could be considered to remove. This patch is tested by ndctl cxl_test and physical CXL expander card with v6.12. Signed-off-by: Huaisheng Ye <huaisheng.ye@intel.com> --- drivers/cxl/core/regs.c | 32 +++++++------------------------- drivers/cxl/cxl.h | 2 +- drivers/cxl/pci.c | 7 ++++++- 3 files changed, 14 insertions(+), 27 deletions(-)