| Message ID | 20241007-dcd-type2-upstream-v4-15-c261ee6eeded@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Ira Weiny |
| Headers | show |
| Series | DCD: Add support for Dynamic Capacity Devices (DCD) | expand |
On Mon, 07 Oct 2024 18:16:21 -0500 Ira Weiny <ira.weiny@intel.com> wrote: > create_pmem_region_store() and create_ram_region_store() are identical > with the exception of the region mode. With the addition of DC region > mode this would end up being 3 copies of the same code. > > Refactor create_pmem_region_store() and create_ram_region_store() to use > a single common function to be used in subsequent DC code. > > Suggested-by: Fan Ni <fan.ni@samsung.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Nice. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Is it worth dragging out cleanup like this to the start of the series so Dave can queue it up as 'good to have whatever' and reduce this set a bit? Jonathan > --- > drivers/cxl/core/region.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index ab00203f285a..2ca6148d108c 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2552,9 +2552,8 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM); > } > > + > +static ssize_t create_pmem_region_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + return create_region_store(dev, buf, len, CXL_REGION_PMEM); > +} > DEVICE_ATTR_RW(create_pmem_region); > > static ssize_t create_ram_region_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t len) > { > - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > - struct cxl_region *cxlr; > - int rc, id; > - > - rc = sscanf(buf, "region%d\n", &id); > - if (rc != 1) > - return -EINVAL; > - > - cxlr = __create_region(cxlrd, CXL_REGION_RAM, id); > - if (IS_ERR(cxlr)) > - return PTR_ERR(cxlr); > - > - return len; > + return create_region_store(dev, buf, len, CXL_REGION_RAM); > } > DEVICE_ATTR_RW(create_ram_region); > >
On Mon, Oct 07, 2024 at 06:16:21PM -0500, Ira Weiny wrote: > create_pmem_region_store() and create_ram_region_store() are identical > with the exception of the region mode. With the addition of DC region > mode this would end up being 3 copies of the same code. > > Refactor create_pmem_region_store() and create_ram_region_store() to use > a single common function to be used in subsequent DC code. > > Suggested-by: Fan Ni <fan.ni@samsung.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- Reviewed-by: Fan Ni <fan.ni@samsung.com> > drivers/cxl/core/region.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index ab00203f285a..2ca6148d108c 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2552,9 +2552,8 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM); > } > > -static ssize_t create_pmem_region_store(struct device *dev, > - struct device_attribute *attr, > - const char *buf, size_t len) > +static ssize_t create_region_store(struct device *dev, const char *buf, > + size_t len, enum cxl_region_mode mode) > { > struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > struct cxl_region *cxlr; > @@ -2564,31 +2563,26 @@ static ssize_t create_pmem_region_store(struct device *dev, > if (rc != 1) > return -EINVAL; > > - cxlr = __create_region(cxlrd, CXL_REGION_PMEM, id); > + cxlr = __create_region(cxlrd, mode, id); > if (IS_ERR(cxlr)) > return PTR_ERR(cxlr); > > return len; > } > + > +static ssize_t create_pmem_region_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + return create_region_store(dev, buf, len, CXL_REGION_PMEM); > +} > DEVICE_ATTR_RW(create_pmem_region); > > static ssize_t create_ram_region_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t len) > { > - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > - struct cxl_region *cxlr; > - int rc, id; > - > - rc = sscanf(buf, "region%d\n", &id); > - if (rc != 1) > - return -EINVAL; > - > - cxlr = __create_region(cxlrd, CXL_REGION_RAM, id); > - if (IS_ERR(cxlr)) > - return PTR_ERR(cxlr); > - > - return len; > + return create_region_store(dev, buf, len, CXL_REGION_RAM); > } > DEVICE_ATTR_RW(create_ram_region); > > > -- > 2.46.0 >
Jonathan Cameron wrote: > On Mon, 07 Oct 2024 18:16:21 -0500 > Ira Weiny <ira.weiny@intel.com> wrote: > > > create_pmem_region_store() and create_ram_region_store() are identical > > with the exception of the region mode. With the addition of DC region > > mode this would end up being 3 copies of the same code. > > > > Refactor create_pmem_region_store() and create_ram_region_store() to use > > a single common function to be used in subsequent DC code. > > > > Suggested-by: Fan Ni <fan.ni@samsung.com> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > Nice. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Is it worth dragging out cleanup like this to the start of the series so > Dave can queue it up as 'good to have whatever' and reduce this set > a bit? The problem was that this patch depended on the region mode change... But that was an easy change. I've moved it forward. Ira
On Mon, Oct 07, 2024 at 06:16:21PM -0500, Ira Weiny wrote: > create_pmem_region_store() and create_ram_region_store() are identical > with the exception of the region mode. With the addition of DC region > mode this would end up being 3 copies of the same code. > > Refactor create_pmem_region_store() and create_ram_region_store() to use > a single common function to be used in subsequent DC code. Reviewed-by: Alison Schofield <alison.schofield@intel.com> --snip
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index ab00203f285a..2ca6148d108c 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2552,9 +2552,8 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM); } -static ssize_t create_pmem_region_store(struct device *dev, - struct device_attribute *attr, - const char *buf, size_t len) +static ssize_t create_region_store(struct device *dev, const char *buf, + size_t len, enum cxl_region_mode mode) { struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); struct cxl_region *cxlr; @@ -2564,31 +2563,26 @@ static ssize_t create_pmem_region_store(struct device *dev, if (rc != 1) return -EINVAL; - cxlr = __create_region(cxlrd, CXL_REGION_PMEM, id); + cxlr = __create_region(cxlrd, mode, id); if (IS_ERR(cxlr)) return PTR_ERR(cxlr); return len; } + +static ssize_t create_pmem_region_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + return create_region_store(dev, buf, len, CXL_REGION_PMEM); +} DEVICE_ATTR_RW(create_pmem_region); static ssize_t create_ram_region_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); - struct cxl_region *cxlr; - int rc, id; - - rc = sscanf(buf, "region%d\n", &id); - if (rc != 1) - return -EINVAL; - - cxlr = __create_region(cxlrd, CXL_REGION_RAM, id); - if (IS_ERR(cxlr)) - return PTR_ERR(cxlr); - - return len; + return create_region_store(dev, buf, len, CXL_REGION_RAM); } DEVICE_ATTR_RW(create_ram_region);
create_pmem_region_store() and create_ram_region_store() are identical with the exception of the region mode. With the addition of DC region mode this would end up being 3 copies of the same code. Refactor create_pmem_region_store() and create_ram_region_store() to use a single common function to be used in subsequent DC code. Suggested-by: Fan Ni <fan.ni@samsung.com> Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- drivers/cxl/core/region.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)