Re: [PATCH v1 05/20] cxl/region: Separate cxl_region_alloc() from devm_cxl_add_region()

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Tue, 15 Jul 2025 21:11:28 +0200 Robert Richter <rrichter@amd.com> wrote:

> In interleaving configs multiple endpoint decoders connect to the same
> region. The region's parameters must be the same for all endpoint
> decoders that share the interleaving setup. During initialization, the
> region's parameters are determined for each endpoint decoder.
> If a region for the same hpa range already exists, no new region is
> created and the existing one is reused.
> 
> To simplify region setup and the collection of the region parameters,
> separate region allocation from its registration. This allows it to
> allocate and setup a region before checking the parameters with
> existing other regions and adding it to the cxl tree or releasing it
> and instead reusing an existing region.
> 
> Here, only separate cxl_region_alloc() from devm_cxl_add_region().
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---

Hello Robert,

Thank you for this great patch! I have one small nit:

>  /**
> - * devm_cxl_add_region - Adds a region to a decoder
> - * @cxlrd: root decoder
> - * @id: memregion id to create, or memregion_free() on failure
> - * @mode: mode for the endpoint decoders of this region
> - * @type: select whether this is an expander or accelerator (type-2 or type-3)
> + * devm_cxl_add_region - Adds a region to the CXL hierarchy.
> + * @cxlr: region to be added
> + * @id: memregion id to create must match current @port_id of the
> + *      region's @cxlrd
>   *
>   * This is the second step of region initialization. Regions exist within an
>   * address space which is mapped by a @cxlrd.
>   *
> - * Return: 0 if the region was added to the @cxlrd, else returns negative error
> - * code. The region will be named "regionZ" where Z is the unique region number.
> + * Return: Pointer to the region if the region could be registered
> + * (for use in a tail call). The region will be named "regionZ" where
> + * Z is the unique region number. On errors, devm_cxl_add_region()
> + * returns an encoded negative error code and releases or unregisters
> + * @cxlr.
>   */

It seems like the changes that this new return description corresponds to
were actually made in the previous patch. Would it make sense to move this
new description to the previous patch?

> -static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
> -					      int id,
> -					      enum cxl_partition_mode mode,
> -					      enum cxl_decoder_type type)
> +static struct cxl_region *devm_cxl_add_region(struct cxl_region *cxlr, int id)

Otherwise, LGTM!

Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>

Sent using hkml (https://github.com/sjp38/hackermail)