RE: [PATCH ndctl 5/7] cxl/region: determine region type based on root decoder capability

[Dan Williams <dan.j.williams@intel.com>]

Vishal Verma wrote:
> In the common case, root decoders are expected to be either pmem
> capable, or volatile capable, but not necessarily both simultaneously.
> If a decoder only has one of pmem or volatile capabilities,
> cxl-create-region should just infer the type of the region (pmem
> or ram) based on this capability.
> 
> Maintain the default behavior of cxl-create-region to choose type=pmem,
> but only as a fallback if the selected root decoder has multiple
> capabilities. If it is only capable of either pmem, or ram, then infer
> region type from this without requiring it to be specified explicitly.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/cxl-create-region.txt |  3 ++-
>  cxl/region.c                            | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
> index ada0e52..f11a412 100644
> --- a/Documentation/cxl/cxl-create-region.txt
> +++ b/Documentation/cxl/cxl-create-region.txt
> @@ -75,7 +75,8 @@ include::bus-option.txt[]
>  
>  -t::
>  --type=::
> -	Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
> +	Specify the region type - 'pmem' or 'ram'. Default to root decoder
> +	capability, and if that is ambiguous, default to 'pmem'.
>  
>  -U::
>  --uuid=::
> diff --git a/cxl/region.c b/cxl/region.c
> index 9079b2d..1c8ccc7 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder,
>  	return 0;
>  }
>  
> +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
> +{
> +	int num_cap = 0;
> +
> +	/* if param.type was explicitly specified, nothing to do here */
> +	if (param.type)
> +		return;
> +
> +	/*
> +	 * if the root decoder only has one type of capability, default
> +	 * to that mode for the region.
> +	 */
> +	if (cxl_decoder_is_pmem_capable(p->root_decoder))
> +		num_cap++;
> +	if (cxl_decoder_is_volatile_capable(p->root_decoder))
> +		num_cap++;
> +
> +	if (num_cap == 1) {
> +		if (cxl_decoder_is_volatile_capable(p->root_decoder))
> +			p->mode = CXL_DECODER_MODE_RAM;
> +		else if (cxl_decoder_is_pmem_capable(p->root_decoder))
> +			p->mode = CXL_DECODER_MODE_PMEM;
> +	}

Is @num_cap needed? I.e. if this just does:

    if (cxl_decoder_is_volatile_capable(p->root_decoder))
    	p->mode = CXL_DECODER_MODE_RAM;
    if (cxl_decoder_is_pmem_capable(p->root_decoder))
    	p->mode = CXL_DECODER_MODE_PMEM;

...then it matches the changelog of defaulting to pmem if both types are
set, and otherwise the single capability dominates.