Re: [PATCH 01/16] cxl/regs: Add cxl_unmap_component_regs()

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Wed, 30 Jul 2025 16:47:03 -0500
Ben Cheatham <Benjamin.Cheatham@amd.com> wrote:

Hi Ben,

Whilst I know there has been some discussion of whether it is appropriate
to enable this functionality without a full use case and definite confirmation
someone is going to use it, I'm loath to leave completely unread code
on list.  (you talk about this in the cover letter).

> In order to allocate the MSI/-X interrupt for CXL error isolation the
> PCIe portdrv driver needs to map the MMIO-space isolation capability
> register that contains the interrupt vector. The CXL core already
> provides a function to map registers in this space:
> cxl_map_component_regs(). The mappings given this function are managed

given to, or returned by?

> by devres. If the isolation registers are left mapped by the PCIe
> portdrv driver, the CXL driver will run into a devres conflict when it
> goes to map the same registers during probe.
> 
> Add cxl_unmap_component_regs() to be called from the PCIe portdrv
> driver. This function will unwind the devres allocations done by
> cxl_map_component_regs(), which will allow the PCIe portdrv driver to
> map the isolation capability register without conflicts.
> 
> Factor out the register mapping retrieval code in
> cxl_map_component_regs() to be reused by cxl_map/unmap_component_regs().
> 
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> ---
>  drivers/cxl/core/regs.c | 77 +++++++++++++++++++++++++++++++----------
>  drivers/cxl/cxl.h       |  3 ++
>  2 files changed, 62 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index b8e767a9571c..da8e668a3b70 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -201,40 +201,81 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_iomap_block, "CXL");
>  
> -int cxl_map_component_regs(const struct cxl_register_map *map,
> +struct mapinfo {
> +	const struct cxl_reg_map *rmap;
> +	void __iomem **addr;
> +};
> +
> +static int cxl_get_mapinfo(const struct cxl_register_map *map,
>  			   struct cxl_component_regs *regs,
> -			   unsigned long map_mask)
> +			   unsigned long map_mask, struct mapinfo *info)
>  {
> -	struct device *host = map->host;
> -	struct mapinfo {
> -		const struct cxl_reg_map *rmap;
> -		void __iomem **addr;
> -	} mapinfo[] = {
> +	struct mapinfo mapinfo[] = {
>  		{ &map->component_map.hdm_decoder, &regs->hdm_decoder },
>  		{ &map->component_map.ras, &regs->ras },
>  	};
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
> -		struct mapinfo *mi = &mapinfo[i];

Whilst the usefulness of this local variable is reduced I think
it is still justified and keeping it will reduce the churn
here somewhat.

> -		resource_size_t addr;
> -		resource_size_t length;
> -
> -		if (!mi->rmap->valid)
> +		if (!mapinfo[i].rmap->valid)
>  			continue;
> -		if (!test_bit(mi->rmap->id, &map_mask))
> +		if (!test_bit(mapinfo[i].rmap->id, &map_mask))
>  			continue;
> -		addr = map->resource + mi->rmap->offset;
> -		length = mi->rmap->size;
> -		*(mi->addr) = devm_cxl_iomap_block(host, addr, length);
> -		if (!*(mi->addr))
> -			return -ENOMEM;
> +
> +		*info = mapinfo[i];
> +		return 0;

I'm struggling to see how this logic works.  We now only find the first
entry in mapinfo that is valid? The new cxl_map_component_regs()
doesn't seem to include a loop either so it's not that we enter here
multiple times.


I think this only works if only one bit is set in map_mask?
It is always called like that, but the function doesn't check it and
if we are going to always call it with BIT(X) then pass X in instead
that making it inherently only deal with a one hot bitmap.


>  	}
>  
> +	return -ENXIO;
> +}
> +
> +int cxl_map_component_regs(const struct cxl_register_map *map,
> +			   struct cxl_component_regs *regs,
> +			   unsigned long map_mask)

Similarly I'd make this the bit position not the bitmap with
1 bit set.

> +{
> +	struct device *host = map->host;
> +	resource_size_t addr, length;
> +	struct mapinfo mi;
> +	int rc;
> +
> +	rc = cxl_get_mapinfo(map, regs, map_mask, &mi);
> +	if (rc)
> +		return rc;
> +
> +	addr = map->resource + mi.rmap->offset;
> +	length = mi.rmap->size;
> +	*mi.addr = devm_cxl_iomap_block(host, addr, length);
> +	if (!(*mi.addr))
> +		return -ENOMEM;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_map_component_regs, "CXL");
>  
> +int cxl_unmap_component_regs(const struct cxl_register_map *map,
> +			     struct cxl_component_regs *regs,
> +			     unsigned long map_mask)
And this one.

> +{
> +	struct device *host = map->host;
> +	resource_size_t addr, length;
> +	struct mapinfo mi;
> +	int rc;
> +
> +	rc = cxl_get_mapinfo(map, regs, map_mask, &mi);
> +	if (rc)
> +		return rc;
> +
> +	if (!(*mi.addr))
> +		return 0;
> +
> +	addr = map->resource + mi.rmap->offset;
> +	length = mi.rmap->size;
> +
> +	devm_iounmap(host, *mi.addr);
> +	devm_release_mem_region(host, addr, length);

Add a little helper for devm_cxl_iounmap_block()
So that it clearly pairs with what happens in devm_cxl_iomap_block()


> +	return 0;
> +}