Re: [PATCH v4 12/14] cxl: Simplify cxl_rd_ops allocation and handling

Alison Schofield
2025-11-04T23:02:34Z

On Mon, Nov 03, 2025 at 07:47:53PM +0100, Robert Richter wrote:
> A root decoder's callback handlers are collected in struct cxl_rd_ops.
> The structure is dynamically allocated, though it contains only a few
> pointers in it. This also requires to check two pointes to check for
> the existance of a callback.
>
> Simplify the allocation, release and handler check by embedding the
> ops statical in struct cxl_root_decoder.
>
> Implementation is equivalent to how struct cxl_root_ops handles the
> callbacks.

The allocation was intentionally dynamic because the root decoder ops
only existed for CFMWS's defined w XOR Arithmetic.

From the commit msg:
>> To avoid maintaining a static ops instance populated with mostly NULL
>> pointers, allocate the ops structure dynamically only when a platform
>> requires overrides (e.g. XOR interleave decoding).
>> The setup can be extended as additional callbacks are added.
See: 524b2b76f365 ("cxl: Move hpa_to_spa callback to a new root decoder ops structure")

Has the usage changed?

-- Alison

>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
> drivers/cxl/acpi.c | 8 ++------
> drivers/cxl/core/region.c | 20 +++++---------------
> drivers/cxl/cxl.h | 2 +-
> 3 files changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index f9bbc77f3ec2..778ee29430ea 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -471,12 +471,8 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
> cxlrd->qos_class = cfmws->qtg_id;
>
> if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_XOR) {
> - cxlrd->ops = kzalloc(sizeof(*cxlrd->ops), GFP_KERNEL);
> - if (!cxlrd->ops)
> - return -ENOMEM;
> -
> - cxlrd->ops->hpa_to_spa = cxl_apply_xor_maps;
> - cxlrd->ops->spa_to_hpa = cxl_apply_xor_maps;
> + cxlrd->ops.hpa_to_spa = cxl_apply_xor_maps;
> + cxlrd->ops.spa_to_hpa = cxl_apply_xor_maps;
> }
>
> rc = cxl_decoder_add(cxld);
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 379a67cc8e31..dec003084521 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2932,16 +2932,6 @@ static bool cxl_is_hpa_in_chunk(u64 hpa, struct cxl_region *cxlr, int pos)
> return false;
> }
>
> -static bool has_hpa_to_spa(struct cxl_root_decoder *cxlrd)
> -{
> - return cxlrd->ops && cxlrd->ops->hpa_to_spa;
> -}
> -
> -static bool has_spa_to_hpa(struct cxl_root_decoder *cxlrd)
> -{
> - return cxlrd->ops && cxlrd->ops->spa_to_hpa;
> -}
> -
> u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> u64 dpa)
> {
> @@ -2996,8 +2986,8 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> hpa = hpa_offset + p->res->start + p->cache_size;
>
> /* Root decoder translation overrides typical modulo decode */
> - if (has_hpa_to_spa(cxlrd))
> - hpa = cxlrd->ops->hpa_to_spa(cxlrd, hpa);
> + if (cxlrd->ops.hpa_to_spa)
> + hpa = cxlrd->ops.hpa_to_spa(cxlrd, hpa);
>
> if (!cxl_resource_contains_addr(p->res, hpa)) {
> dev_dbg(&cxlr->dev,
> @@ -3006,7 +2996,7 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> }
>
> /* Simple chunk check, by pos & gran, only applies to modulo decodes */
> - if (!has_hpa_to_spa(cxlrd) && (!cxl_is_hpa_in_chunk(hpa, cxlr, pos)))
> + if (!cxlrd->ops.hpa_to_spa && !cxl_is_hpa_in_chunk(hpa, cxlr, pos))
> return ULLONG_MAX;
>
> return hpa;
> @@ -3041,8 +3031,8 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> * If the root decoder has SPA to CXL HPA callback, use it. Otherwise
> * CXL HPA is assumed to equal SPA.
> */
> - if (has_spa_to_hpa(cxlrd)) {
> - hpa = cxlrd->ops->spa_to_hpa(cxlrd, p->res->start + offset);
> + if (cxlrd->ops.spa_to_hpa) {
> + hpa = cxlrd->ops.spa_to_hpa(cxlrd, p->res->start + offset);
> hpa_offset = hpa - p->res->start;
> } else {
> hpa_offset = offset;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0af46d1b0abc..75fd45ddca38 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -451,7 +451,7 @@ struct cxl_root_decoder {
> void *platform_data;
> struct mutex range_lock;
> int qos_class;
> - struct cxl_rd_ops *ops;
> + struct cxl_rd_ops ops;
> struct cxl_switch_decoder cxlsd;
> };
>
> --
> 2.47.3
>