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

[Ira Weiny <ira.weiny@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;
> +	}

I feel like the default for p->mode should be moved here from
parse_create_options().  But I'm not sure what the flows might be like in
that case.  That means p->mode would default to NONE until here.

That would make the man page behavior and this function match up nicely
for future maintenance.

But I don't think this is wrong.  So:

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> +}
> +
>  static int create_region_validate_config(struct cxl_ctx *ctx,
>  					 struct parsed_params *p)
>  {
> @@ -481,6 +506,8 @@ found:
>  		return -ENXIO;
>  	}
>  
> +	set_type_from_decoder(ctx, p);
> +
>  	rc = validate_decoder(p->root_decoder, p);
>  	if (rc)
>  		return rc;
> 
> -- 
> 2.39.1
> 
>