Re: [RFC PATCH v6 06/12] memory: scrub: Add scrub subsystem driver supports configuring memory scrubs in the system

[Jonathan Cameron]

> 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.
> +};