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 v3] cxl/core/regs: Refactor out functions to count regblocks of given type
Date: Mon, 13 Jan 2025 16:31:41 +0000 [thread overview]
Message-ID: <20250113163141.00002a10@huawei.com> (raw)
In-Reply-To: <20250113064345.6835-1-huaisheng.ye@intel.com>
On Mon, 13 Jan 2025 14:43:45 +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 the implementation 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. The
> operations can be optimized.
>
> 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, starting from scratch,
> even if PMU0 just has been searched. 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 existing cxl_count_regblock
> couldn't return error codes.
>
> With this patch, only need to have minor modifications 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 and create
> __cxl_find_regblock_instance for counting instances more efficiently.
>
I'd suggest moving the testing setup to a cover letter. It is more detail
that we necessarily need in the git log. Maybe if this change is otherwise
acceptable, Dave might tidy this up whilst applying.
> This patch is tested by ndctl cxl_test and physical CXL expander card
> with v6.13-rc6.
>
> 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
>
Change log belongs below the ---
> Changes
> =======
> v2 -> v3:
>
> 1. Create static function __cxl_find_regblock_instance() for implementation of
> locating a register block or counting instances by type / index
> underneath. (Jonathan Cameron)
> 2. cxl_count_regblock() and cxl_find_regblock_instance() respectively call
> __cxl_find_regblock_instance for the purpose of counting instances and
> locating RB. (Jonathan Cameron)
> 3. Change parameter index's type to 'unsigned int' in cxl_find_regblock_instance (Jonathan Cameron)
> 4. Rebase patch to v6.13-rc6
>
> =======
> v1 -> v2:
>
> 1. Reserved cxl_count_regblock() for original function interface (Ming Li)
> 2. Reset 'map->resource' to 'CXL_RESOURCE_NONE' before returning the count of instances in
> cxl_find_regblock_instance() (Ming Li)
> 3. Append results of ndctl test suite and Qemu testing PMU devices to commit log (Ming Li)
> 4. Rebase patch to v6.13-rc5
>
> [v2] https://lore.kernel.org/all/20241230122239.3445117-1-huaisheng.ye@intel.com/
>
> Signed-off-by: Huaisheng Ye <huaisheng.ye@intel.com>
> ---
Put change logs here.
The code looks good to me. I did wonder if it made sense to keep so much documentation
for the static helper function, but on balance I think that is fine.
So with the patch description tidied up
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
prev parent reply other threads:[~2025-01-13 16:31 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-13 6:43 [PATCH v3] cxl/core/regs: Refactor out functions to count regblocks of given type Huaisheng Ye
2025-01-13 16:31 ` Jonathan Cameron [this message]
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=20250113163141.00002a10@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