Re: [RFC PATCH 3/3] dcd: Add support for multiple DC regions

[Ira Weiny <ira.weiny@intel.com>]

anisa.su887@ wrote:
> From: Fan Ni <fan.ni@samsung.com>
> 
> With the change, we add following support:
> 1. Allow creating multiple DC regions (up to 8);
> 2. Allow DC extents to belong to regions other than region 0;
> 3. Modify sysfs entries to enable the above capabilities;
> 4. Shareable attribute is added to dc region (partition);
> 
> This series is tested with proper NDCTL fix, see:
> https://github.com/anisa-su993/anisa-ndctl/tree/multiple-dc-region-support
> 

[snip]

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 6b976da4a70a..faa4656f9542 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -463,8 +463,22 @@ static const char *cxl_mode_name(enum cxl_partition_mode mode)
>  		return "ram";
>  	case CXL_PARTMODE_PMEM:
>  		return "pmem";
> -	case CXL_PARTMODE_DYNAMIC_RAM_A:
> -		return "dynamic_ram_a";
> +	case CXL_PARTMODE_DYNAMIC_RAM_0:
> +		return "dynamic_ram_0";

If my v9 were to land then this lands this would break users who developed
against 'ram_a'.

Either we need to change ram_a to ram_0 in the base series or use
ram_[b,c,...] etc.

[same comment throughout]

> +	case CXL_PARTMODE_DYNAMIC_RAM_1:
> +		return "dynamic_ram_1";
> +	case CXL_PARTMODE_DYNAMIC_RAM_2:
> +		return "dynamic_ram_2";
> +	case CXL_PARTMODE_DYNAMIC_RAM_3:
> +		return "dynamic_ram_3";
> +	case CXL_PARTMODE_DYNAMIC_RAM_4:
> +		return "dynamic_ram_4";
> +	case CXL_PARTMODE_DYNAMIC_RAM_5:
> +		return "dynamic_ram_5";
> +	case CXL_PARTMODE_DYNAMIC_RAM_6:
> +		return "dynamic_ram_6";
> +	case CXL_PARTMODE_DYNAMIC_RAM_7:
> +		return "dynamic_ram_7";
>  	default:
>  		return "";
>  	};

[snip]

> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 2bad68f13e21..e28cd6827c7d 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -106,7 +106,7 @@ int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  			 resource_size_t base, resource_size_t len,
>  			 resource_size_t skipped);
>  
> -#define CXL_NR_PARTITIONS_MAX 3
> +#define CXL_NR_PARTITIONS_MAX 10
>  
>  struct cxl_dpa_info {
>  	u64 size;
> @@ -456,6 +456,7 @@ struct cxl_dev_state {
>  	struct resource dpa_res;
>  	struct cxl_dpa_partition part[CXL_NR_PARTITIONS_MAX];
>  	unsigned int nr_partitions;
> +	unsigned int nr_dc_partitions;

I think nr_partitions needs to include the dc count.  And when it does I
don't think we need a separate dc count.

After looking at this patch I'm thinking that the changes made after
dropping partition support from v8 to v9 the merging of the
volitile/persistant/dc partitions into a single range might have made this
support easier?

Is that why yall did not try to use v8?  I've really not looked into the
detail of if this is really all that is needed to support more partitions.
If this is all it takes I think what we really need is a use case.

Basically keep this patch (with the name change I mention) until such time
as v9 lands with a use case for single partitions.  Then when multiple
partitions come we can land this change.

patch 1/3 is a bug fix and needs to be in v9.  2/3 I don't quite
understand yet but is a bug fix as well.  So if it is an issue it will
need to go in with v9.

Thanks for the work!
Ira

[snip]