Re: [PATCH v3 08/18] cxl/region: Move find_cxl_root() to cxl_add_to_region()

Dave Jiang
2025-02-20T16:39:03Z

On 2/11/25 2:53 AM, Robert Richter wrote:
> When adding an endpoint to a region, the root port is determined
> first. Move this directly into cxl_add_to_region(). This is in
> preparation of the initialization of endpoints that iterates the port
> hierarchy from the endpoint up to the root port.
>
> As a side-effect the root argument is removed from the argument lists
> of cxl_add_to_region() and related functions. Now, the endpoint is the
> only parameter to add a region. This simplifies the function
> interface.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/region.c | 6 ++++--
> drivers/cxl/cxl.h | 6 ++----
> drivers/cxl/port.c | 15 +++------------
> 3 files changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b8201c2faa87..0e38bcb43be6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3312,9 +3312,11 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> return ERR_PTR(rc);
> }
>
> -int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
> +int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
> {
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> + struct cxl_port *port = cxled_to_port(cxled);
> + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> struct range *hpa = &cxled->cxld.hpa_range;
> struct cxl_decoder *cxld = &cxled->cxld;
> struct device *cxlrd_dev, *region_dev;
> @@ -3324,7 +3326,7 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
> bool attach = false;
> int rc;
>
> - cxlrd_dev = device_find_child(&root->dev, &cxld->hpa_range,
> + cxlrd_dev = device_find_child(&cxl_root->port.dev, &cxld->hpa_range,
> match_root_decoder_by_range);
> if (!cxlrd_dev) {
> dev_err(cxlmd->dev.parent,
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0d7aff8b97b3..85dfc8df0a80 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -854,8 +854,7 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_port *port);
> #ifdef CONFIG_CXL_REGION
> bool is_cxl_pmem_region(struct device *dev);
> struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev);
> -int cxl_add_to_region(struct cxl_port *root,
> - struct cxl_endpoint_decoder *cxled);
> +int cxl_add_to_region(struct cxl_endpoint_decoder *cxled);
> struct cxl_dax_region *to_cxl_dax_region(struct device *dev);
> #else
> static inline bool is_cxl_pmem_region(struct device *dev)
> @@ -866,8 +865,7 @@ static inline struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev)
> {
> return NULL;
> }
> -static inline int cxl_add_to_region(struct cxl_port *root,
> - struct cxl_endpoint_decoder *cxled)
> +static inline int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
> {
> return 0;
> }
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index d2bfd1ff5492..74587a403e3d 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -30,7 +30,7 @@ static void schedule_detach(void *cxlmd)
> schedule_cxl_memdev_detach(cxlmd);
> }
>
> -static int discover_region(struct device *dev, void *root)
> +static int discover_region(struct device *dev, void *unused)
> {
> struct cxl_endpoint_decoder *cxled;
> int rc;
> @@ -49,7 +49,7 @@ static int discover_region(struct device *dev, void *root)
> * Region enumeration is opportunistic, if this add-event fails,
> * continue to the next endpoint decoder.
> */
> - rc = cxl_add_to_region(root, cxled);
> + rc = cxl_add_to_region(cxled);
> if (rc)
> dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
> cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
> @@ -95,7 +95,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
> struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
> struct cxl_dev_state *cxlds = cxlmd->cxlds;
> struct cxl_hdm *cxlhdm;
> - struct cxl_port *root;
> int rc;
>
> rc = cxl_dvsec_rr_decode(cxlds, &info);
> @@ -126,19 +125,11 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
> if (rc)
> return rc;
>
> - /*
> - * This can't fail in practice as CXL root exit unregisters all
> - * descendant ports and that in turn synchronizes with cxl_port_probe()
> - */
> - struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> -
> - root = &cxl_root->port;
> -
> /*
> * Now that all endpoint decoders are successfully enumerated, try to
> * assemble regions from committed decoders
> */
> - device_for_each_child(&port->dev, root, discover_region);
> + device_for_each_child(&port->dev, NULL, discover_region);
>
> return 0;
> }