Dave Jiang wrote:
>
>
> On 11/4/24 7:10 PM, [email protected] wrote:
> > From: Navneet Singh <[email protected]>
> >
> > 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 <[email protected]>
> > Co-authored-by: Sushant1 Kumar <[email protected]>
> > Signed-off-by: Sushant1 Kumar <[email protected]>
> > Co-authored-by: Ira Weiny <[email protected]>
> > Signed-off-by: Ira Weiny <[email protected]>
>
> 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]
