| Message ID | 20240324-dcd-type2-upstream-v1-2-b7b00d623625@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | DCD: Add support for Dynamic Capacity Devices (DCD) | expand |
On Sun, 24 Mar 2024 16:18:05 -0700 ira.weiny@intel.com wrote: > From: Navneet Singh <navneet.singh@intel.com> > > Until now region modes and decoder modes were equivalent in that they > were either PMEM or RAM. With the upcoming addition of Dynamic Capacity > regions (which will represent an array of device regions [better named > partitions] the index of which could be different on different > interleaved devices), the mode of an endpoint decoder and a region will > no longer be equivalent. > > Define a new region mode enumeration and adjust the code for it. > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Signed-off-by: Navneet Singh <navneet.singh@intel.com> > Co-developed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> I can't really remember the reasoning behind this split, but from a fresh read it seems reasonable. Some trivial comments inline. Jonathan > > --- > Changes for v1 > <none> > --- > drivers/cxl/core/region.c | 77 +++++++++++++++++++++++++++++++++++------------ > drivers/cxl/cxl.h | 26 ++++++++++++++-- > 2 files changed, 81 insertions(+), 22 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 4c7fd2d5cccb..1723d17f121e 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2800,6 +2814,24 @@ static int match_region_by_range(struct device *dev, void *data) > return rc; > } > > +static enum cxl_region_mode > +cxl_decoder_to_region_mode(enum cxl_decoder_mode mode) > +{ > + switch (mode) { > + case CXL_DECODER_NONE: > + return CXL_REGION_NONE; > + case CXL_DECODER_RAM: > + return CXL_REGION_RAM; > + case CXL_DECODER_PMEM: > + return CXL_REGION_PMEM; > + case CXL_DECODER_MIXED: > + default: > + return CXL_REGION_MIXED; > + } > + Dead code. > + return CXL_REGION_MIXED; > +} > + > /* Establish an empty region covering the given HPA range */ > static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > struct cxl_endpoint_decoder *cxled) > @@ -2808,12 +2840,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > struct cxl_port *port = cxlrd_to_port(cxlrd); > struct range *hpa = &cxled->cxld.hpa_range; > struct cxl_region_params *p; > + enum cxl_region_mode mode; > struct cxl_region *cxlr; > struct resource *res; > int rc; > > + if (cxled->mode == CXL_DECODER_DEAD) > + return ERR_PTR(-EINVAL); Not a bad thing necessarily, but why do we now need this and didn't before? > + > + mode = cxl_decoder_to_region_mode(cxled->mode); > do { > - cxlr = __create_region(cxlrd, cxled->mode, > + cxlr = __create_region(cxlrd, mode, > atomic_read(&cxlrd->region_id)); > } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY); > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 003feebab79b..9a0cce1e6fca 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > /* > * Track whether this decoder is reserved for region autodiscovery, or > * free for userspace provisioning. > @@ -511,7 +532,8 @@ struct cxl_region_params { > * struct cxl_region - CXL region > * @dev: This region's device > * @id: This region's id. Id is globally unique across all regions > - * @mode: Endpoint decoder allocation / access mode > + * @mode: Region mode which defines which endpoint decoder mode the region is mode or potentially modes? If region is mixed, I guess that means endpoint could be pmem or ram in theory? Don't think anyone has implemented anything yet, but is the potential there? > + * compatible with
On Sun, 24 Mar 2024, ira.weiny@intel.com wrote: >From: Navneet Singh <navneet.singh@intel.com> > >Until now region modes and decoder modes were equivalent in that they >were either PMEM or RAM. With the upcoming addition of Dynamic Capacity >regions (which will represent an array of device regions [better named >partitions] the index of which could be different on different >interleaved devices), the mode of an endpoint decoder and a region will >no longer be equivalent. > >Define a new region mode enumeration and adjust the code for it. Could this could also be picked up regardless of dcd? Thanks, Davidlohr
Davidlohr Bueso wrote: > On Sun, 24 Mar 2024, ira.weiny@intel.com wrote: > > >From: Navneet Singh <navneet.singh@intel.com> > > > >Until now region modes and decoder modes were equivalent in that they > >were either PMEM or RAM. With the upcoming addition of Dynamic Capacity > >regions (which will represent an array of device regions [better named > >partitions] the index of which could be different on different > >interleaved devices), the mode of an endpoint decoder and a region will > >no longer be equivalent. > > > >Define a new region mode enumeration and adjust the code for it. > > Could this could also be picked up regardless of dcd? It could but there is no need for it without DCD. I will work on re-ordering the cleanups if Dave will agree to take them early. Ira
On 3/27/24 10:22 PM, Ira Weiny wrote: > Davidlohr Bueso wrote: >> On Sun, 24 Mar 2024, ira.weiny@intel.com wrote: >> >>> From: Navneet Singh <navneet.singh@intel.com> >>> >>> Until now region modes and decoder modes were equivalent in that they >>> were either PMEM or RAM. With the upcoming addition of Dynamic Capacity >>> regions (which will represent an array of device regions [better named >>> partitions] the index of which could be different on different >>> interleaved devices), the mode of an endpoint decoder and a region will >>> no longer be equivalent. >>> >>> Define a new region mode enumeration and adjust the code for it. >> >> Could this could also be picked up regardless of dcd? > > It could but there is no need for it without DCD. > > I will work on re-ordering the cleanups if Dave will agree to take them > early. There's no reason for the change unless it comes with DCD right? And probably no urgent need to taking it ahead then? > > Ira
Jonathan Cameron wrote: > On Sun, 24 Mar 2024 16:18:05 -0700 > ira.weiny@intel.com wrote: [snip] > > > > --- > > Changes for v1 > > <none> > > --- > > drivers/cxl/core/region.c | 77 +++++++++++++++++++++++++++++++++++------------ > > drivers/cxl/cxl.h | 26 ++++++++++++++-- > > 2 files changed, 81 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index 4c7fd2d5cccb..1723d17f121e 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > > > @@ -2800,6 +2814,24 @@ static int match_region_by_range(struct device *dev, void *data) > > return rc; > > } > > > > +static enum cxl_region_mode > > +cxl_decoder_to_region_mode(enum cxl_decoder_mode mode) > > +{ > > + switch (mode) { > > + case CXL_DECODER_NONE: > > + return CXL_REGION_NONE; > > + case CXL_DECODER_RAM: > > + return CXL_REGION_RAM; > > + case CXL_DECODER_PMEM: > > + return CXL_REGION_PMEM; > > + case CXL_DECODER_MIXED: > > + default: > > + return CXL_REGION_MIXED; > > + } > > + > > Dead code. Fixed thanks. > > > + return CXL_REGION_MIXED; > > +} > > + > > /* Establish an empty region covering the given HPA range */ > > static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > > struct cxl_endpoint_decoder *cxled) > > @@ -2808,12 +2840,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > > struct cxl_port *port = cxlrd_to_port(cxlrd); > > struct range *hpa = &cxled->cxld.hpa_range; > > struct cxl_region_params *p; > > + enum cxl_region_mode mode; > > struct cxl_region *cxlr; > > struct resource *res; > > int rc; > > > > + if (cxled->mode == CXL_DECODER_DEAD) > > + return ERR_PTR(-EINVAL); > > Not a bad thing necessarily, but why do we now need this and didn't before? Ah. Because in devm_cxl_add_region() the mode of CXL_DECODER_DEAD used to return -EINVAL. There is no logical equivalent to decoder dead in the region mode (regions don't need it). So this correctly flags the error based on the decoder mode rather than introduce a mode for regions which does not make sense. I'll update the commit message because that is hard to see. > > > + > > + mode = cxl_decoder_to_region_mode(cxled->mode); > > do { > > - cxlr = __create_region(cxlrd, cxled->mode, > > + cxlr = __create_region(cxlrd, mode, > > atomic_read(&cxlrd->region_id)); > > } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY); > > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 003feebab79b..9a0cce1e6fca 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > > > /* > > * Track whether this decoder is reserved for region autodiscovery, or > > * free for userspace provisioning. > > @@ -511,7 +532,8 @@ struct cxl_region_params { > > * struct cxl_region - CXL region > > * @dev: This region's device > > * @id: This region's id. Id is globally unique across all regions > > - * @mode: Endpoint decoder allocation / access mode > > + * @mode: Region mode which defines which endpoint decoder mode the region is > mode or potentially modes? > > If region is mixed, I guess that means endpoint could be pmem or ram in theory? > Don't think anyone has implemented anything yet, but is the potential there? Yes the potential is there. The endpoint decoder is set to CXL_DECODER_MIXED in __cxl_dpa_reserve(). But I am unclear how that will ever be executed except if the BIOS sets up a decoder to span ram/pmem. In this case the rest of the stack is not going to work and will complain about mixed mode. Ok Dan clued me in. Check out cxl_dpa_alloc(). Spanning partitions is not allowed. So the comment is targeted toward the 'normal' case even though the region could be created incorrectly via BIOS. Ira
Davidlohr Bueso wrote: > On Sun, 24 Mar 2024, ira.weiny@intel.com wrote: > > >From: Navneet Singh <navneet.singh@intel.com> > > > >Until now region modes and decoder modes were equivalent in that they > >were either PMEM or RAM. With the upcoming addition of Dynamic Capacity > >regions (which will represent an array of device regions [better named > >partitions] the index of which could be different on different > >interleaved devices), the mode of an endpoint decoder and a region will > >no longer be equivalent. > > > >Define a new region mode enumeration and adjust the code for it. > > Could this could also be picked up regardless of dcd? It could be. But there is no practical need for it without the addition of DCD. So I don't think it should be. Ira
Dave Jiang wrote: > > > On 3/27/24 10:22 PM, Ira Weiny wrote: > > Davidlohr Bueso wrote: > >> On Sun, 24 Mar 2024, ira.weiny@intel.com wrote: > >> > >>> From: Navneet Singh <navneet.singh@intel.com> > >>> > >>> Until now region modes and decoder modes were equivalent in that they > >>> were either PMEM or RAM. With the upcoming addition of Dynamic Capacity > >>> regions (which will represent an array of device regions [better named > >>> partitions] the index of which could be different on different > >>> interleaved devices), the mode of an endpoint decoder and a region will > >>> no longer be equivalent. > >>> > >>> Define a new region mode enumeration and adjust the code for it. > >> > >> Could this could also be picked up regardless of dcd? > > > > It could but there is no need for it without DCD. > > > > I will work on re-ordering the cleanups if Dave will agree to take them > > early. > > There's no reason for the change unless it comes with DCD right? And probably no urgent need to taking it ahead then? > > I think I just replied for a 2nd time to this... yea. LOL I have to do better... Ira
On Sun, Mar 24, 2024 at 04:18:05PM -0700, Ira Weiny wrote: > From: Navneet Singh <navneet.singh@intel.com> > > Until now region modes and decoder modes were equivalent in that they > were either PMEM or RAM. With the upcoming addition of Dynamic Capacity > regions (which will represent an array of device regions [better named > partitions] the index of which could be different on different > interleaved devices), the mode of an endpoint decoder and a region will > no longer be equivalent. > > Define a new region mode enumeration and adjust the code for it. > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Signed-off-by: Navneet Singh <navneet.singh@intel.com> > Co-developed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes for v1 > <none> > --- > drivers/cxl/core/region.c | 77 +++++++++++++++++++++++++++++++++++------------ > drivers/cxl/cxl.h | 26 ++++++++++++++-- > 2 files changed, 81 insertions(+), 22 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 4c7fd2d5cccb..1723d17f121e 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -40,7 +40,7 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr, > rc = down_read_interruptible(&cxl_region_rwsem); > if (rc) > return rc; > - if (cxlr->mode != CXL_DECODER_PMEM) > + if (cxlr->mode != CXL_REGION_PMEM) > rc = sysfs_emit(buf, "\n"); > else > rc = sysfs_emit(buf, "%pUb\n", &p->uuid); > @@ -353,7 +353,7 @@ static umode_t cxl_region_visible(struct kobject *kobj, struct attribute *a, > * Support tooling that expects to find a 'uuid' attribute for all > * regions regardless of mode. > */ > - if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_DECODER_PMEM) > + if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_REGION_PMEM) > return 0444; > return a->mode; > } > @@ -516,7 +516,7 @@ static ssize_t mode_show(struct device *dev, struct device_attribute *attr, > { > struct cxl_region *cxlr = to_cxl_region(dev); > > - return sysfs_emit(buf, "%s\n", cxl_decoder_mode_name(cxlr->mode)); > + return sysfs_emit(buf, "%s\n", cxl_region_mode_name(cxlr->mode)); > } > static DEVICE_ATTR_RO(mode); > > @@ -542,7 +542,7 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size) > > /* ways, granularity and uuid (if PMEM) need to be set before HPA */ > if (!p->interleave_ways || !p->interleave_granularity || > - (cxlr->mode == CXL_DECODER_PMEM && uuid_is_null(&p->uuid))) > + (cxlr->mode == CXL_REGION_PMEM && uuid_is_null(&p->uuid))) > return -ENXIO; > > div64_u64_rem(size, (u64)SZ_256M * p->interleave_ways, &remainder); > @@ -1683,6 +1683,17 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr) > return rc; > } > > +static bool cxl_modes_compatible(enum cxl_region_mode rmode, > + enum cxl_decoder_mode dmode) > +{ Perhaps is_region_mode_compatible() ? Seems we have precedence for asking these questions that have boolean responses. I picked 'region' because it is the region we are trying to construct. > + if (rmode == CXL_REGION_RAM && dmode == CXL_DECODER_RAM) > + return true; > + if (rmode == CXL_REGION_PMEM && dmode == CXL_DECODER_PMEM) > + return true; > + > + return false; > +} > + > static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_endpoint_decoder *cxled, int pos) > { > @@ -1693,9 +1704,11 @@ static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_dport *dport; > int rc = -ENXIO; > > - if (cxled->mode != cxlr->mode) { > - dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > - dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > + if (!cxl_modes_compatible(cxlr->mode, cxled->mode)) { > + dev_dbg(&cxlr->dev, "%s region mode: %s mismatch decoder: %s\n", > + dev_name(&cxled->cxld.dev), > + cxl_region_mode_name(cxlr->mode), > + cxl_decoder_mode_name(cxled->mode)); > return -EINVAL; > } Does the above return bypass this next code segment (not in your diff): if (cxled->mode == CXL_DECODER_DEAD) { dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); return -ENODEV; } It seems we are changing the return value on DEAD. More below where a new check for DEAD is added ... snip > /* Establish an empty region covering the given HPA range */ > static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > struct cxl_endpoint_decoder *cxled) > @@ -2808,12 +2840,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > struct cxl_port *port = cxlrd_to_port(cxlrd); > struct range *hpa = &cxled->cxld.hpa_range; > struct cxl_region_params *p; > + enum cxl_region_mode mode; > struct cxl_region *cxlr; > struct resource *res; > int rc; > > + if (cxled->mode == CXL_DECODER_DEAD) > + return ERR_PTR(-EINVAL); I see this addition, but it is in a different place and with a different return value. Help me understand that this is no change in behavior. > + > + mode = cxl_decoder_to_region_mode(cxled->mode); > do { > - cxlr = __create_region(cxlrd, cxled->mode, > + cxlr = __create_region(cxlrd, mode, > atomic_read(&cxlrd->region_id)); > } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY); > snip > /* > * Track whether this decoder is reserved for region autodiscovery, or > * free for userspace provisioning. > @@ -511,7 +532,8 @@ struct cxl_region_params { > * struct cxl_region - CXL region > * @dev: This region's device > * @id: This region's id. Id is globally unique across all regions > - * @mode: Endpoint decoder allocation / access mode > + * @mode: Region mode which defines which endpoint decoder mode the region is > + * compatible with Maybe... @mode: Region mode used for decoder compatibility check snip to end --Alison >
Dave Jiang wrote: > > > On 3/27/24 10:22 PM, Ira Weiny wrote: > > Davidlohr Bueso wrote: > >> On Sun, 24 Mar 2024, ira.weiny@intel.com wrote: > >> > >>> From: Navneet Singh <navneet.singh@intel.com> > >>> > >>> Until now region modes and decoder modes were equivalent in that they > >>> were either PMEM or RAM. With the upcoming addition of Dynamic Capacity > >>> regions (which will represent an array of device regions [better named > >>> partitions] the index of which could be different on different > >>> interleaved devices), the mode of an endpoint decoder and a region will > >>> no longer be equivalent. > >>> > >>> Define a new region mode enumeration and adjust the code for it. > >> > >> Could this could also be picked up regardless of dcd? > > > > It could but there is no need for it without DCD. > > > > I will work on re-ordering the cleanups if Dave will agree to take them > > early. > > There's no reason for the change unless it comes with DCD right? And probably no urgent need to taking it ahead then? No I don't think so. Ira > > > > Ira
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 4c7fd2d5cccb..1723d17f121e 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -40,7 +40,7 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr, rc = down_read_interruptible(&cxl_region_rwsem); if (rc) return rc; - if (cxlr->mode != CXL_DECODER_PMEM) + if (cxlr->mode != CXL_REGION_PMEM) rc = sysfs_emit(buf, "\n"); else rc = sysfs_emit(buf, "%pUb\n", &p->uuid); @@ -353,7 +353,7 @@ static umode_t cxl_region_visible(struct kobject *kobj, struct attribute *a, * Support tooling that expects to find a 'uuid' attribute for all * regions regardless of mode. */ - if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_DECODER_PMEM) + if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_REGION_PMEM) return 0444; return a->mode; } @@ -516,7 +516,7 @@ static ssize_t mode_show(struct device *dev, struct device_attribute *attr, { struct cxl_region *cxlr = to_cxl_region(dev); - return sysfs_emit(buf, "%s\n", cxl_decoder_mode_name(cxlr->mode)); + return sysfs_emit(buf, "%s\n", cxl_region_mode_name(cxlr->mode)); } static DEVICE_ATTR_RO(mode); @@ -542,7 +542,7 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size) /* ways, granularity and uuid (if PMEM) need to be set before HPA */ if (!p->interleave_ways || !p->interleave_granularity || - (cxlr->mode == CXL_DECODER_PMEM && uuid_is_null(&p->uuid))) + (cxlr->mode == CXL_REGION_PMEM && uuid_is_null(&p->uuid))) return -ENXIO; div64_u64_rem(size, (u64)SZ_256M * p->interleave_ways, &remainder); @@ -1683,6 +1683,17 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr) return rc; } +static bool cxl_modes_compatible(enum cxl_region_mode rmode, + enum cxl_decoder_mode dmode) +{ + if (rmode == CXL_REGION_RAM && dmode == CXL_DECODER_RAM) + return true; + if (rmode == CXL_REGION_PMEM && dmode == CXL_DECODER_PMEM) + return true; + + return false; +} + static int cxl_region_attach(struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled, int pos) { @@ -1693,9 +1704,11 @@ static int cxl_region_attach(struct cxl_region *cxlr, struct cxl_dport *dport; int rc = -ENXIO; - if (cxled->mode != cxlr->mode) { - dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", - dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); + if (!cxl_modes_compatible(cxlr->mode, cxled->mode)) { + dev_dbg(&cxlr->dev, "%s region mode: %s mismatch decoder: %s\n", + dev_name(&cxled->cxld.dev), + cxl_region_mode_name(cxlr->mode), + cxl_decoder_mode_name(cxled->mode)); return -EINVAL; } @@ -2168,7 +2181,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i * devm_cxl_add_region - Adds a region to a decoder * @cxlrd: root decoder * @id: memregion id to create, or memregion_free() on failure - * @mode: mode for the endpoint decoders of this region + * @mode: mode of this region * @type: select whether this is an expander or accelerator (type-2 or type-3) * * This is the second step of region initialization. Regions exist within an @@ -2179,7 +2192,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i */ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, int id, - enum cxl_decoder_mode mode, + enum cxl_region_mode mode, enum cxl_decoder_type type) { struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent); @@ -2188,11 +2201,12 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, int rc; switch (mode) { - case CXL_DECODER_RAM: - case CXL_DECODER_PMEM: + case CXL_REGION_RAM: + case CXL_REGION_PMEM: break; default: - dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode); + dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %s\n", + cxl_region_mode_name(mode)); return ERR_PTR(-EINVAL); } @@ -2242,7 +2256,7 @@ static ssize_t create_ram_region_show(struct device *dev, } static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, - enum cxl_decoder_mode mode, int id) + enum cxl_region_mode mode, int id) { int rc; @@ -2270,7 +2284,7 @@ static ssize_t create_pmem_region_store(struct device *dev, if (rc != 1) return -EINVAL; - cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id); + cxlr = __create_region(cxlrd, CXL_REGION_PMEM, id); if (IS_ERR(cxlr)) return PTR_ERR(cxlr); @@ -2290,7 +2304,7 @@ static ssize_t create_ram_region_store(struct device *dev, if (rc != 1) return -EINVAL; - cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id); + cxlr = __create_region(cxlrd, CXL_REGION_RAM, id); if (IS_ERR(cxlr)) return PTR_ERR(cxlr); @@ -2800,6 +2814,24 @@ static int match_region_by_range(struct device *dev, void *data) return rc; } +static enum cxl_region_mode +cxl_decoder_to_region_mode(enum cxl_decoder_mode mode) +{ + switch (mode) { + case CXL_DECODER_NONE: + return CXL_REGION_NONE; + case CXL_DECODER_RAM: + return CXL_REGION_RAM; + case CXL_DECODER_PMEM: + return CXL_REGION_PMEM; + case CXL_DECODER_MIXED: + default: + return CXL_REGION_MIXED; + } + + return CXL_REGION_MIXED; +} + /* Establish an empty region covering the given HPA range */ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, struct cxl_endpoint_decoder *cxled) @@ -2808,12 +2840,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, struct cxl_port *port = cxlrd_to_port(cxlrd); struct range *hpa = &cxled->cxld.hpa_range; struct cxl_region_params *p; + enum cxl_region_mode mode; struct cxl_region *cxlr; struct resource *res; int rc; + if (cxled->mode == CXL_DECODER_DEAD) + return ERR_PTR(-EINVAL); + + mode = cxl_decoder_to_region_mode(cxled->mode); do { - cxlr = __create_region(cxlrd, cxled->mode, + cxlr = __create_region(cxlrd, mode, atomic_read(&cxlrd->region_id)); } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY); @@ -2996,9 +3033,9 @@ static int cxl_region_probe(struct device *dev) return rc; switch (cxlr->mode) { - case CXL_DECODER_PMEM: + case CXL_REGION_PMEM: return devm_cxl_add_pmem_region(cxlr); - case CXL_DECODER_RAM: + case CXL_REGION_RAM: /* * The region can not be manged by CXL if any portion of * it is already online as 'System RAM' @@ -3010,8 +3047,8 @@ static int cxl_region_probe(struct device *dev) return 0; return devm_cxl_add_dax_region(cxlr); default: - dev_dbg(&cxlr->dev, "unsupported region mode: %d\n", - cxlr->mode); + dev_dbg(&cxlr->dev, "unsupported region mode: %s\n", + cxl_region_mode_name(cxlr->mode)); return -ENXIO; } } diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 003feebab79b..9a0cce1e6fca 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -383,6 +383,27 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode) return "mixed"; } +enum cxl_region_mode { + CXL_REGION_NONE, + CXL_REGION_RAM, + CXL_REGION_PMEM, + CXL_REGION_MIXED, +}; + +static inline const char *cxl_region_mode_name(enum cxl_region_mode mode) +{ + static const char * const names[] = { + [CXL_REGION_NONE] = "none", + [CXL_REGION_RAM] = "ram", + [CXL_REGION_PMEM] = "pmem", + [CXL_REGION_MIXED] = "mixed", + }; + + if (mode >= CXL_REGION_NONE && mode <= CXL_REGION_MIXED) + return names[mode]; + return "mixed"; +} + /* * Track whether this decoder is reserved for region autodiscovery, or * free for userspace provisioning. @@ -511,7 +532,8 @@ struct cxl_region_params { * struct cxl_region - CXL region * @dev: This region's device * @id: This region's id. Id is globally unique across all regions - * @mode: Endpoint decoder allocation / access mode + * @mode: Region mode which defines which endpoint decoder mode the region is + * compatible with * @type: Endpoint decoder target type * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge @@ -521,7 +543,7 @@ struct cxl_region_params { struct cxl_region { struct device dev; int id; - enum cxl_decoder_mode mode; + enum cxl_region_mode mode; enum cxl_decoder_type type; struct cxl_nvdimm_bridge *cxl_nvb; struct cxl_pmem_region *cxlr_pmem;