Re: [PATCH v13 05/15] cxl: Introduce reusable HDM decoder settings

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 22 Sep 2026 08:39:14 +0000
Srirangan Madhavan <smadhavan@nvidia.com> wrote:

> Represent HDM programming state with struct cxl_decoder_settings and pass
> it to the commit helpers. Keep endpoint skip and switch targets at their
> call sites, place flags with the programming state, and leave runtime
> region ownership outside the snapshot.
> 
> Separate commit initiation from completion waiting so reset restoration
> can reuse register programming without changing normal DPA-lock policy.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
Hi Srirangan,

I'm not sure if others will agree, but my inclination here
would not be to use the struct_group_tagged machine because
of the structure then being embedded in two places.

I'd just define the structure independently and pay the price
in churn to access the version in cxl_decoder.  The point being
to say 'these are the bits of cxl_decoder that we need
to store for reset purposes'.

Jonathan

> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 69941f7fdb76..1acdf537e4e9 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -42,8 +42,8 @@ enum cxl_decoder_type {
>   * @interleave_ways: number of cxl_dports in this decode
>   * @interleave_granularity: data stride per dport
>   * @target_type: accelerator vs expander (type2 vs type3) selector
> - * @region: currently assigned region for this decoder
>   * @flags: memory type capabilities and locking
> + * @region: currently assigned region for this decoder
>   * @target_map: cached copy of hardware port-id list, available at init
>   *              before all @dport objects have been instantiated. While
>   *              dport id is 8bit, CFMWS interleave targets are 32bits.
> @@ -52,18 +52,31 @@ enum cxl_decoder_type {
>  */
>  struct cxl_decoder {
>  	struct device dev;
> -	int id;
> -	struct range hpa_range;
> -	int interleave_ways;
> -	int interleave_granularity;
> -	enum cxl_decoder_type target_type;
> +
> +	struct_group_tagged(cxl_decoder_config, config,

Whilst strictly speaking correct, to me struct_group_tagged()
should only really be used when there is only one place that
the structure appears.  Otherwise it is just a bit obscure
to read.  It does give us the option of accessing the elements
in here without the extra jump, but I'm not sure that is
a good idea in this case.  So I'd just make this a normal
structure.  After all what is special about it being
defined here that isn't equally true of the cxl_decoder_settings
structure below?

> +		int id;
> +		struct range hpa_range;
> +		int interleave_ways;
> +		int interleave_granularity;
> +		enum cxl_decoder_type target_type;
> +		unsigned long flags;
> +	);
>  	struct cxl_region *region;
> -	unsigned long flags;
>  	u32 target_map[CXL_DECODER_MAX_INTERLEAVE];
>  	int (*commit)(struct cxl_decoder *cxld);
>  	void (*reset)(struct cxl_decoder *cxld);
>  };
>  
> +/**
> + * struct cxl_decoder_settings - CXL HDM decoder programming snapshot
> + * @config: common decoder configuration
> + * @target_or_skip_reg_val: switch target list or endpoint skip register value
> + */
> +struct cxl_decoder_settings {
> +	struct cxl_decoder_config config;
> +	u64 target_or_skip_reg_val;
> +};
> +
>  /*
>   * Using struct_group() allows for per register-block-type helper routines,
>   * without requiring block-type agnostic code to include the prefix.