Re: [ndctl PATCH v2 4/6] cxl/region: Add creation of Dynamic capacity regions

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

Dave Jiang wrote:
> 
> 
> On 11/4/24 7:10 PM, ira.weiny@intel.com wrote:
> > From: Navneet Singh <navneet.singh@intel.com>
> > 
> > CXL Dynamic Capacity Devices (DCDs) optionally support dynamic capacity
> > with up to eight partitions (Regions) (dc0-dc7).  CXL regions can now be
> > spare and defined as dynamic capacity (dc).
> > 
> > Add support for DCD devices.  Query for DCD capabilities.  Add the
> > ability to add DC partitions to a CXL DC region.
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > Co-authored-by: Sushant1 Kumar <sushant1.kumar@intel.com>
> > Signed-off-by: Sushant1 Kumar <sushant1.kumar@intel.com>
> > Co-authored-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> A few small things below.

[snip]

> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index 5cbfb3e7d466b491ef87ea285f7e50d3bac230db..4caa2d02313bf71960971c4eaa67fa42cea08d55 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -1267,7 +1267,6 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
> >  	char buf[SYSFS_ATTR_SIZE];
> >  	struct stat st;
> >  	char *host;
> > -
> 
> Stray line delete

Oh...  I thought that was adding a space here when I reviewed the diff.
Yes fixed.

> >  	if (!path)
> >  		return NULL;
> >  	dbg(ctx, "%s: base: \'%s\'\n", devname, cxlmem_base);
> > @@ -1304,6 +1303,19 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)

[snip]

> > @@ -1540,6 +1552,14 @@ CXL_EXPORT int cxl_memdev_get_ram_qos_class(struct cxl_memdev *memdev)
> >  	return memdev->ram_qos_class;
> >  }
> >  
> > +CXL_EXPORT unsigned long long cxl_memdev_get_dc_size(struct cxl_memdev *memdev, int index)
> 
> If you make index 'unsigned int', you can skip the index >= 0 check below right?

True.  To be safe the following needs to be done then.

diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 17ed682548b9..e0584cd658ec 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -263,8 +263,10 @@ static inline bool cxl_decoder_mode_is_dc(enum cxl_decoder_mode mode)
        return (mode >= CXL_DECODER_MODE_DC0 && mode <= CXL_DECODER_MODE_DC7);
 }
 
-static inline int cxl_decoder_dc_mode_to_index(enum cxl_decoder_mode mode)
+static inline unsigned cxl_decoder_dc_mode_to_index(enum cxl_decoder_mode mode)
 {
+       if (mode < CXL_DECODER_MODE_DC0)
+               return 0;
        return mode - CXL_DECODER_MODE_DC0;
 }
 
I'm not sure which is best.  Right now cxl_decoder_dc_mode_to_index() is only
called if the mode has been checked to be DC.  So the above check is a noop.
But in general returning an errno would be better for
cxl_decoder_dc_mode_to_index() is in error.

I'll clean this up to match what is in dc_mode_to_region_index().  See below.

[snip]

> > @@ -2318,6 +2354,14 @@ static void *add_cxl_decoder(void *parent, int id, const char *cxldecoder_base)
> >  	case CXL_PORT_SWITCH:
> >  		decoder->pmem_capable = true;
> >  		decoder->volatile_capable = true;
> > +		decoder->dc_capable[0] = true;
> > +		decoder->dc_capable[1] = true;
> > +		decoder->dc_capable[2] = true;
> > +		decoder->dc_capable[3] = true;
> > +		decoder->dc_capable[4] = true;
> > +		decoder->dc_capable[5] = true;
> > +		decoder->dc_capable[6] = true;
> > +		decoder->dc_capable[7] = true;
> 
> for loop?

Sure.  Changed.

> 
> >  		decoder->mem_capable = true;
> >  		decoder->accelmem_capable = true;
> >  		sprintf(path, "%s/locked", cxldecoder_base);

[snip]

> > @@ -2648,6 +2724,14 @@ CXL_EXPORT bool cxl_decoder_is_mem_capable(struct cxl_decoder *decoder)
> >  	return decoder->mem_capable;
> >  }
> >  
> > +CXL_EXPORT bool cxl_decoder_is_dc_capable(struct cxl_decoder *decoder, int index)
> 
> Same comment as above WRT index.

This forces changes outside of cxl_decoder_is_dc_capable().  Specifically with
dc_mode_to_region_index().  But I actually like checking for the error from
dc_mode_to_region_index() outside of cxl_decoder_is_dc_capable().

I've made the change.


[snip]

> > diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> > index 0f45be89b6a00477d13fb6d7f1906213a3073c48..10abfa63dfc759b1589f9f039da1b920f8eb605e 100644
> > --- a/cxl/lib/private.h
> > +++ b/cxl/lib/private.h
> > @@ -12,7 +12,6 @@
> >  #include <util/bitmap.h>
> >  
> >  #define CXL_EXPORT __attribute__ ((visibility("default")))
> > -
> 
> Errant line deletion

Fixed.

> 
> >  struct cxl_pmem {
> >  	int id;
> >  	void *dev_buf;
> > @@ -47,6 +46,9 @@ struct cxl_memdev {
> >  	struct list_node list;
> >  	unsigned long long pmem_size;
> >  	unsigned long long ram_size;
> > +	unsigned long long dc_size[MAX_NUM_DC_REGIONS];
> > +	unsigned long long dc_qos_class[MAX_NUM_DC_REGIONS];
> > +	int dc_partition_count;
> >  	int ram_qos_class;
> >  	int pmem_qos_class;
> >  	int payload_max;
> > @@ -111,6 +113,7 @@ struct cxl_endpoint {
> >  	struct cxl_memdev *memdev;
> >  };
> >  
> > +
> 
> Errant line addition

Fixed.


[snip]

> > @@ -488,6 +506,13 @@ static int validate_decoder(struct cxl_decoder *decoder,
> >  			return -EINVAL;
> >  		}
> >  		break;
> > +	case CXL_DECODER_MODE_DC0 ... CXL_DECODER_MODE_DC7:
> > +		index =  dc_mode_to_region_index(p->mode);
> 
> Extra space after =

Fixed.

Thanks for the review!

Ira
> 
> DJ

[snip]