| Message ID | 20210610185725.897541-3-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Region Creation | expand |
On Thu, 10 Jun 2021 11:57:23 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > Introduce a verification mechanism for a region. Regions have complex > configuration requirements and it is beneficial to provide a way to > verify the constraints are met before trying to bind. Primarily it adds > ABI to inform userspace of more detailed information about what failed > rather than the limited choices of errno at bind time. > > It's important to point out that a verified region can still fail to > bind, but the first step in binding will be to run the same verification > algorithm. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > -- > > Functionally it might make sense to squash this patch in with other > patches adding attributes. From a discussion standpoint however, it's > nice to have this broken out as I suspect there might be some debate > about it. Hmm. Definitely squash it in later, as this is downright odd at the moment! Is there precedence elsewhere for this interface approach? I can see the advantage of it as it lets us pass through invalid states whilst configuring but it is somewhat unusual. Probably one for linux-api@vger.kernel.org to get more exposure to people who care about this stuff. I suspect there aren't that many people on linux-cxl yet ;) Jonathan > --- > Documentation/ABI/testing/sysfs-bus-cxl | 13 +++++++++++++ > drivers/cxl/region.c | 22 +++++++++++++++++++++- > 2 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index 5bcbefd4ea38..699c8514fd7b 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -146,3 +146,16 @@ Contact: linux-cxl@vger.kernel.org > Description: > Deletes the named region. A region must be unbound from the > region driver before being deleted. > + > +What: /sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/verify > +Date: June, 2021 > +KernelVersion: v5.14 > +Contact: linux-cxl@vger.kernel.org > +Description: Doing 'what' to this file causes to this to happen? You want to state that "Reading this file instructs..." > + Instructs the kernel to verify that the regionX.Y:Z is properly > + configured and provide more detailed information about > + configuration errors. A value of 0 indicates the region is > + properly configured and ready to bind, otherwise a negative > + integer is returned describing the first error found in the > + configuration. A verified region can still fail binding due to > + lack of resources. > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c > index 1f47bc17bd50..ea1ac848c713 100644 > --- a/drivers/cxl/region.c > +++ b/drivers/cxl/region.c > @@ -20,11 +20,31 @@ > * relationship between decoder and region when the region is interleaved. > */ > > -static void cxl_region_release(struct device *dev); > +static ssize_t verify_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + return sysfs_emit(buf, "0"); > +} > + > +static DEVICE_ATTR_RO(verify); > + > +static struct attribute *region_attrs[] = { > + &dev_attr_verify.attr, > + NULL, > +}; > > +static const struct attribute_group region_group = { > + .attrs = region_attrs, > +}; > + > +static const struct attribute_group *region_groups[] = { > + ®ion_group, > +}; > + > +static void cxl_region_release(struct device *dev); > static const struct device_type cxl_region_type = { > .name = "cxl_region", > .release = cxl_region_release, > + .groups = region_groups, > }; > > static struct cxl_region *to_cxl_region(struct device *dev)
On Thu, Jun 10, 2021 at 11:57 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > Introduce a verification mechanism for a region. Regions have complex > configuration requirements and it is beneficial to provide a way to > verify the constraints are met before trying to bind. Primarily it adds > ABI to inform userspace of more detailed information about what failed > rather than the limited choices of errno at bind time. If you want to give userspace more data about an internal process that's a TRACE_EVENT(). > It's important to point out that a verified region can still fail to > bind, but the first step in binding will be to run the same verification > algorithm. I don't see this point of giving userspace a racy / less accurate answer than the bind result.
On 21-06-11 17:59:15, Dan Williams wrote: > On Thu, Jun 10, 2021 at 11:57 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > Introduce a verification mechanism for a region. Regions have complex > > configuration requirements and it is beneficial to provide a way to > > verify the constraints are met before trying to bind. Primarily it adds > > ABI to inform userspace of more detailed information about what failed > > rather than the limited choices of errno at bind time. > > If you want to give userspace more data about an internal process > that's a TRACE_EVENT(). Addressed this in the other email... > > > It's important to point out that a verified region can still fail to > > bind, but the first step in binding will be to run the same verification > > algorithm. > > I don't see this point of giving userspace a racy / less accurate > answer than the bind result. Hopefully I didn't forget to mention that verify() would be an optional call from userspace. Perhaps making it debugfs makes more sense?
diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl index 5bcbefd4ea38..699c8514fd7b 100644 --- a/Documentation/ABI/testing/sysfs-bus-cxl +++ b/Documentation/ABI/testing/sysfs-bus-cxl @@ -146,3 +146,16 @@ Contact: linux-cxl@vger.kernel.org Description: Deletes the named region. A region must be unbound from the region driver before being deleted. + +What: /sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/verify +Date: June, 2021 +KernelVersion: v5.14 +Contact: linux-cxl@vger.kernel.org +Description: + Instructs the kernel to verify that the regionX.Y:Z is properly + configured and provide more detailed information about + configuration errors. A value of 0 indicates the region is + properly configured and ready to bind, otherwise a negative + integer is returned describing the first error found in the + configuration. A verified region can still fail binding due to + lack of resources. diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c index 1f47bc17bd50..ea1ac848c713 100644 --- a/drivers/cxl/region.c +++ b/drivers/cxl/region.c @@ -20,11 +20,31 @@ * relationship between decoder and region when the region is interleaved. */ -static void cxl_region_release(struct device *dev); +static ssize_t verify_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sysfs_emit(buf, "0"); +} + +static DEVICE_ATTR_RO(verify); + +static struct attribute *region_attrs[] = { + &dev_attr_verify.attr, + NULL, +}; +static const struct attribute_group region_group = { + .attrs = region_attrs, +}; + +static const struct attribute_group *region_groups[] = { + ®ion_group, +}; + +static void cxl_region_release(struct device *dev); static const struct device_type cxl_region_type = { .name = "cxl_region", .release = cxl_region_release, + .groups = region_groups, }; static struct cxl_region *to_cxl_region(struct device *dev)
Introduce a verification mechanism for a region. Regions have complex configuration requirements and it is beneficial to provide a way to verify the constraints are met before trying to bind. Primarily it adds ABI to inform userspace of more detailed information about what failed rather than the limited choices of errno at bind time. It's important to point out that a verified region can still fail to bind, but the first step in binding will be to run the same verification algorithm. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> -- Functionally it might make sense to squash this patch in with other patches adding attributes. From a discussion standpoint however, it's nice to have this broken out as I suspect there might be some debate about it. --- Documentation/ABI/testing/sysfs-bus-cxl | 13 +++++++++++++ drivers/cxl/region.c | 22 +++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-)