| Message ID | 20240205193218.1657243-4-dave.jiang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Fix memdev qos_class sysfs attributes | expand |
Dave Jiang wrote: > Current implementation exports only to > /sys/bus/cxl/devices/.../memN/qos_class. With both ram and pmem exposed, > the second registered sysfs attribute is rejected as duplicate. It's not > possible to create qos_class under the dev_groups via the driver due to > the ram and pmem sysfs sub-directories already created by the device sysfs > groups. Move the ram and pmem qos_class to the device sysfs groups and add > a call to sysfs_update() after the perf data are validated so the > qos_class can be visible. The end results should be > /sys/bus/cxl/devices/.../memN/ram/qos_class and > /sys/bus/cxl/devices/.../memN/pmem/qos_class. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > v4: > - Replace open code with sysfs_update_groups() helper. (Jonathan) > --- > drivers/cxl/core/cdat.c | 1 + > drivers/cxl/core/memdev.c | 66 +++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/mem.c | 36 --------------------- > 4 files changed, 69 insertions(+), 36 deletions(-) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index ecbd209ca70a..f5bebc7e7ccf 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c > @@ -382,6 +382,7 @@ void cxl_endpoint_parse_cdat(struct cxl_port *port) > > cxl_memdev_set_qos_class(cxlds, dsmas_xa); > cxl_qos_class_verify(cxlmd); > + cxl_memdev_update_attribute_groups(cxlmd); > } > EXPORT_SYMBOL_NS_GPL(cxl_endpoint_parse_cdat, CXL); > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index dae8802ecdb0..c81f2cd4bcc6 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -447,13 +447,41 @@ static struct attribute *cxl_memdev_attributes[] = { > NULL, > }; > > +static ssize_t pmem_qos_class_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > + > + return sysfs_emit(buf, "%d\n", mds->pmem_perf.qos_class); > +} > + > +static struct device_attribute dev_attr_pmem_qos_class = > + __ATTR(qos_class, 0444, pmem_qos_class_show, NULL); > + > static struct attribute *cxl_memdev_pmem_attributes[] = { > &dev_attr_pmem_size.attr, > + &dev_attr_pmem_qos_class.attr, > NULL, > }; > > +static ssize_t ram_qos_class_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > + > + return sysfs_emit(buf, "%d\n", mds->ram_perf.qos_class); > +} > + > +static struct device_attribute dev_attr_ram_qos_class = > + __ATTR(qos_class, 0444, ram_qos_class_show, NULL); > + > static struct attribute *cxl_memdev_ram_attributes[] = { > &dev_attr_ram_size.attr, > + &dev_attr_ram_qos_class.attr, > NULL, > }; > > @@ -477,14 +505,42 @@ static struct attribute_group cxl_memdev_attribute_group = { > .is_visible = cxl_memdev_visible, > }; > > +static umode_t cxl_ram_visible(struct kobject *kobj, struct attribute *a, int n) > +{ > + struct device *dev = kobj_to_dev(kobj); > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); > + > + if (a == &dev_attr_ram_qos_class.attr) > + if (mds->ram_perf.qos_class == CXL_QOS_CLASS_INVALID) > + return 0; > + > + return a->mode; > +} > + > static struct attribute_group cxl_memdev_ram_attribute_group = { > .name = "ram", > .attrs = cxl_memdev_ram_attributes, > + .is_visible = cxl_ram_visible, > }; > > +static umode_t cxl_pmem_visible(struct kobject *kobj, struct attribute *a, int n) > +{ > + struct device *dev = kobj_to_dev(kobj); > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); > + > + if (a == &dev_attr_pmem_qos_class.attr) > + if (mds->pmem_perf.qos_class == CXL_QOS_CLASS_INVALID) > + return 0; > + > + return a->mode; > +} > + > static struct attribute_group cxl_memdev_pmem_attribute_group = { > .name = "pmem", > .attrs = cxl_memdev_pmem_attributes, > + .is_visible = cxl_pmem_visible, > }; > > static umode_t cxl_memdev_security_visible(struct kobject *kobj, > @@ -519,6 +575,16 @@ static const struct attribute_group *cxl_memdev_attribute_groups[] = { > NULL, > }; > > +void cxl_memdev_update_attribute_groups(struct cxl_memdev *cxlmd) > +{ > + int rc; > + > + rc = sysfs_update_groups(&cxlmd->dev.kobj, cxl_memdev_attribute_groups); > + if (rc) > + dev_dbg(&cxlmd->dev, "Unable to update memdev attribute group.\n"); > +} > +EXPORT_SYMBOL_NS_GPL(cxl_memdev_update_attribute_groups, CXL); So I appreciate that Jonathan pointed out this helper as a replacement for the hand coded loop over all the attribute groups, but I don't understand why all the groups need to be revisited when the groups to update are known? It is also a red-flag to handle the result of a __must_check helper with a silent dev_dbg() statement. Given this qos-class support is optional, sysfs_update_groups(), with its violent "tear it all down on failure" semantic, is too heavyweight. The failure can not really be handled from the endpoint port code because the side-effect tears down all groups even the ones registered before the endpoint port driver loads. Instead, just do something like this: diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index c81f2cd4bcc6..d4e259f3a7e9 100644 --- a/drivers/cxl/core/memdev.c +++ b/drivers/cxl/core/memdev.c @@ -575,15 +575,12 @@ static const struct attribute_group *cxl_memdev_attribute_groups[] = { NULL, }; -void cxl_memdev_update_attribute_groups(struct cxl_memdev *cxlmd) +void cxl_memdev_update_perf(struct cxl_memdev *cxlmd) { - int rc; - - rc = sysfs_update_groups(&cxlmd->dev.kobj, cxl_memdev_attribute_groups); - if (rc) - dev_dbg(&cxlmd->dev, "Unable to update memdev attribute group.\n"); + sysfs_update_group(&cxlmd->dev.kobj, &cxl_memdev_ram_attribute_group); + sysfs_update_group(&cxlmd->dev.kobj, &cxl_memdev_pmem_attribute_group); } -EXPORT_SYMBOL_NS_GPL(cxl_memdev_update_attribute_groups, CXL); +EXPORT_SYMBOL_NS_GPL(cxl_memdev_update_perf, CXL); static const struct device_type cxl_memdev_type = { .name = "cxl_memdev", ...where failures are truly ignored.
On 2/5/24 1:38 PM, Dan Williams wrote: > Dave Jiang wrote: >> Current implementation exports only to >> /sys/bus/cxl/devices/.../memN/qos_class. With both ram and pmem exposed, >> the second registered sysfs attribute is rejected as duplicate. It's not >> possible to create qos_class under the dev_groups via the driver due to >> the ram and pmem sysfs sub-directories already created by the device sysfs >> groups. Move the ram and pmem qos_class to the device sysfs groups and add >> a call to sysfs_update() after the perf data are validated so the >> qos_class can be visible. The end results should be >> /sys/bus/cxl/devices/.../memN/ram/qos_class and >> /sys/bus/cxl/devices/.../memN/pmem/qos_class. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> v4: >> - Replace open code with sysfs_update_groups() helper. (Jonathan) >> --- >> drivers/cxl/core/cdat.c | 1 + >> drivers/cxl/core/memdev.c | 66 +++++++++++++++++++++++++++++++++++++++ >> drivers/cxl/cxl.h | 2 ++ >> drivers/cxl/mem.c | 36 --------------------- >> 4 files changed, 69 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c >> index ecbd209ca70a..f5bebc7e7ccf 100644 >> --- a/drivers/cxl/core/cdat.c >> +++ b/drivers/cxl/core/cdat.c >> @@ -382,6 +382,7 @@ void cxl_endpoint_parse_cdat(struct cxl_port *port) >> >> cxl_memdev_set_qos_class(cxlds, dsmas_xa); >> cxl_qos_class_verify(cxlmd); >> + cxl_memdev_update_attribute_groups(cxlmd); >> } >> EXPORT_SYMBOL_NS_GPL(cxl_endpoint_parse_cdat, CXL); >> >> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c >> index dae8802ecdb0..c81f2cd4bcc6 100644 >> --- a/drivers/cxl/core/memdev.c >> +++ b/drivers/cxl/core/memdev.c >> @@ -447,13 +447,41 @@ static struct attribute *cxl_memdev_attributes[] = { >> NULL, >> }; >> >> +static ssize_t pmem_qos_class_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); >> + struct cxl_dev_state *cxlds = cxlmd->cxlds; >> + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); >> + >> + return sysfs_emit(buf, "%d\n", mds->pmem_perf.qos_class); >> +} >> + >> +static struct device_attribute dev_attr_pmem_qos_class = >> + __ATTR(qos_class, 0444, pmem_qos_class_show, NULL); >> + >> static struct attribute *cxl_memdev_pmem_attributes[] = { >> &dev_attr_pmem_size.attr, >> + &dev_attr_pmem_qos_class.attr, >> NULL, >> }; >> >> +static ssize_t ram_qos_class_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); >> + struct cxl_dev_state *cxlds = cxlmd->cxlds; >> + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); >> + >> + return sysfs_emit(buf, "%d\n", mds->ram_perf.qos_class); >> +} >> + >> +static struct device_attribute dev_attr_ram_qos_class = >> + __ATTR(qos_class, 0444, ram_qos_class_show, NULL); >> + >> static struct attribute *cxl_memdev_ram_attributes[] = { >> &dev_attr_ram_size.attr, >> + &dev_attr_ram_qos_class.attr, >> NULL, >> }; >> >> @@ -477,14 +505,42 @@ static struct attribute_group cxl_memdev_attribute_group = { >> .is_visible = cxl_memdev_visible, >> }; >> >> +static umode_t cxl_ram_visible(struct kobject *kobj, struct attribute *a, int n) >> +{ >> + struct device *dev = kobj_to_dev(kobj); >> + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); >> + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); >> + >> + if (a == &dev_attr_ram_qos_class.attr) >> + if (mds->ram_perf.qos_class == CXL_QOS_CLASS_INVALID) >> + return 0; >> + >> + return a->mode; >> +} >> + >> static struct attribute_group cxl_memdev_ram_attribute_group = { >> .name = "ram", >> .attrs = cxl_memdev_ram_attributes, >> + .is_visible = cxl_ram_visible, >> }; >> >> +static umode_t cxl_pmem_visible(struct kobject *kobj, struct attribute *a, int n) >> +{ >> + struct device *dev = kobj_to_dev(kobj); >> + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); >> + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); >> + >> + if (a == &dev_attr_pmem_qos_class.attr) >> + if (mds->pmem_perf.qos_class == CXL_QOS_CLASS_INVALID) >> + return 0; >> + >> + return a->mode; >> +} >> + >> static struct attribute_group cxl_memdev_pmem_attribute_group = { >> .name = "pmem", >> .attrs = cxl_memdev_pmem_attributes, >> + .is_visible = cxl_pmem_visible, >> }; >> >> static umode_t cxl_memdev_security_visible(struct kobject *kobj, >> @@ -519,6 +575,16 @@ static const struct attribute_group *cxl_memdev_attribute_groups[] = { >> NULL, >> }; >> >> +void cxl_memdev_update_attribute_groups(struct cxl_memdev *cxlmd) >> +{ >> + int rc; >> + >> + rc = sysfs_update_groups(&cxlmd->dev.kobj, cxl_memdev_attribute_groups); >> + if (rc) >> + dev_dbg(&cxlmd->dev, "Unable to update memdev attribute group.\n"); >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_memdev_update_attribute_groups, CXL); > > So I appreciate that Jonathan pointed out this helper as a replacement > for the hand coded loop over all the attribute groups, but I don't > understand why all the groups need to be revisited when the groups to > update are known? It is also a red-flag to handle the result of a > __must_check helper with a silent dev_dbg() statement. For some reason I had it stuck in my head that you wanted a caller that updated all the groups. > > Given this qos-class support is optional, sysfs_update_groups(), with > its violent "tear it all down on failure" semantic, is too heavyweight. > The failure can not really be handled from the endpoint port code > because the side-effect tears down all groups even the ones registered > before the endpoint port driver loads. > > Instead, just do something like this: > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index c81f2cd4bcc6..d4e259f3a7e9 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -575,15 +575,12 @@ static const struct attribute_group *cxl_memdev_attribute_groups[] = { > NULL, > }; > > -void cxl_memdev_update_attribute_groups(struct cxl_memdev *cxlmd) > +void cxl_memdev_update_perf(struct cxl_memdev *cxlmd) > { > - int rc; > - > - rc = sysfs_update_groups(&cxlmd->dev.kobj, cxl_memdev_attribute_groups); > - if (rc) > - dev_dbg(&cxlmd->dev, "Unable to update memdev attribute group.\n"); > + sysfs_update_group(&cxlmd->dev.kobj, &cxl_memdev_ram_attribute_group); > + sysfs_update_group(&cxlmd->dev.kobj, &cxl_memdev_pmem_attribute_group); > } > -EXPORT_SYMBOL_NS_GPL(cxl_memdev_update_attribute_groups, CXL); > +EXPORT_SYMBOL_NS_GPL(cxl_memdev_update_perf, CXL); > > static const struct device_type cxl_memdev_type = { > .name = "cxl_memdev", > > ...where failures are truly ignored.
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c index ecbd209ca70a..f5bebc7e7ccf 100644 --- a/drivers/cxl/core/cdat.c +++ b/drivers/cxl/core/cdat.c @@ -382,6 +382,7 @@ void cxl_endpoint_parse_cdat(struct cxl_port *port) cxl_memdev_set_qos_class(cxlds, dsmas_xa); cxl_qos_class_verify(cxlmd); + cxl_memdev_update_attribute_groups(cxlmd); } EXPORT_SYMBOL_NS_GPL(cxl_endpoint_parse_cdat, CXL); diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index dae8802ecdb0..c81f2cd4bcc6 100644 --- a/drivers/cxl/core/memdev.c +++ b/drivers/cxl/core/memdev.c @@ -447,13 +447,41 @@ static struct attribute *cxl_memdev_attributes[] = { NULL, }; +static ssize_t pmem_qos_class_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); + struct cxl_dev_state *cxlds = cxlmd->cxlds; + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); + + return sysfs_emit(buf, "%d\n", mds->pmem_perf.qos_class); +} + +static struct device_attribute dev_attr_pmem_qos_class = + __ATTR(qos_class, 0444, pmem_qos_class_show, NULL); + static struct attribute *cxl_memdev_pmem_attributes[] = { &dev_attr_pmem_size.attr, + &dev_attr_pmem_qos_class.attr, NULL, }; +static ssize_t ram_qos_class_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); + struct cxl_dev_state *cxlds = cxlmd->cxlds; + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); + + return sysfs_emit(buf, "%d\n", mds->ram_perf.qos_class); +} + +static struct device_attribute dev_attr_ram_qos_class = + __ATTR(qos_class, 0444, ram_qos_class_show, NULL); + static struct attribute *cxl_memdev_ram_attributes[] = { &dev_attr_ram_size.attr, + &dev_attr_ram_qos_class.attr, NULL, }; @@ -477,14 +505,42 @@ static struct attribute_group cxl_memdev_attribute_group = { .is_visible = cxl_memdev_visible, }; +static umode_t cxl_ram_visible(struct kobject *kobj, struct attribute *a, int n) +{ + struct device *dev = kobj_to_dev(kobj); + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); + + if (a == &dev_attr_ram_qos_class.attr) + if (mds->ram_perf.qos_class == CXL_QOS_CLASS_INVALID) + return 0; + + return a->mode; +} + static struct attribute_group cxl_memdev_ram_attribute_group = { .name = "ram", .attrs = cxl_memdev_ram_attributes, + .is_visible = cxl_ram_visible, }; +static umode_t cxl_pmem_visible(struct kobject *kobj, struct attribute *a, int n) +{ + struct device *dev = kobj_to_dev(kobj); + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); + + if (a == &dev_attr_pmem_qos_class.attr) + if (mds->pmem_perf.qos_class == CXL_QOS_CLASS_INVALID) + return 0; + + return a->mode; +} + static struct attribute_group cxl_memdev_pmem_attribute_group = { .name = "pmem", .attrs = cxl_memdev_pmem_attributes, + .is_visible = cxl_pmem_visible, }; static umode_t cxl_memdev_security_visible(struct kobject *kobj, @@ -519,6 +575,16 @@ static const struct attribute_group *cxl_memdev_attribute_groups[] = { NULL, }; +void cxl_memdev_update_attribute_groups(struct cxl_memdev *cxlmd) +{ + int rc; + + rc = sysfs_update_groups(&cxlmd->dev.kobj, cxl_memdev_attribute_groups); + if (rc) + dev_dbg(&cxlmd->dev, "Unable to update memdev attribute group.\n"); +} +EXPORT_SYMBOL_NS_GPL(cxl_memdev_update_attribute_groups, CXL); + static const struct device_type cxl_memdev_type = { .name = "cxl_memdev", .release = cxl_memdev_release, diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index b6017c0c57b4..1f630b30d845 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -880,6 +880,8 @@ void cxl_switch_parse_cdat(struct cxl_port *port); int cxl_endpoint_get_perf_coordinates(struct cxl_port *port, struct access_coordinate *coord); +void cxl_memdev_update_attribute_groups(struct cxl_memdev *cxlmd); + /* * Unit test builds overrides this to __weak, find the 'strong' version * of these symbols in tools/testing/cxl/. diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index 547f5a145fc5..0c79d9ce877c 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -215,32 +215,6 @@ static ssize_t trigger_poison_list_store(struct device *dev, } static DEVICE_ATTR_WO(trigger_poison_list); -static ssize_t ram_qos_class_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct cxl_memdev *cxlmd = to_cxl_memdev(dev); - struct cxl_dev_state *cxlds = cxlmd->cxlds; - struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); - - return sysfs_emit(buf, "%d\n", mds->ram_perf.qos_class); -} - -static struct device_attribute dev_attr_ram_qos_class = - __ATTR(qos_class, 0444, ram_qos_class_show, NULL); - -static ssize_t pmem_qos_class_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct cxl_memdev *cxlmd = to_cxl_memdev(dev); - struct cxl_dev_state *cxlds = cxlmd->cxlds; - struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); - - return sysfs_emit(buf, "%d\n", mds->pmem_perf.qos_class); -} - -static struct device_attribute dev_attr_pmem_qos_class = - __ATTR(qos_class, 0444, pmem_qos_class_show, NULL); - static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n) { struct device *dev = kobj_to_dev(kobj); @@ -252,21 +226,11 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n) mds->poison.enabled_cmds)) return 0; - if (a == &dev_attr_pmem_qos_class.attr) - if (mds->pmem_perf.qos_class == CXL_QOS_CLASS_INVALID) - return 0; - - if (a == &dev_attr_ram_qos_class.attr) - if (mds->ram_perf.qos_class == CXL_QOS_CLASS_INVALID) - return 0; - return a->mode; } static struct attribute *cxl_mem_attrs[] = { &dev_attr_trigger_poison_list.attr, - &dev_attr_ram_qos_class.attr, - &dev_attr_pmem_qos_class.attr, NULL };
Current implementation exports only to /sys/bus/cxl/devices/.../memN/qos_class. With both ram and pmem exposed, the second registered sysfs attribute is rejected as duplicate. It's not possible to create qos_class under the dev_groups via the driver due to the ram and pmem sysfs sub-directories already created by the device sysfs groups. Move the ram and pmem qos_class to the device sysfs groups and add a call to sysfs_update() after the perf data are validated so the qos_class can be visible. The end results should be /sys/bus/cxl/devices/.../memN/ram/qos_class and /sys/bus/cxl/devices/.../memN/pmem/qos_class. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v4: - Replace open code with sysfs_update_groups() helper. (Jonathan) --- drivers/cxl/core/cdat.c | 1 + drivers/cxl/core/memdev.c | 66 +++++++++++++++++++++++++++++++++++++++ drivers/cxl/cxl.h | 2 ++ drivers/cxl/mem.c | 36 --------------------- 4 files changed, 69 insertions(+), 36 deletions(-)