Received-SPF: pass (google.com: domain of linux-kernel+bounces-175010-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 9 May 2024 14:47:56 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: <shiju.jose@huawei.com>, <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>
CC: <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>, <shiju.jose@huawei.com>
Subject: RE: [RFC PATCH v8 01/10] ras: scrub: Add scrub subsystem
Message-ID: <663d448c2ef3_1c0a1929453@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240419164720.1765-1-shiju.jose@huawei.com>
 <20240419164720.1765-2-shiju.jose@huawei.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240419164720.1765-2-shiju.jose@huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cVVqSmhReWp2UjRoVXI5S29LL0liSGdiZFNNcVd4UFBVaWh2RERYNlo2cVFs?=
 =?utf-8?B?ejA5UWEzaWZIb0FQUzc1ckhDVCt2d3lzTGlraVk1eUVJRTFIRGpTbWJRK3BH?=
 =?utf-8?B?NEdBOWVVTmpmaXlBSWgzWEppMjRtK1ZEWnlhMTRxZVFtSU9wRi8ya0ZITEkx?=
 =?utf-8?B?ZXdoTGVXc1lObXFqeDZhaURhVUVmaUVDenROZHE1NzZGOHFCamxNUXN6cVdE?=
 =?utf-8?B?UVVTRW5wM0poOFJ6VnBFQ3dpOXpqR3JhL3c0VWhtMXBORTltaVZLZWJHS1ll?=
 =?utf-8?B?dnJTSERhRjQ2eDE3bTlXMjVzd1cwUEUwRXdjZnNKMVFJTHZQSEpNQmdlanM5?=
 =?utf-8?B?QmZpTm5lTGN4SFZtRExwVnUyZUdXazJDcGE0VTd5U21PNjVXTm1TeXQ4MFJU?=
 =?utf-8?B?cXFSVmpNdFF3dDlaZkJvSWl5NFo0WWdrVXA3bjNrMmZYRlp2dmhNempkWWl3?=
 =?utf-8?B?SEFwTnVPUGsxdUpkSTc0ajNxM2Q2Vm1DOUJBWEtKNnJFa0NlemdDUU5JR2Y0?=
 =?utf-8?B?TWtWYm0zNGFJY2lnWXNCbkVVSVhseXVkZFE4NlhsamNYUDlVU1RDTGJNakpq?=
 =?utf-8?B?SW9VT0dRMmE0M29UTkh2OEhUQWRRRUlOTnFWVWVqRFJYaURmcFJCRExYUitR?=
 =?utf-8?B?ejhFZ1ZsSitMelMranpPWlZWU3hrdHJxaDdGNi8vdmVmMHNDam1HVHN5QVls?=
 =?utf-8?B?aElZak1hVnR6cDVyUDA5Zk1VQk5ORE4wWWdrQUcyN3BiSENTWnBnN1pzUGto?=
 =?utf-8?B?MFlGMlE1YTdFRUxGNUZVcCtrQUswUzZxRjc4V25LMk12RjdFWGhQMmxIaWpa?=
 =?utf-8?B?TkhrRTlYMW5wMVU4MFUxTWYvbmVRSTBUTTBWRnlUczlrdnl4a01SNHkrWmRy?=
 =?utf-8?B?NTBpMUVZUmpDZzI1OTljK2RjUXpFSFZIdmg3Mk9rZ3VIcGNPVHUrRWFZaHBP?=
 =?utf-8?B?aHFmaFR5SC9pOHVidllkMElPWjAwRGxnVnNMelZEQjBYS09mMnNCQXp2WjE4?=
 =?utf-8?B?SGhxcFhmUk0wUDJDYXJCTURUb2IxVmJ2ZWRROUtKeWZhaVAwdlplL1V6Tzc3?=
 =?utf-8?B?WkI2U1NDaGVUdDN2bTMvQzlyNEtxYXJrazcwRWtqZFdweEt3RmJmQmJoem1T?=
 =?utf-8?B?MWdLK21zY2d4RSthWkUrcXdYekt1NllQYTFJTHAzam4rT2NVMUZYTUFjQ3lP?=
 =?utf-8?B?QitHTGdsb1NkTndHV3gxWVVJYnVKQTYrclZ6cnZMb25HMzM4Ymw4ak1PZVh5?=
 =?utf-8?B?cEVZcGhBZUJSUEVZYjRUUUk2L0VERkdRbENQUStmTng3Ui83T1Zic2lTVUlR?=
 =?utf-8?B?RXBvOEIxZWlyNXdXd3VVS2RWTDJYQjBGMW41UGxoK21CTkdxelF5N2ZqREhz?=
 =?utf-8?B?cExFRUl5SXlsazhCTVVwZTdmaTBxck1DZ1dxdGxubnY1S3E2b3FISFROUUFB?=
 =?utf-8?B?ZkVuOURhNjkrU3JCR1IvT21DdHZWM3FkUXFsNTUzNlk0T3BPYjEwaTZyRXRa?=
 =?utf-8?B?UW9rS0toL0ZWNnN0V1NaV05SWVpHcnN5Y3NMaEFYNnVYdWJMU3ljelloc1Zr?=
 =?utf-8?B?Z0ZBMXBtdVptVGtPZHBtUmNzVnhWNG9teUpISlJtc1VJRHVSRWxCTUUwRk9Z?=
 =?utf-8?B?TituM2U4SHZsak5OUStjYm9iUFVBZVJ4YXVBOCtvbDRnTVBjWDRzSlFCcnFi?=
 =?utf-8?B?RmtUNFRKQzlSMTFXSUw3c1M0TjZrT0VVck9KSzdDL3FlT1gvWW9yL1YwU2t5?=
 =?utf-8?B?RytNWGw2QWlnWG9VOXB3OW9aUXhkZE8vbENkMXo0SVlST3h2MmZITWlhRDQw?=
 =?utf-8?B?N3Y3YmZnMjdBcGZDSGxTRzN5N1hPei9kSHlIL2VLQlBCL1ZtTGJPQzdiTnph?=
 =?utf-8?B?cGZpVTI1SnYzd3U0YkYxSE54bWxEQ0JybEJvVnJjdUFORzVRRnZMbEV1MWdM?=
 =?utf-8?B?dnFFUzUxR0ZQeW9sTTl1OGtzZExiZ2t0WHRzV2w1UWVUWUN0RjdmUXNpd0dI?=
 =?utf-8?B?dFYyRGdxNEw3Q2o1ZGpBSWxYQVRqeVhIMkx3VkNnS3pFejlWMTRGT3AvVkFQ?=
 =?utf-8?B?WE4rd2xtaHN1TDZRUTVndHVxeG1NQVhYRm5zMlVKOXp5WFdKVEM4Y3BIMjhr?=
 =?utf-8?B?ZEs1ZVFvWmN5MlRzc3JyTGJ2ei9CV2hsYk1DVVcyK0IwVjdVUTBrZlNWUTl0?=
 =?utf-8?B?alE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4dec9619-3f2e-431d-28ef-08dc7071b2b4
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2024 21:48:01.2850
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oelbHCfZu+kzOKzK4rrW9eKm9BSXY6dgwF7mkkg7W8e2+ZMnGc5bMgcBZ/vnQ7PXw0TiZ6I8DfdTvSul019lLPuaY6hCg/PvDHFHd26TxfQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6504
X-OriginatorOrg: intel.com

shiju.jose@ 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
> 
> 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.

Why create "ras" class versus just a "srcub" class? I am otherwise not
aware of a precedent for class device hierarchy. For example, on my
system there is:

/sys/class/
├── scsi_device
├── scsi_disk
├── scsi_generic
└── scsi_host

..not:

/sys/class/scsi/
├── device
├── disk
├── generic
└── host


> +
> +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
> +		correspond to each scrub device registered with the
> +		scrub subsystem.

I notice there are some visibility rules in the code, but those
expectations are not documented here.

This documentation would also help developers writing new users of
scrub_device_register().

> +
> +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
> +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
> +	  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 */
> +#define SCRUB_ID_PREFIX "ras"
> +#define SCRUB_ID_FORMAT SCRUB_ID_PREFIX "%d"
> +
> +static DEFINE_IDA(scrub_ida);
> +
> +struct scrub_device {
> +	int id;
> +	struct device dev;
> +	const struct scrub_ops *ops;
> +};
> +
> +#define to_scrub_device(d) container_of(d, struct scrub_device, dev)
> +static ssize_t enable_background_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buf, size_t len)
> +{
> +	struct scrub_device *scrub_dev = to_scrub_device(dev);
> +	bool enable;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &enable);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = scrub_dev->ops->set_enabled_bg(dev, enable);
> +	if (ret)
> +		return ret;

It strikes me as somewhat pointless to have such a thin sysfs
implementation whose only job is to call down into a callback to do the
work. Unless there are other consumers of 'struct scrub_ops' outside of
these sysfs files why not just have the low-level drivers register their
corresponding attributes themselves?

Unless the functionality is truly generic just let the low-level driver
be responsible for conforming to the sysfs ABI expectations, and, for
example, each register their own "enable_background" attribute if they
support that semantic.

So scrub_device_register() would grow a 'const struct attribute_group
**groups' argument, or something along those lines.