Thanks for reviewing. >-----Original Message----- >From: Borislav Petkov <bp@xxxxxxxxx> >Sent: 13 September 2024 18:25 >To: Shiju Jose <shiju.jose@xxxxxxxxxx> >Cc: linux-edac@xxxxxxxxxxxxxxx; linux-cxl@xxxxxxxxxxxxxxx; linux- >acpi@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; >tony.luck@xxxxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx; >mchehab@xxxxxxxxxx; dan.j.williams@xxxxxxxxx; dave@xxxxxxxxxxxx; Jonathan >Cameron <jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx; >alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx; >david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx; leo.duran@xxxxxxx; >Yazen.Ghannam@xxxxxxx; rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx; >Jon.Grimm@xxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; >naoya.horiguchi@xxxxxxx; james.morse@xxxxxxx; jthoughton@xxxxxxxxxx; >somasundaram.a@xxxxxxx; erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx; >duenwen@xxxxxxxxxx; mike.malvestuto@xxxxxxxxx; gthelen@xxxxxxxxxx; >wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx; >wbs@xxxxxxxxxxxxxxxxxxxxxx; nifan.cxl@xxxxxxxxx; jgroves@xxxxxxxxxx; >vsalve@xxxxxxxxxx; tanxiaofei <tanxiaofei@xxxxxxxxxx>; Zengtao (B) ><prime.zeng@xxxxxxxxxxxxx>; Roberto Sassu <roberto.sassu@xxxxxxxxxx>; >kangkang.shen@xxxxxxxxxxxxx; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>; >Linuxarm <linuxarm@xxxxxxxxxx> >Subject: Re: [PATCH v12 02/17] EDAC: Add EDAC scrub control driver > >On Wed, Sep 11, 2024 at 10:04:31AM +0100, shiju.jose@xxxxxxxxxx wrote: >> From: Shiju Jose <shiju.jose@xxxxxxxxxx> >> >> Add generic EDAC scrub control driver supports configuring the memory >> scrubbers > >s/supports configuring the/in order to configure/ Will change. > >> in the system. The device with scrub feature, get the scrub descriptor >> from the EDAC scrub and registers with the EDAC RAS feature driver, >> which adds the sysfs scrub control interface. > >That sentence reads wrong. You are right. Will update. > >> The scrub control attributes for a scrub instance are available to >> userspace in /sys/bus/edac/devices/<dev-name>/scrub*/. >> >> Generic EDAC scrub driver and the common sysfs scrub interface >> promotes unambiguous access from the userspace irrespective of the >> underlying scrub devices. > >Huh? > >Do you wanna say something along the lines that the common sysfs scrub >interface abstracts the control of an arbitrary scrubbing functionality into a >common set of functions or so? Sure. Will change. > >> The sysfs scrub attribute nodes would be present only if the client >> driver has implemented the corresponding attribute callback function >> and pass in ops to the EDAC RAS feature driver during registration. >> >> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> >> Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx> >> --- >> Documentation/ABI/testing/sysfs-edac-scrub | 69 ++++ >> drivers/edac/Makefile | 1 + >> drivers/edac/edac_device.c | 6 +- >> drivers/edac/edac_scrub.c | 377 +++++++++++++++++++++ >> include/linux/edac.h | 30 ++ >> 5 files changed, 482 insertions(+), 1 deletion(-) create mode 100644 >> Documentation/ABI/testing/sysfs-edac-scrub >> create mode 100755 drivers/edac/edac_scrub.c >> >> diff --git a/Documentation/ABI/testing/sysfs-edac-scrub >> b/Documentation/ABI/testing/sysfs-edac-scrub >> new file mode 100644 >> index 000000000000..f465cc91423f >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-edac-scrub > >... > >> +What: /sys/bus/edac/devices/<dev- >name>/scrub*/current_cycle_duration >> +Date: Oct 2024 >> +KernelVersion: 6.12 >> +Contact: linux-edac@xxxxxxxxxxxxxxx >> +Description: >> + (RW) The current scrub cycle duration in seconds and must be >> + within the supported range by the memory scrubber. > >So in reading about that interface, where is the user doc explaining how one >should use scrubbers? User doc for scrub feature is part of the patches for CXL patrol scrub, https://lore.kernel.org/linux-mm/20240911090447.751-11-shiju.jose@xxxxxxxxxx/ RAS2 scrub, https://lore.kernel.org/linux-mm/20240911090447.751-15-shiju.jose@xxxxxxxxxx/ > >> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile index >> 4edfb83ffbee..fbf0e39ec678 100644 >> --- a/drivers/edac/Makefile >> +++ b/drivers/edac/Makefile >> @@ -10,6 +10,7 @@ obj-$(CONFIG_EDAC) := edac_core.o >> >> edac_core-y := edac_mc.o edac_device.o edac_mc_sysfs.o >> edac_core-y += edac_module.o edac_device_sysfs.o wq.o >> +edac_core-y += edac_scrub.o > >Just scrub.[co]. The file is already in drivers/edac/. Too many "edac" >strings. :) Ok. Will change the file names. > >> >> edac_core-$(CONFIG_EDAC_DEBUG) += debugfs.o >> >> diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c >> index e4a5d010ea2d..6381896b6424 100644 >> --- a/drivers/edac/edac_device.c >> +++ b/drivers/edac/edac_device.c >> @@ -608,12 +608,16 @@ static int edac_dev_feat_init(struct device *parent, >> const struct edac_dev_feature *ras_feat, >> const struct attribute_group **attr_groups) { >> - int num; >> + int num, ret; >> >> switch (ras_feat->ft_type) { >> case RAS_FEAT_SCRUB: >> dev_data->scrub_ops = ras_feat->scrub_ops; >> dev_data->private = ras_feat->ctx; >> + ret = edac_scrub_get_desc(parent, attr_groups, >> + ras_feat->instance); >> + if (ret) >> + return ret; >> return 1; >> case RAS_FEAT_ECS: >> num = ras_feat->ecs_info.num_media_frus; >> diff --git a/drivers/edac/edac_scrub.c b/drivers/edac/edac_scrub.c new >> file mode 100755 index 000000000000..3f8f37629acf >> --- /dev/null >> +++ b/drivers/edac/edac_scrub.c >> @@ -0,0 +1,377 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Generic EDAC scrub driver supports controlling the memory >> + * scrubbers in the system and the common sysfs scrub interface >> + * promotes unambiguous access from the userspace. >> + * >> + * Copyright (c) 2024 HiSilicon Limited. >> + */ >> + >> +#define pr_fmt(fmt) "EDAC SCRUB: " fmt >> + >> +#include <linux/edac.h> >> + >> +enum edac_scrub_attributes { >> + SCRUB_ADDR_RANGE_BASE, >> + SCRUB_ADDR_RANGE_SIZE, >> + SCRUB_ENABLE_BACKGROUND, >> + SCRUB_ENABLE_ON_DEMAND, >> + SCRUB_MIN_CYCLE_DURATION, >> + SCRUB_MAX_CYCLE_DURATION, >> + SCRUB_CURRENT_CYCLE_DURATION, >> + SCRUB_MAX_ATTRS >> +}; >> + >> +struct edac_scrub_dev_attr { >> + struct device_attribute dev_attr; >> + u8 instance; >> +}; >> + >> +struct edac_scrub_context { >> + char name[EDAC_FEAT_NAME_LEN]; >> + struct edac_scrub_dev_attr scrub_dev_attr[SCRUB_MAX_ATTRS]; >> + struct attribute *scrub_attrs[SCRUB_MAX_ATTRS + 1]; >> + struct attribute_group group; >> +}; >> + >> +#define to_scrub_dev_attr(_dev_attr) \ >> + container_of(_dev_attr, struct edac_scrub_dev_attr, dev_attr) >> + >> +static ssize_t addr_range_base_show(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + u64 base, size; >> + int ret; >> + >> + ret = ops->read_range(ras_feat_dev->parent, ctx->scrub[inst].private, >&base, &size); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "0x%llx\n", base); } >> + >> +static ssize_t addr_range_size_show(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + u64 base, size; >> + int ret; >> + >> + ret = ops->read_range(ras_feat_dev->parent, ctx->scrub[inst].private, >&base, &size); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "0x%llx\n", size); } >> + >> +static ssize_t addr_range_base_store(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) { >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + u64 base, size; >> + int ret; >> + >> + ret = ops->read_range(ras_feat_dev->parent, ctx->scrub[inst].private, >&base, &size); >> + if (ret) >> + return ret; > >> + >> + ret = kstrtou64(buf, 0, &base); >> + if (ret < 0) >> + return ret; >> + >> + ret = ops->write_range(ras_feat_dev->parent, ctx->scrub[inst].private, >base, size); >> + if (ret) >> + return ret; >> + >> + return len; >> +} >> + >> +static ssize_t addr_range_size_store(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + const char *buf, >> + size_t len) >> +{ >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + u64 base, size; >> + int ret; >> + >> + ret = ops->read_range(ras_feat_dev->parent, ctx->scrub[inst].private, >&base, &size); >> + if (ret) >> + return ret; >> + > >Can all that repetitive code be abstracted away in macros pls? > >Below too. Sure. Will do. > >> + ret = kstrtou64(buf, 0, &size); >> + if (ret < 0) >> + return ret; >> + >> + ret = ops->write_range(ras_feat_dev->parent, ctx->scrub[inst].private, >base, size); >> + if (ret) >> + return ret; >> + >> + return len; >> +} >> + >> +static ssize_t enable_background_store(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) { >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + bool enable; >> + int ret; >> + >> + ret = kstrtobool(buf, &enable); >> + if (ret < 0) >> + return ret; >> + >> + ret = ops->set_enabled_bg(ras_feat_dev->parent, ctx- >>scrub[inst].private, enable); >> + if (ret) >> + return ret; >> + >> + return len; >> +} >> + >> +static ssize_t enable_background_show(struct device *ras_feat_dev, >> + struct device_attribute *attr, char *buf) { >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + bool enable; >> + int ret; >> + >> + ret = ops->get_enabled_bg(ras_feat_dev->parent, ctx- >>scrub[inst].private, &enable); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "%d\n", enable); } >> + >> +static ssize_t enable_on_demand_show(struct device *ras_feat_dev, >> + struct device_attribute *attr, char *buf) { >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + bool enable; >> + int ret; >> + >> + ret = ops->get_enabled_od(ras_feat_dev->parent, ctx- >>scrub[inst].private, &enable); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "%d\n", enable); } >> + >> +static ssize_t enable_on_demand_store(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) { >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + bool enable; >> + int ret; >> + >> + ret = kstrtobool(buf, &enable); >> + if (ret < 0) >> + return ret; >> + >> + ret = ops->set_enabled_od(ras_feat_dev->parent, ctx- >>scrub[inst].private, enable); >> + if (ret) >> + return ret; >> + >> + return len; >> +} >> + >> +static ssize_t min_cycle_duration_show(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + u32 val; >> + int ret; >> + >> + ret = ops->min_cycle_read(ras_feat_dev->parent, ctx- >>scrub[inst].private, &val); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "%u\n", val); } >> + >> +static ssize_t max_cycle_duration_show(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + u32 val; >> + int ret; >> + >> + ret = ops->max_cycle_read(ras_feat_dev->parent, ctx- >>scrub[inst].private, &val); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "%u\n", val); } >> + >> +static ssize_t current_cycle_duration_show(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + u32 val; >> + int ret; >> + >> + ret = ops->cycle_duration_read(ras_feat_dev->parent, ctx- >>scrub[inst].private, &val); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "%u\n", val); } >> + >> +static ssize_t current_cycle_duration_store(struct device *ras_feat_dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) { >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + long val; >> + int ret; >> + >> + ret = kstrtol(buf, 0, &val); >> + if (ret < 0) >> + return ret; >> + >> + ret = ops->cycle_duration_write(ras_feat_dev->parent, ctx- >>scrub[inst].private, val); >> + if (ret) >> + return ret; >> + >> + return len; >> +} >> + >> +static umode_t scrub_attr_visible(struct kobject *kobj, >> + struct attribute *a, int attr_id) { >> + struct device *ras_feat_dev = kobj_to_dev(kobj); >> + struct device_attribute *dev_attr = >> + container_of(a, struct device_attribute, attr); > >No silly linebreaks like that pls. Check your whole patchset. Will change. > >> + u8 inst = ((struct edac_scrub_dev_attr *)to_scrub_dev_attr(dev_attr))- >>instance; >> + struct edac_dev_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev); >> + const struct edac_scrub_ops *ops = ctx->scrub[inst].scrub_ops; >> + >> + switch (attr_id) { >> + case SCRUB_ADDR_RANGE_BASE: >> + case SCRUB_ADDR_RANGE_SIZE: >> + if (ops->read_range && ops->write_range) >> + return a->mode; >> + if (ops->read_range) >> + return 0444; > > if (...read_range) { > if (...write_range) > return a->mode; > else > return 0444: > } > break; > >and now put a single "return 0;" at the end of the function. Will do, if increase in lines of code is ok? > >Below too. > >> + return 0; >> + case SCRUB_ENABLE_BACKGROUND: >> + if (ops->get_enabled_bg && ops->set_enabled_bg) >> + return a->mode; >> + if (ops->get_enabled_bg) >> + return 0444; >> + return 0; >> + case SCRUB_ENABLE_ON_DEMAND: >> + if (ops->get_enabled_od && ops->set_enabled_od) >> + return a->mode; >> + if (ops->get_enabled_od) >> + return 0444; >> + return 0; >> + case SCRUB_MIN_CYCLE_DURATION: >> + return ops->min_cycle_read ? a->mode : 0; > > if (ops->min_cycle_read) > return a->mode; > >> + case SCRUB_MAX_CYCLE_DURATION: >> + return ops->max_cycle_read ? a->mode : 0; >> + case SCRUB_CURRENT_CYCLE_DURATION: >> + if (ops->cycle_duration_read && ops->cycle_duration_write) >> + return a->mode; >> + if (ops->cycle_duration_read) >> + return 0444; >> + return 0; >> + default: >> + return 0; >> + } >> +} >> + >> +#define EDAC_SCRUB_ATTR_RO(_name, _instance) \ >> + ((struct edac_scrub_dev_attr) { .dev_attr = __ATTR_RO(_name), \ >> + .instance = _instance }) >> + >> +#define EDAC_SCRUB_ATTR_WO(_name, _instance) \ >> + ((struct edac_scrub_dev_attr) { .dev_attr = __ATTR_WO(_name), \ >> + .instance = _instance }) >> + >> +#define EDAC_SCRUB_ATTR_RW(_name, _instance) \ >> + ((struct edac_scrub_dev_attr) { .dev_attr = __ATTR_RW(_name), \ >> + .instance = _instance }) >> + >> +static int scrub_create_desc(struct device *scrub_dev, >> + const struct attribute_group **attr_groups, >> + u8 instance) >> +{ >> + struct edac_scrub_context *scrub_ctx; >> + struct attribute_group *group; >> + int i; >> + >> + scrub_ctx = devm_kzalloc(scrub_dev, sizeof(*scrub_ctx), GFP_KERNEL); >> + if (!scrub_ctx) >> + return -ENOMEM; >> + >> + group = &scrub_ctx->group; >> + scrub_ctx->scrub_dev_attr[0] = >EDAC_SCRUB_ATTR_RW(addr_range_base, instance); >> + scrub_ctx->scrub_dev_attr[1] = >EDAC_SCRUB_ATTR_RW(addr_range_size, instance); >> + scrub_ctx->scrub_dev_attr[2] = >EDAC_SCRUB_ATTR_RW(enable_background, instance); >> + scrub_ctx->scrub_dev_attr[3] = >EDAC_SCRUB_ATTR_RW(enable_on_demand, instance); >> + scrub_ctx->scrub_dev_attr[4] = >EDAC_SCRUB_ATTR_RO(min_cycle_duration, instance); >> + scrub_ctx->scrub_dev_attr[5] = >EDAC_SCRUB_ATTR_RO(max_cycle_duration, instance); >> + scrub_ctx->scrub_dev_attr[6] = >> +EDAC_SCRUB_ATTR_RW(current_cycle_duration, instance); > >Why use the naked numbers when you have enum edac_scrub_attributes? Sure. > >> + for (i = 0; i < SCRUB_MAX_ATTRS; i++) >> + scrub_ctx->scrub_attrs[i] = >> +&scrub_ctx->scrub_dev_attr[i].dev_attr.attr; >> + >> + sprintf(scrub_ctx->name, "%s%d", "scrub", instance); >> + group->name = scrub_ctx->name; >> + group->attrs = scrub_ctx->scrub_attrs; >> + group->is_visible = scrub_attr_visible; >> + >> + attr_groups[0] = group; >> + >> + return 0; >> +} >> + >> +/** >> + * edac_scrub_get_desc - get EDAC scrub descriptors >> + * @scrub_dev: client device, with scrub support >> + * @attr_groups: pointer to attrribute group container > >+ * @attr_groups: pointer to attrribute group container >Unknown word [attrribute] in comment. >Suggestions: ['attribute', 'attributed', 'attributes', "attribute's", 'attributive', >'tribute'] > >Please introduce a spellchecker into your patch creation workflow. Sure. > >-- >Regards/Gruss, > Boris. > >https://people.kernel.org/tglx/notes-about-netiquette > Thanks, Shiju
