From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: Huaisheng Ye <huaisheng.ye@intel.com>
Cc: <dan.j.williams@intel.com>, <dave.jiang@intel.com>,
<ming.li@zohomail.com>, <pei.p.jia@intel.com>,
<linux-cxl@vger.kernel.org>
Subject: Re: [PATCH v2] cxl/core/regs: Refactor out functions to count regblocks of given type
Date: Fri, 10 Jan 2025 15:52:55 +0000 [thread overview]
Message-ID: <20250110155255.00003a1e@huawei.com> (raw)
In-Reply-To: <20241230122239.3445117-1-huaisheng.ye@intel.com>
On Mon, 30 Dec 2024 20:22:39 +0800
Huaisheng Ye <huaisheng.ye@intel.com> 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, refactor out cxl_count_regblock for counting
> instances more efficiently.
>
> This patch is tested by ndctl cxl_test and physical CXL expander card
> with v6.13-rc5.
>
> 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.48s
> 2/11 ndctl:cxl / cxl-region-sysfs.sh OK 2.74s
> 3/11 ndctl:cxl / cxl-labels.sh OK 1.75s
> 4/11 ndctl:cxl / cxl-create-region.sh OK 3.51s
> 5/11 ndctl:cxl / cxl-xor-region.sh OK 1.89s
> 6/11 ndctl:cxl / cxl-events.sh OK 1.63s
> 7/11 ndctl:cxl / cxl-sanitize.sh OK 4.48s
> 8/11 ndctl:cxl / cxl-destroy-region.sh OK 1.90s
> 9/11 ndctl:cxl / cxl-qos-class.sh OK 2.65s
> 10/11 ndctl:cxl / cxl-poison.sh OK 2.86s
> 11/11 ndctl:cxl / cxl-security.sh OK 0.91s
>
> 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.
>
> $ pwd
> /sys/bus/cxl/devices
> $ tree
> [snip]
> ├── 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
> ├── 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
> ├── 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
> ├── 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
> ├── 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
> ├── 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
> ├── 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
> ├── 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
>
> Changes
> =======
> v1 -> v2:
>
> 1. Reserved cxl_count_regblock() for original function interface
> 2. Reset 'map->resource' to 'CXL_RESOURCE_NONE' before returning the count of instances in
> cxl_find_regblock_instance()
> 3. Append results of ndctl test suite and Qemu testing PMU devices to commit log
> 4. Rebase patch to v6.13-rc5
>
> [v1] https://lore.kernel.org/all/20241225083539.2230150-1-huaisheng.ye@intel.com/
>
> Signed-off-by: Huaisheng Ye <huaisheng.ye@intel.com>
> ---
> drivers/cxl/core/regs.c | 19 +++++++++----------
> drivers/cxl/cxl.h | 1 +
> drivers/cxl/pci.c | 3 +++
> 3 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 59cb35b40c7e..d0db954da0ff 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,
> }
>
> /**
The idea seems good.
I'm not keen on this function gaining super powers if we can avoid it.
Instead you could rename the implementation to __cxl_find_regblock_instance()
which does what you have here (but static) and have a cxl_find_regblock_instance()
that takes an unsigned index parameter and is documented as this was previously.
If someone wants the count they should use cxl_count_regblock().
If they want an entry they should call cxl_find_regblock_instance()
Both call the same implementation underneath.
Jonathan
> - * 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 instances counted,
> + * 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 and @index.
> */
> int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type,
> struct cxl_register_map *map, int index)
> @@ -342,6 +344,9 @@ 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;
> }
> EXPORT_SYMBOL_NS_GPL(cxl_find_regblock_instance, "CXL");
> @@ -371,19 +376,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..18bd05f29354 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -302,6 +302,7 @@ 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,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 6d94ff4a4f1a..c68c4a0bdbe0 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1009,6 +1009,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;
>
next prev parent reply other threads:[~2025-01-10 15:52 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-30 12:22 [PATCH v2] cxl/core/regs: Refactor out functions to count regblocks of given type Huaisheng Ye
2025-01-10 15:52 ` Jonathan Cameron [this message]
2025-01-11 3:09 ` Ye, Huaisheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250110155255.00003a1e@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=huaisheng.ye@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=ming.li@zohomail.com \
--cc=pei.p.jia@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox