Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 25 Mar 2024 16:20:10 +0000 | From | Jonathan Cameron <> | Subject | Re: [PATCH 02/26] cxl/core: Separate region mode from decoder mode |
| |
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
| ![\](/images/icornerr.gif) |