> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx
> +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.
> +};
