| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250613114708.00000807@huawei.com>
Date: Fri, 13 Jun 2025 11:47:08 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Li Zhijian <lizhijian@...itsu.com>
CC: <linux-cxl@...r.kernel.org>, <dave@...olabs.net>, <dave.jiang@...el.com>,
<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
<ira.weiny@...el.com>, <dan.j.williams@...el.com>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH for-next] cxl/core: Consolidate auto region attachment
logic
On Tue, 3 Jun 2025 13:36:45 +0800
Li Zhijian <lizhijian@...itsu.com> wrote:
> Move all auto-attach handling from cxl_region_attach() into the
> cxl_region_attach_auto() function. This combines the partial handling
> previously in cxl_region_attach_auto() with the remaining logic that
> was directly implemented in cxl_region_attach().
>
> Specifically, cxl_region_attach_auto() now handles:
> - Adding new targets when in auto-discovery mode
> - Waiting for all required targets
> - Sorting and validating targets when ready
>
> This improves code organization by:
> - Keeping all auto-attach logic in a single function
> - Reducing complexity in the main attach function
> - Making the control flow easier to follow
>
> No functional change intended.
>
> Signed-off-by: Li Zhijian <lizhijian@...itsu.com>
Hi.
One minor comment inline. More generally I don't feel that strongly
either way on this one. Lets see if others think the improvements
are worth the churn.
Jonathan
> ---
> drivers/cxl/core/region.c | 164 +++++++++++++++++++-------------------
> 1 file changed, 82 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c3f4dc244df7..e7618d59b548 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1702,44 +1702,6 @@ static int cxl_region_attach_position(struct cxl_region *cxlr,
> return rc;
> }
>
> -static int cxl_region_attach_auto(struct cxl_region *cxlr,
> - struct cxl_endpoint_decoder *cxled, int pos)
> -{
> - struct cxl_region_params *p = &cxlr->params;
> -
> - if (cxled->state != CXL_DECODER_STATE_AUTO) {
> - dev_err(&cxlr->dev,
> - "%s: unable to add decoder to autodetected region\n",
> - dev_name(&cxled->cxld.dev));
> - return -EINVAL;
> - }
> -
> - if (pos >= 0) {
> - dev_dbg(&cxlr->dev, "%s: expected auto position, not %d\n",
> - dev_name(&cxled->cxld.dev), pos);
> - return -EINVAL;
> - }
> -
> - if (p->nr_targets >= p->interleave_ways) {
> - dev_err(&cxlr->dev, "%s: no more target slots available\n",
> - dev_name(&cxled->cxld.dev));
> - return -ENXIO;
> - }
> -
> - /*
> - * Temporarily record the endpoint decoder into the target array. Yes,
> - * this means that userspace can view devices in the wrong position
> - * before the region activates, and must be careful to understand when
> - * it might be racing region autodiscovery.
> - */
> - pos = p->nr_targets;
> - p->targets[pos] = cxled;
> - cxled->pos = pos;
> - p->nr_targets++;
> -
> - return 0;
> -}
> -
> static int cmp_interleave_pos(const void *a, const void *b)
> {
> struct cxl_endpoint_decoder *cxled_a = *(typeof(cxled_a) *)a;
> @@ -1905,6 +1867,86 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
> return rc;
> }
>
> +static int cxl_region_attach_auto(struct cxl_region *cxlr,
> + struct cxl_endpoint_decoder *cxled, int pos)
> +{
> + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> + struct cxl_region_params *p = &cxlr->params;
> + struct cxl_port *root_port;
> + int i, rc;
> +
> + if (cxled->state != CXL_DECODER_STATE_AUTO) {
> + dev_err(&cxlr->dev,
> + "%s: unable to add decoder to autodetected region\n",
> + dev_name(&cxled->cxld.dev));
> + return -EINVAL;
> + }
> +
> + if (pos >= 0) {
> + dev_dbg(&cxlr->dev, "%s: expected auto position, not %d\n",
> + dev_name(&cxled->cxld.dev), pos);
> + return -EINVAL;
> + }
> +
> + if (p->nr_targets >= p->interleave_ways) {
> + dev_err(&cxlr->dev, "%s: no more target slots available\n",
> + dev_name(&cxled->cxld.dev));
> + return -ENXIO;
> + }
> +
> + /*
> + * Temporarily record the endpoint decoder into the target array. Yes,
> + * this means that userspace can view devices in the wrong position
> + * before the region activates, and must be careful to understand when
> + * it might be racing region autodiscovery.
> + */
> + pos = p->nr_targets;
> + p->targets[pos] = cxled;
> + cxled->pos = pos;
> + p->nr_targets++;
> +
> + /* await more targets to arrive... */
> + if (p->nr_targets < p->interleave_ways)
> + return 0;
> +
> + /*
> + * All targets are here, which implies all PCI enumeration that
> + * affects this region has been completed. Walk the topology to
> + * sort the devices into their relative region decode position.
> + */
> + rc = cxl_region_sort_targets(cxlr);
> + if (rc)
> + return rc;
> +
> + root_port = cxlrd_to_port(cxlrd);
> + for (i = 0; i < p->nr_targets; i++) {
> + struct cxl_port *ep_port;
> + struct cxl_dport *dport;
> +
> + cxled = p->targets[i];
I don't much like this reuse of the cxled here given it's a totally different
one to the code above (which was the parameter passed in).
Maybe just use
ep_port = cxled_to_port(p->targets[i]);
...
rc = cxl_region_attach_popsition(cxlr, cxlrd, p->targets[i],
dport, i);
or call it target_cxled or similar.
> + ep_port = cxled_to_port(cxled);
> + dport = cxl_find_dport_by_dev(root_port,
> + ep_port->host_bridge);
> + rc = cxl_region_attach_position(cxlr, cxlrd, cxled,
> + dport, i);
> + if (rc)
> + return rc;
> + }
> +
> + rc = cxl_region_setup_targets(cxlr);
> + if (rc)
> + return rc;
> +
> + /*
> + * If target setup succeeds in the autodiscovery case
> + * then the region is already committed.
> + */
> + p->state = CXL_CONFIG_COMMIT;
> + cxl_region_shared_upstream_bandwidth_update(cxlr);
> +
> + return 0;
> +}
> +
> static int cxl_region_attach(struct cxl_region *cxlr,
> struct cxl_endpoint_decoder *cxled, int pos)
> {
> @@ -1986,50 +2028,8 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>
> cxl_region_perf_data_calculate(cxlr, cxled);
>
> - if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> - int i;
> -
> - rc = cxl_region_attach_auto(cxlr, cxled, pos);
> - if (rc)
> - return rc;
> -
> - /* await more targets to arrive... */
> - if (p->nr_targets < p->interleave_ways)
> - return 0;
> -
> - /*
> - * All targets are here, which implies all PCI enumeration that
> - * affects this region has been completed. Walk the topology to
> - * sort the devices into their relative region decode position.
> - */
> - rc = cxl_region_sort_targets(cxlr);
> - if (rc)
> - return rc;
> -
> - for (i = 0; i < p->nr_targets; i++) {
> - cxled = p->targets[i];
> - ep_port = cxled_to_port(cxled);
> - dport = cxl_find_dport_by_dev(root_port,
> - ep_port->host_bridge);
> - rc = cxl_region_attach_position(cxlr, cxlrd, cxled,
> - dport, i);
> - if (rc)
> - return rc;
> - }
> -
> - rc = cxl_region_setup_targets(cxlr);
> - if (rc)
> - return rc;
> -
> - /*
> - * If target setup succeeds in the autodiscovery case
> - * then the region is already committed.
> - */
> - p->state = CXL_CONFIG_COMMIT;
> - cxl_region_shared_upstream_bandwidth_update(cxlr);
> -
> - return 0;
> - }
> + if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
> + return cxl_region_attach_auto(cxlr, cxled, pos);
>
> rc = cxl_region_validate_position(cxlr, cxled, pos);
> if (rc)
Powered by blists - more mailing lists