| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <78c387a7074a4146b34dc853a6798883@huawei.com>
Date: Mon, 16 Sep 2024 09:22:17 +0000
From: Shiju Jose <shiju.jose@...wei.com>
To: Borislav Petkov <bp@...en8.de>
CC: "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
"linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linux-mm@...ck.org" <linux-mm@...ck.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "tony.luck@...el.com" <tony.luck@...el.com>,
"rafael@...nel.org" <rafael@...nel.org>, "lenb@...nel.org" <lenb@...nel.org>,
"mchehab@...nel.org" <mchehab@...nel.org>, "dan.j.williams@...el.com"
<dan.j.williams@...el.com>, "dave@...olabs.net" <dave@...olabs.net>,
"Jonathan Cameron" <jonathan.cameron@...wei.com>, "dave.jiang@...el.com"
<dave.jiang@...el.com>, "alison.schofield@...el.com"
<alison.schofield@...el.com>, "vishal.l.verma@...el.com"
<vishal.l.verma@...el.com>, "ira.weiny@...el.com" <ira.weiny@...el.com>,
"david@...hat.com" <david@...hat.com>, "Vilas.Sridharan@....com"
<Vilas.Sridharan@....com>, "leo.duran@....com" <leo.duran@....com>,
"Yazen.Ghannam@....com" <Yazen.Ghannam@....com>, "rientjes@...gle.com"
<rientjes@...gle.com>, "jiaqiyan@...gle.com" <jiaqiyan@...gle.com>,
"Jon.Grimm@....com" <Jon.Grimm@....com>, "dave.hansen@...ux.intel.com"
<dave.hansen@...ux.intel.com>, "naoya.horiguchi@....com"
<naoya.horiguchi@....com>, "james.morse@....com" <james.morse@....com>,
"jthoughton@...gle.com" <jthoughton@...gle.com>, "somasundaram.a@....com"
<somasundaram.a@....com>, "erdemaktas@...gle.com" <erdemaktas@...gle.com>,
"pgonda@...gle.com" <pgonda@...gle.com>, "duenwen@...gle.com"
<duenwen@...gle.com>, "mike.malvestuto@...el.com"
<mike.malvestuto@...el.com>, "gthelen@...gle.com" <gthelen@...gle.com>,
"wschwartz@...erecomputing.com" <wschwartz@...erecomputing.com>,
"dferguson@...erecomputing.com" <dferguson@...erecomputing.com>,
"wbs@...amperecomputing.com" <wbs@...amperecomputing.com>,
"nifan.cxl@...il.com" <nifan.cxl@...il.com>, "jgroves@...ron.com"
<jgroves@...ron.com>, "vsalve@...ron.com" <vsalve@...ron.com>, tanxiaofei
<tanxiaofei@...wei.com>, "Zengtao (B)" <prime.zeng@...ilicon.com>, "Roberto
Sassu" <roberto.sassu@...wei.com>, "kangkang.shen@...urewei.com"
<kangkang.shen@...urewei.com>, wanghuiqiang <wanghuiqiang@...wei.com>,
Linuxarm <linuxarm@...wei.com>
Subject: RE: [PATCH v12 02/17] EDAC: Add EDAC scrub control driver
Thanks for reviewing.
>-----Original Message-----
>From: Borislav Petkov <bp@...en8.de>
>Sent: 13 September 2024 18:25
>To: Shiju Jose <shiju.jose@...wei.com>
>Cc: linux-edac@...r.kernel.org; linux-cxl@...r.kernel.org; linux-
>acpi@...r.kernel.org; linux-mm@...ck.org; linux-kernel@...r.kernel.org;
>tony.luck@...el.com; rafael@...nel.org; lenb@...nel.org;
>mchehab@...nel.org; dan.j.williams@...el.com; dave@...olabs.net; Jonathan
>Cameron <jonathan.cameron@...wei.com>; dave.jiang@...el.com;
>alison.schofield@...el.com; vishal.l.verma@...el.com; ira.weiny@...el.com;
>david@...hat.com; Vilas.Sridharan@....com; leo.duran@....com;
>Yazen.Ghannam@....com; rientjes@...gle.com; jiaqiyan@...gle.com;
>Jon.Grimm@....com; dave.hansen@...ux.intel.com;
>naoya.horiguchi@....com; james.morse@....com; jthoughton@...gle.com;
>somasundaram.a@....com; erdemaktas@...gle.com; pgonda@...gle.com;
>duenwen@...gle.com; mike.malvestuto@...el.com; gthelen@...gle.com;
>wschwartz@...erecomputing.com; dferguson@...erecomputing.com;
>wbs@...amperecomputing.com; nifan.cxl@...il.com; jgroves@...ron.com;
>vsalve@...ron.com; tanxiaofei <tanxiaofei@...wei.com>; Zengtao (B)
><prime.zeng@...ilicon.com>; Roberto Sassu <roberto.sassu@...wei.com>;
>kangkang.shen@...urewei.com; wanghuiqiang <wanghuiqiang@...wei.com>;
>Linuxarm <linuxarm@...wei.com>
>Subject: Re: [PATCH v12 02/17] EDAC: Add EDAC scrub control driver
>
>On Wed, Sep 11, 2024 at 10:04:31AM +0100, shiju.jose@...wei.com wrote:
>> From: Shiju Jose <shiju.jose@...wei.com>
>>
>> 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@...wei.com>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
>> ---
>> 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@...r.kernel.org
>> +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@huawei.com/
RAS2 scrub, https://lore.kernel.org/linux-mm/20240911090447.751-15-shiju.jose@huawei.com/
>
>> 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
Powered by blists - more mailing lists