From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
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>, <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>,
<tanxiaofei@huawei.com>, <prime.zeng@hisilicon.com>,
<kangkang.shen@futurewei.com>, <wanghuiqiang@huawei.com>,
<linuxarm@huawei.com>
Subject: Re: [RFC PATCH v6 06/12] memory: scrub: Add scrub subsystem driver supports configuring memory scrubs in the system
Date: Tue, 20 Feb 2024 12:39:22 +0000 [thread overview]
Message-ID: <20240220123922.00007142@Huawei.com> (raw)
In-Reply-To: <20240215111455.1462-7-shiju.jose@huawei.com>
> From: Shiju Jose <shiju.jose@huawei.com>
>
> Add scrub driver supports configuring the memory scrubs in the system.
> The scrub driver provides the interface for registering the scrub devices
> and supports configuring memory scrubs in the system.
> Driver exposes the sysfs scrub control attributes to the user in
> /sys/class/scrub/scrubX/regionN/
>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
Hi Shiju,
A few minor things inline. Given I reviewed this internally I don't
have that much to add!
Jonathan
> ---
> .../ABI/testing/sysfs-class-scrub-configure | 91 +++++
> drivers/memory/Kconfig | 1 +
> drivers/memory/Makefile | 1 +
> drivers/memory/scrub/Kconfig | 11 +
> drivers/memory/scrub/Makefile | 6 +
> drivers/memory/scrub/memory-scrub.c | 367 ++++++++++++++++++
> include/memory/memory-scrub.h | 78 ++++
> 7 files changed, 555 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-scrub-configure
> create mode 100644 drivers/memory/scrub/Kconfig
> create mode 100644 drivers/memory/scrub/Makefile
> create mode 100755 drivers/memory/scrub/memory-scrub.c
> create mode 100755 include/memory/memory-scrub.h
>
> diff --git a/Documentation/ABI/testing/sysfs-class-scrub-configure b/Documentation/ABI/testing/sysfs-class-scrub-configure
> new file mode 100644
> index 000000000000..d2d422b667cf
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-scrub-configure
> +What: /sys/class/scrub/scrubX/regionN/rate_available
> +Date: January 2024
> +KernelVersion: 6.8
> +Contact: linux-kernel@vger.kernel.org
> +Description:
> + (RO) Supported range for the scrub rate)
> + by the scrubber for a memory region.
> + The unit of the scrub rate vary depends on the scrub.
Not good to have a unit that is dependent on scrub. We need to figure
out how to either define that, or provide an interface to expose it
to userspace and make it a userspace tool problem.
> diff --git a/drivers/memory/scrub/memory-scrub.c b/drivers/memory/scrub/memory-scrub.c
> new file mode 100755
> index 000000000000..a160b7a047e4
> --- /dev/null
> +SCRUB_ATTR_RW(addr_base);
> +SCRUB_ATTR_RW(addr_size);
> +SCRUB_ATTR_RW(enable);
> +SCRUB_ATTR_RW(enable_background_scrub);
> +SCRUB_ATTR_RW(rate);
> +SCRUB_ATTR_RO(rate_available);
> +
> +static struct attribute *scrub_attrs[] = {
> + &dev_attr_addr_base.attr,
> + &dev_attr_addr_size.attr,
> + &dev_attr_enable.attr,
> + &dev_attr_enable_background_scrub.attr,
> + &dev_attr_rate.attr,
> + &dev_attr_rate_available.attr,
> + NULL,
no comma
> +};
> +
> +static struct device *
> +scrub_device_register(struct device *dev, const char *name, void *drvdata,
> + const struct scrub_ops *ops,
> + int region_id,
> + struct attribute_group *attr_group)
> +{
> + 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);
> + if (scrub_dev->id < 0) {
> + kfree(scrub_dev);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + snprintf((char *)scrub_dev->region_name, SCRUB_MAX_SYSFS_ATTR_NAME_LENGTH,
> + "region%d", region_id);
> + if (attr_group) {
I'd like a comment on this. Not immediately obvious what this parameter is to me,
or when we would and wouldn't have one.
> + attr_group->name = (char *)scrub_dev->region_name;
> + scrub_dev->groups[0] = attr_group;
> + scrub_dev->region_id = region_id;
> + } else {
> + scrub_dev->group.name = (char *)scrub_dev->region_name;
In both paths, drop out of if / else
> + scrub_dev->group.attrs = scrub_attrs;
> + scrub_dev->group.is_visible = scrub_attr_visible;
> + scrub_dev->groups[0] = &scrub_dev->group;
> + scrub_dev->ops = ops;
> + scrub_dev->region_id = region_id;
Set in both paths, so drop out of the if / else;
> + }
> +
> + hdev->groups = scrub_dev->groups;
> + hdev->class = &scrub_class;
> + hdev->parent = dev;
> + dev_set_drvdata(hdev, drvdata);
> + dev_set_name(hdev, SCRUB_ID_FORMAT, scrub_dev->id);
> + snprintf(scrub_dev->name, SCRUB_DEV_MAX_NAME_LENGTH, "%s", name);
> + err = device_register(hdev);
> + if (err) {
> + put_device(hdev);
> + return ERR_PTR(err);
> + }
> +
> + return hdev;
> +}
> +
> +static void devm_scrub_release(void *dev)
> +{
> + struct device *hdev = dev;
> +
> + device_unregister(hdev);
Trivial but local variable doesn't really add anything.
deivce_unregister(dev);
is pretty clear on types!
> +}
> diff --git a/include/memory/memory-scrub.h b/include/memory/memory-scrub.h
> new file mode 100755
> index 000000000000..3d7054e98b9a
> --- /dev/null
> +++ b/include/memory/memory-scrub.h
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Memory scrub controller driver support to configure
> + * the controls of the memory scrub and enable.
> + *
> + * Copyright (c) 2023 HiSilicon Limited.
> + */
> +
> +#ifndef __MEMORY_SCRUB_H
> +#define __MEMORY_SCRUB_H
> +
> +#include <linux/types.h>
> +
> +enum scrub_types {
> + scrub_common,
> + scrub_max,
No comma on a terminating entry like this.
> +};
next prev parent reply other threads:[~2024-02-20 12:39 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-15 11:14 [RFC PATCH v6 00/12] cxl: Add support for CXL feature commands, CXL device patrol scrub control and DDR5 ECS control features shiju.jose
2024-02-15 11:14 ` [RFC PATCH v6 01/12] cxl/mbox: Add GET_SUPPORTED_FEATURES mailbox command shiju.jose
2024-02-20 11:02 ` Jonathan Cameron
2024-03-05 23:57 ` fan
2024-02-15 11:14 ` [RFC PATCH v6 02/12] cxl/mbox: Add GET_FEATURE " shiju.jose
2024-02-20 11:14 ` Jonathan Cameron
2024-02-23 12:23 ` Shiju Jose
2024-02-15 11:14 ` [RFC PATCH v6 03/12] cxl/mbox: Add SET_FEATURE " shiju.jose
2024-02-20 11:27 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 04/12] cxl/memscrub: Add CXL device patrol scrub control feature shiju.jose
2024-02-20 11:59 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 05/12] cxl/memscrub: Add CXL device ECS " shiju.jose
2024-02-20 12:17 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 06/12] memory: scrub: Add scrub subsystem driver supports configuring memory scrubs in the system shiju.jose
2024-02-20 12:39 ` Jonathan Cameron [this message]
2024-02-15 11:14 ` [RFC PATCH v6 07/12] cxl/memscrub: Register CXL device patrol scrub with scrub configure driver shiju.jose
2024-02-20 12:43 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 08/12] cxl/memscrub: Register CXL device ECS " shiju.jose
2024-02-20 13:39 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 09/12] ACPI:RASF: Add common library for RASF and RAS2 PCC interfaces shiju.jose
2024-02-20 13:53 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 10/12] ACPICA: ACPI 6.5: Add support for RAS2 table shiju.jose
2024-02-20 13:58 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 11/12] ACPI:RAS2: Add driver for ACPI RAS2 feature table (RAS2) shiju.jose
2024-02-15 11:14 ` [RFC PATCH v6 12/12] memory: RAS2: Add memory RAS2 driver shiju.jose
2024-02-20 16:12 ` Jonathan Cameron
2024-02-22 0:20 ` [RFC PATCH v6 00/12] cxl: Add support for CXL feature commands, CXL device patrol scrub control and DDR5 ECS control features Dan Williams
2024-02-23 12:16 ` Shiju Jose
2024-02-23 19:42 ` Dan Williams
2024-02-26 10:29 ` Jonathan Cameron
2024-02-29 19:51 ` Dan Williams
2024-03-01 14:41 ` Jonathan Cameron
2024-03-05 0:52 ` Jiaqi Yan
2024-02-29 20:41 ` Tony Luck
2024-03-01 13:19 ` Jonathan Cameron
2024-02-28 22:26 ` John Groves
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=20240220123922.00007142@Huawei.com \
--to=jonathan.cameron@huawei.com \
--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=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=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=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).