From: Borislav Petkov <bp@alien8.de>
To: shiju.jose@huawei.com
Cc: linux-cxl@vger.kernel.org, linux-acpi@vger.kernel.org,
linux-mm@kvack.org, dan.j.williams@intel.com, dave@stgolabs.net,
jonathan.cameron@huawei.com, dave.jiang@intel.com,
alison.schofield@intel.com, vishal.l.verma@intel.com,
ira.weiny@intel.com, linux-edac@vger.kernel.org,
linux-kernel@vger.kernel.org, david@redhat.com,
Vilas.Sridharan@amd.com, leo.duran@amd.com,
Yazen.Ghannam@amd.com, rientjes@google.com, jiaqiyan@google.com,
tony.luck@intel.com, Jon.Grimm@amd.com,
dave.hansen@linux.intel.com, rafael@kernel.org, lenb@kernel.org,
naoya.horiguchi@nec.com, james.morse@arm.com,
jthoughton@google.com, somasundaram.a@hpe.com,
erdemaktas@google.com, pgonda@google.com, duenwen@google.com,
mike.malvestuto@intel.com, gthelen@google.com,
wschwartz@amperecomputing.com, dferguson@amperecomputing.com,
wbs@os.amperecomputing.com, nifan.cxl@gmail.com,
tanxiaofei@huawei.com, prime.zeng@hisilicon.com,
kangkang.shen@futurewei.com, wanghuiqiang@huawei.com,
linuxarm@huawei.com
Subject: Re: [RFC PATCH v8 01/10] ras: scrub: Add scrub subsystem
Date: Thu, 25 Apr 2024 12:15:42 +0200 [thread overview]
Message-ID: <20240425101542.GAZiotThrq7bOE9Ieb@fat_crate.local> (raw)
In-Reply-To: <20240419164720.1765-2-shiju.jose@huawei.com>
On Sat, Apr 20, 2024 at 12:47:10AM +0800, shiju.jose@huawei.com wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
>
> Add scrub subsystem supports configuring the memory scrubbers
> in the system. The scrub subsystem provides the interface for
> registering the scrub devices. The scrub control attributes
> are provided to the user in /sys/class/ras/rasX/scrub
>
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
> .../ABI/testing/sysfs-class-scrub-configure | 47 +++
> drivers/ras/Kconfig | 7 +
> drivers/ras/Makefile | 1 +
> drivers/ras/memory_scrub.c | 271 ++++++++++++++++++
> include/linux/memory_scrub.h | 37 +++
> 5 files changed, 363 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-scrub-configure
> create mode 100755 drivers/ras/memory_scrub.c
> create mode 100755 include/linux/memory_scrub.h
ERROR: modpost: missing MODULE_LICENSE() in drivers/ras/memory_scrub.o
make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
make[1]: *** [/mnt/kernel/kernel/2nd/linux/Makefile:1871: modpost] Error 2
make: *** [Makefile:240: __sub-make] Error 2
Each patch of yours needs to build.
> diff --git a/Documentation/ABI/testing/sysfs-class-scrub-configure b/Documentation/ABI/testing/sysfs-class-scrub-configure
> new file mode 100644
> index 000000000000..3ed77dbb00ad
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-scrub-configure
> @@ -0,0 +1,47 @@
> +What: /sys/class/ras/
> +Date: March 2024
> +KernelVersion: 6.9
> +Contact: linux-kernel@vger.kernel.org
> +Description:
> + The ras/ class subdirectory belongs to the
> + common ras features such as scrub subsystem.
> +
> +What: /sys/class/ras/rasX/scrub/
> +Date: March 2024
> +KernelVersion: 6.9
> +Contact: linux-kernel@vger.kernel.org
> +Description:
> + The /sys/class/ras/ras{0,1,2,3,...}/scrub directories
You have different scrubbers.
I'd prefer if you put their names in here instead and do this structure:
/sys/class/ras/scrub/cxl-patrol
/ars
/cxl-ecs
/acpi-ras2
and so on.
Unless the idea is for those devices to have multiple RAS-specific
functionality than just scrubbing. Then you want to do
/sys/class/ras/cxl/scrub
/other_function
/sys/class/ras/ars/scrub
/...
You get the idea.
> + correspond to each scrub device registered with the
> + scrub subsystem.
> +
> +What: /sys/class/ras/rasX/scrub/name
> +Date: March 2024
> +KernelVersion: 6.9
> +Contact: linux-kernel@vger.kernel.org
> +Description:
> + (RO) name of the memory scrubber
> +
> +What: /sys/class/ras/rasX/scrub/enable_background
> +Date: March 2024
> +KernelVersion: 6.9
> +Contact: linux-kernel@vger.kernel.org
> +Description:
> + (RW) Enable/Disable background(patrol) scrubbing if supported.
> +
> +What: /sys/class/ras/rasX/scrub/rate_available
That's dumping a range so I guess it should be called probably
"possible_rates" or so, so that it is clear what it means.
If some scrubbers support only a discrete set of rate values, then
"possible_rates" fits too if you dump them as a list of values.
> +Date: March 2024
> +KernelVersion: 6.9
> +Contact: linux-kernel@vger.kernel.org
> +Description:
> + (RO) Supported range for the scrub rate by the scrubber.
> + The scrub rate represents in hours.
> +
> +What: /sys/class/ras/rasX/scrub/rate
> +Date: March 2024
> +KernelVersion: 6.9
> +Contact: linux-kernel@vger.kernel.org
> +Description:
> + (RW) The scrub rate specified and it must be with in the
> + supported range by the scrubber.
> + The scrub rate represents in hours.
> diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig
> index fc4f4bb94a4c..181701479564 100644
> --- a/drivers/ras/Kconfig
> +++ b/drivers/ras/Kconfig
> @@ -46,4 +46,11 @@ config RAS_FMPM
> Memory will be retired during boot time and run time depending on
> platform-specific policies.
>
> +config SCRUB
> + tristate "Memory scrub driver"
> + help
> + This option selects the memory scrub subsystem, supports
s/This option selects/Enable/
> + configuring the parameters of underlying scrubbers in the
> + system for the DRAM memories.
> +
> endif
> diff --git a/drivers/ras/Makefile b/drivers/ras/Makefile
> index 11f95d59d397..89bcf0d84355 100644
> --- a/drivers/ras/Makefile
> +++ b/drivers/ras/Makefile
> @@ -2,6 +2,7 @@
> obj-$(CONFIG_RAS) += ras.o
> obj-$(CONFIG_DEBUG_FS) += debugfs.o
> obj-$(CONFIG_RAS_CEC) += cec.o
> +obj-$(CONFIG_SCRUB) += memory_scrub.o
>
> obj-$(CONFIG_RAS_FMPM) += amd/fmpm.o
> obj-y += amd/atl/
> diff --git a/drivers/ras/memory_scrub.c b/drivers/ras/memory_scrub.c
> new file mode 100755
> index 000000000000..7e995380ec3a
> --- /dev/null
> +++ b/drivers/ras/memory_scrub.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Memory scrub subsystem supports configuring the registered
> + * memory scrubbers.
> + *
> + * Copyright (c) 2024 HiSilicon Limited.
> + */
> +
> +#define pr_fmt(fmt) "MEM SCRUB: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/kfifo.h>
> +#include <linux/memory_scrub.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +/* memory scrubber config definitions */
No need for that comment.
> +static ssize_t rate_available_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct scrub_device *scrub_dev = to_scrub_device(dev);
> + u64 min_sr, max_sr;
> + int ret;
> +
> + ret = scrub_dev->ops->rate_avail_range(dev, &min_sr, &max_sr);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "0x%llx-0x%llx\n", min_sr, max_sr);
> +}
This glue driver will need to store the min and max scrub rates on init
and rate_store() will have to verify the newly supplied rate is within
that range before writing it.
Not the user, nor the underlying hw driver.
> +
> +DEVICE_ATTR_RW(enable_background);
> +DEVICE_ATTR_RO(name);
> +DEVICE_ATTR_RW(rate);
> +DEVICE_ATTR_RO(rate_available);
static
> +
> +static struct attribute *scrub_attrs[] = {
> + &dev_attr_enable_background.attr,
> + &dev_attr_name.attr,
> + &dev_attr_rate.attr,
> + &dev_attr_rate_available.attr,
> + NULL
> +};
> +
> +static umode_t scrub_attr_visible(struct kobject *kobj,
> + struct attribute *a, int attr_id)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct scrub_device *scrub_dev = to_scrub_device(dev);
> + const struct scrub_ops *ops = scrub_dev->ops;
> +
> + if (a == &dev_attr_enable_background.attr) {
> + if (ops->set_enabled_bg && ops->get_enabled_bg)
> + return a->mode;
> + if (ops->get_enabled_bg)
> + return 0444;
> + return 0;
> + }
> + if (a == &dev_attr_name.attr)
> + return ops->get_name ? a->mode : 0;
> + if (a == &dev_attr_rate_available.attr)
> + return ops->rate_avail_range ? a->mode : 0;
> + if (a == &dev_attr_rate.attr) { /* Write only makes little sense */
> + if (ops->rate_read && ops->rate_write)
> + return a->mode;
> + if (ops->rate_read)
> + return 0444;
> + return 0;
> + }
All of that stuff's permissions should be root-only.
> +
> + return 0;
> +}
> +
> +static const struct attribute_group scrub_attr_group = {
> + .name = "scrub",
> + .attrs = scrub_attrs,
> + .is_visible = scrub_attr_visible,
> +};
> +
> +static const struct attribute_group *scrub_attr_groups[] = {
> + &scrub_attr_group,
> + NULL
> +};
> +
> +static void scrub_dev_release(struct device *dev)
> +{
> + struct scrub_device *scrub_dev = to_scrub_device(dev);
> +
> + ida_free(&scrub_ida, scrub_dev->id);
> + kfree(scrub_dev);
> +}
> +
> +static struct class scrub_class = {
> + .name = "ras",
> + .dev_groups = scrub_attr_groups,
> + .dev_release = scrub_dev_release,
> +};
> +
> +static struct device *
> +scrub_device_register(struct device *parent, void *drvdata,
> + const struct scrub_ops *ops)
> +{
> + struct scrub_device *scrub_dev;
> + struct device *hdev;
> + int err;
> +
> + scrub_dev = kzalloc(sizeof(*scrub_dev), GFP_KERNEL);
> + if (!scrub_dev)
> + return ERR_PTR(-ENOMEM);
> + hdev = &scrub_dev->dev;
> +
> + scrub_dev->id = ida_alloc(&scrub_ida, GFP_KERNEL);
What's that silly thing for?
> + if (scrub_dev->id < 0) {
> + kfree(scrub_dev);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + scrub_dev->ops = ops;
> + hdev->class = &scrub_class;
> + hdev->parent = parent;
> + dev_set_drvdata(hdev, drvdata);
> + dev_set_name(hdev, SCRUB_ID_FORMAT, scrub_dev->id);
> + err = device_register(hdev);
> + if (err) {
> + put_device(hdev);
> + return ERR_PTR(err);
> + }
> +
> + return hdev;
> +}
> +
> +static void devm_scrub_release(void *dev)
> +{
> + device_unregister(dev);
> +}
> +
> +/**
> + * devm_scrub_device_register - register scrubber device
> + * @dev: the parent device
> + * @drvdata: driver data to attach to the scrub device
> + * @ops: pointer to scrub_ops structure (optional)
> + *
> + * Returns the pointer to the new device on success, ERR_PTR() otherwise.
> + * The new device would be automatically unregistered with the parent device.
> + */
> +struct device *
> +devm_scrub_device_register(struct device *dev, void *drvdata,
> + const struct scrub_ops *ops)
> +{
> + struct device *hdev;
> + int ret;
> +
> + if (!dev)
> + return ERR_PTR(-EINVAL);
> +
> + hdev = scrub_device_register(dev, drvdata, ops);
> + if (IS_ERR(hdev))
> + return hdev;
> +
> + ret = devm_add_action_or_reset(dev, devm_scrub_release, hdev);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return hdev;
> +}
> +EXPORT_SYMBOL_GPL(devm_scrub_device_register);
> +
> +static int __init memory_scrub_control_init(void)
> +{
> + return class_register(&scrub_class);
> +}
> +subsys_initcall(memory_scrub_control_init);
You can't just blindly register this thing without checking whether
there are even any hw scrubber devices on the system.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
next prev parent reply other threads:[~2024-04-25 10:16 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-19 16:47 [RFC PATCH v8 00/10] ras: scrub: introduce subsystem + CXL/ACPI-RAS2 drivers shiju.jose
2024-04-19 16:47 ` [RFC PATCH v8 01/10] ras: scrub: Add scrub subsystem shiju.jose
2024-04-24 20:25 ` fan
2024-04-25 10:38 ` Shiju Jose
2024-04-25 10:15 ` Borislav Petkov [this message]
2024-04-25 18:11 ` Shiju Jose
2024-05-06 10:30 ` Borislav Petkov
2024-05-08 16:59 ` Shiju Jose
2024-05-08 17:20 ` Borislav Petkov
2024-05-08 17:44 ` Shiju Jose
2024-05-08 19:25 ` Borislav Petkov
2024-05-09 9:19 ` Jonathan Cameron
2024-05-09 15:52 ` Borislav Petkov
2024-05-09 20:03 ` Borislav Petkov
2024-05-09 21:21 ` Dan Williams
2024-05-09 21:51 ` Borislav Petkov
2024-05-09 22:59 ` Dan Williams
2024-05-10 9:25 ` Borislav Petkov
2024-05-10 17:13 ` Dan Williams
2024-05-11 10:17 ` Borislav Petkov
2024-05-17 11:15 ` Jonathan Cameron
2024-05-17 11:44 ` Jonathan Cameron
2024-05-10 13:31 ` Jonathan Cameron
2024-05-09 21:47 ` Dan Williams
2024-05-10 9:03 ` Jonathan Cameron
2024-04-19 16:47 ` [RFC PATCH v8 02/10] cxl/mbox: Add GET_SUPPORTED_FEATURES mailbox command shiju.jose
2024-04-19 16:47 ` [RFC PATCH v8 03/10] cxl/mbox: Add GET_FEATURE " shiju.jose
2024-04-24 23:19 ` fan
2024-04-25 10:38 ` Shiju Jose
2024-04-19 16:47 ` [RFC PATCH v8 04/10] cxl/mbox: Add SET_FEATURE " shiju.jose
2024-04-25 17:26 ` fan
2024-04-19 16:47 ` [RFC PATCH v8 05/10] cxl/memscrub: Add CXL device patrol scrub control feature shiju.jose
2024-04-26 23:56 ` fan
2024-04-29 11:20 ` Shiju Jose
2024-04-29 12:21 ` Jonathan Cameron
2024-05-10 0:26 ` Dan Williams
2024-05-10 11:23 ` Jonathan Cameron
2024-04-19 16:47 ` [RFC PATCH v8 06/10] ACPICA: Add __free() based cleanup function for acpi_put_table shiju.jose
2024-04-19 18:06 ` Jonathan Cameron
2024-04-19 16:47 ` [RFC PATCH v8 07/10] platform: Add __free() based cleanup function for platform_device_put shiju.jose
2024-04-19 16:47 ` [RFC PATCH v8 08/10] ACPI:RAS2: Add ACPI RAS2 driver shiju.jose
2024-04-19 16:47 ` [RFC PATCH v8 09/10] ras: scrub: Add scrub control attributes for ACPI RAS2 shiju.jose
2024-04-19 16:47 ` [RFC PATCH v8 10/10] ras: scrub: ACPI RAS2: Add memory ACPI RAS2 driver shiju.jose
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240425101542.GAZiotThrq7bOE9Ieb@fat_crate.local \
--to=bp@alien8.de \
--cc=Jon.Grimm@amd.com \
--cc=Vilas.Sridharan@amd.com \
--cc=Yazen.Ghannam@amd.com \
--cc=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=david@redhat.com \
--cc=dferguson@amperecomputing.com \
--cc=duenwen@google.com \
--cc=erdemaktas@google.com \
--cc=gthelen@google.com \
--cc=ira.weiny@intel.com \
--cc=james.morse@arm.com \
--cc=jiaqiyan@google.com \
--cc=jonathan.cameron@huawei.com \
--cc=jthoughton@google.com \
--cc=kangkang.shen@futurewei.com \
--cc=lenb@kernel.org \
--cc=leo.duran@amd.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxarm@huawei.com \
--cc=mike.malvestuto@intel.com \
--cc=naoya.horiguchi@nec.com \
--cc=nifan.cxl@gmail.com \
--cc=pgonda@google.com \
--cc=prime.zeng@hisilicon.com \
--cc=rafael@kernel.org \
--cc=rientjes@google.com \
--cc=shiju.jose@huawei.com \
--cc=somasundaram.a@hpe.com \
--cc=tanxiaofei@huawei.com \
--cc=tony.luck@intel.com \
--cc=vishal.l.verma@intel.com \
--cc=wanghuiqiang@huawei.com \
--cc=wbs@os.amperecomputing.com \
--cc=wschwartz@amperecomputing.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).