On Wed, 30 Jul 2025 16:47:03 -0500
Ben Cheatham <Benjamin.Cheatham@xxxxxxx> 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@xxxxxxx>
> ---
> 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, ®s->hdm_decoder },
> { &map->component_map.ras, ®s->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;
> +}
