Messages in this thread | | | Date | Fri, 5 Apr 2024 13:56:08 -0700 | From | Ira Weiny <> | Subject | Re: [PATCH 06/26] cxl/port: Add Dynamic Capacity mode support to endpoint decoders |
| |
Jonathan Cameron wrote: > On Sun, 24 Mar 2024 16:18:09 -0700 > ira.weiny@intel.com wrote: > > > From: Navneet Singh <navneet.singh@intel.com> > >
[snip]
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > > index fff2581b8033..8b3efaf6563c 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-cxl > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > > @@ -316,23 +316,24 @@ Description: > > > > > > What: /sys/bus/cxl/devices/decoderX.Y/mode > > -Date: May, 2022 > > -KernelVersion: v6.0 > > +Date: May, 2022, June 2024 > > +KernelVersion: v6.0, v6.10 (dcY) > > Contact: linux-cxl@vger.kernel.org > > Description: > > (RW) When a CXL decoder is of devtype "cxl_decoder_endpoint" it > > translates from a host physical address range, to a device local > > address range. Device-local address ranges are further split > > - into a 'ram' (volatile memory) range and 'pmem' (persistent > > - memory) range. The 'mode' attribute emits one of 'ram', 'pmem', > > - 'mixed', or 'none'. The 'mixed' indication is for error cases > > - when a decoder straddles the volatile/persistent partition > > - boundary, and 'none' indicates the decoder is not actively > > - decoding, or no DPA allocation policy has been set. > > + into a 'ram' (volatile memory) range, 'pmem' (persistent > > + memory) range, or Dynamic Capacity (DC) range. The 'mode' > > + attribute emits one of 'ram', 'pmem', 'dcY', 'mixed', or > > + 'none'. The 'mixed' indication is for error cases when a > > + decoder straddles the volatile/persistent partition boundary, > > I love corners. What happen if no persistent and decoder straddles > volatile / dc0? Would only happen if the bios was having fun I think..
Yep same issue with a wonky bios... I did not change this text. Only got reformatted. But it is worth changing to:
.. The 'mixed' indication is for error cases when a decoder straddles partition boundaries, ...
> > > + and 'none' indicates the decoder is not actively decoding, or > > + no DPA allocation policy has been set. > > > > 'mode' can be written, when the decoder is in the 'disabled' > > - state, with either 'ram' or 'pmem' to set the boundaries for the > > - next allocation. > > + state, with 'ram', 'pmem', or 'dcY' to set the boundaries for > > + the next allocation. > > > > > > What: /sys/bus/cxl/devices/decoderX.Y/dpa_resource > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > > index 66b8419fd0c3..e22b6f4f7145 100644 > > --- a/drivers/cxl/core/hdm.c > > +++ b/drivers/cxl/core/hdm.c > > @@ -255,6 +255,14 @@ static void devm_cxl_dpa_release(struct cxl_endpoint_decoder *cxled) > > __cxl_dpa_release(cxled); > > } > > > > +static int dc_mode_to_region_index(enum cxl_decoder_mode mode) > > +{ > > + if (mode < CXL_DECODER_DC0 || CXL_DECODER_DC7 < mode) > > + return -EINVAL; > > + > > + return mode - CXL_DECODER_DC0; > > +} > > + > > static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, > > resource_size_t base, resource_size_t len, > > resource_size_t skipped) > > @@ -411,6 +419,7 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled, > > struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > struct device *dev = &cxled->cxld.dev; > > + int rc; > > > > guard(rwsem_write)(&cxl_dpa_rwsem); > > if (cxled->cxld.flags & CXL_DECODER_F_ENABLE) > > @@ -433,6 +442,16 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled, > > return -ENXIO; > > } > > break; > > + case CXL_DECODER_DC0 ... CXL_DECODER_DC7: > > + rc = dc_mode_to_region_index(mode); > > + if (rc < 0) > > + return rc; > > Can't fail, so you could not bother checking.. Seems very unlikely > that function will gain other error cases in the future.
Sure, done.
> > > + > > + if (resource_size(&cxlds->dc_res[rc]) == 0) { > > + dev_dbg(dev, "no available dynamic capacity\n"); > > + return -ENXIO; > > + } > > + break; > > default: > > dev_dbg(dev, "unsupported mode: %d\n", mode); > > return -EINVAL; > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > index e59d9d37aa65..80c0651794eb 100644 > > --- a/drivers/cxl/core/port.c > > +++ b/drivers/cxl/core/port.c > > @@ -208,6 +208,22 @@ static ssize_t mode_store(struct device *dev, struct device_attribute *attr, > > mode = CXL_DECODER_PMEM; > > else if (sysfs_streq(buf, "ram")) > > mode = CXL_DECODER_RAM; > > + else if (sysfs_streq(buf, "dc0")) > > + mode = CXL_DECODER_DC0; > > + else if (sysfs_streq(buf, "dc1")) > > + mode = CXL_DECODER_DC1; > > + else if (sysfs_streq(buf, "dc2")) > > + mode = CXL_DECODER_DC2; > > + else if (sysfs_streq(buf, "dc3")) > > + mode = CXL_DECODER_DC3; > > + else if (sysfs_streq(buf, "dc4")) > > + mode = CXL_DECODER_DC4; > > + else if (sysfs_streq(buf, "dc5")) > > + mode = CXL_DECODER_DC5; > > + else if (sysfs_streq(buf, "dc6")) > > + mode = CXL_DECODER_DC6; > > + else if (sysfs_streq(buf, "dc7")) > > + mode = CXL_DECODER_DC7; > > Fully agree with the comment that a string + enum table and search > is probably appropriate here. >
Ok, done.
Ira
| |