RE: [PATCH v15 02/15] EDAC: Add scrub control feature

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Fan Ni <nifan.cxl@gmail.com>
>Sent: 08 November 2024 00:36
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-edac@vger.kernel.org; linux-cxl@vger.kernel.org; linux-
>acpi@vger.kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
>bp@alien8.de; tony.luck@intel.com; rafael@kernel.org; lenb@kernel.org;
>mchehab@kernel.org; dan.j.williams@intel.com; dave@stgolabs.net; Jonathan
>Cameron <jonathan.cameron@huawei.com>; gregkh@linuxfoundation.org;
>sudeep.holla@arm.com; jassisinghbrar@gmail.com; dave.jiang@intel.com;
>alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>david@redhat.com; Vilas.Sridharan@amd.com; leo.duran@amd.com;
>Yazen.Ghannam@amd.com; rientjes@google.com; jiaqiyan@google.com;
>Jon.Grimm@amd.com; dave.hansen@linux.intel.com;
>naoya.horiguchi@nec.com; james.morse@arm.com; jthoughton@google.com;
>somasundaram.a@hpe.com; erdemaktas@google.com; pgonda@google.com;
>duenwen@google.com; gthelen@google.com;
>wschwartz@amperecomputing.com; dferguson@amperecomputing.com;
>wbs@os.amperecomputing.com; nifan.cxl@gmail.com; tanxiaofei
><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Roberto
>Sassu <roberto.sassu@huawei.com>; kangkang.shen@futurewei.com;
>wanghuiqiang <wanghuiqiang@huawei.com>; Linuxarm
><linuxarm@huawei.com>
>Subject: Re: [PATCH v15 02/15] EDAC: Add scrub control feature
>
>On Fri, Nov 01, 2024 at 09:17:20AM +0000, shiju.jose@huawei.com wrote:
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> Add a generic EDAC scrub control to manage memory scrubbers in the system.
>> Devices with a scrub feature register with the EDAC device driver,
>> which retrieves the scrub descriptor from the EDAC scrub driver and
>> exposes the sysfs scrub control attributes for a scrub instance to
>> userspace at /sys/bus/edac/devices/<dev-name>/scrubX/.
>>
>> The common sysfs scrub control interface abstracts the control of
>> arbitrary scrubbing functionality into a common set of functions. The
>> sysfs scrub attribute nodes are only present if the client driver has
>> implemented the corresponding attribute callback function and passed
>> the
>> operations(ops) to the EDAC device driver during registration.
>>
>> 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>
>> ---
>
>Minor comments inline.
>
>>  Documentation/ABI/testing/sysfs-edac-scrub |  74 ++++++++
>>  drivers/edac/Makefile                      |   1 +
>>  drivers/edac/edac_device.c                 |  40 +++-
>>  drivers/edac/scrub.c                       | 209 +++++++++++++++++++++
>>  include/linux/edac.h                       |  34 ++++
>>  5 files changed, 354 insertions(+), 4 deletions(-)  create mode
>> 100644 Documentation/ABI/testing/sysfs-edac-scrub
>>  create mode 100755 drivers/edac/scrub.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-edac-scrub
>> b/Documentation/ABI/testing/sysfs-edac-scrub
>> new file mode 100644
>> index 000000000000..d8d11165ff2a
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-edac-scrub
>
>...
>
>> diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
>> index e9229b5f8afe..cd700a64406e 100644
>> --- a/drivers/edac/edac_device.c
>> +++ b/drivers/edac/edac_device.c
>> @@ -576,6 +576,7 @@ static void edac_dev_release(struct device *dev)
>> {
>>  	struct edac_dev_feat_ctx *ctx = container_of(dev, struct
>> edac_dev_feat_ctx, dev);
>>
>> +	kfree(ctx->scrub);
>>  	kfree(ctx->dev.groups);
>>  	kfree(ctx);
>>  }
>> @@ -609,6 +610,8 @@ int edac_dev_register(struct device *parent, char
>*name,
>>  		      const struct edac_dev_feature *ras_features)  {
>>  	const struct attribute_group **ras_attr_groups;
>> +	int scrub_cnt = 0, scrub_inst = 0;
>> +	struct edac_dev_data *dev_data;
>>  	struct edac_dev_feat_ctx *ctx;
>>  	int attr_gcnt = 0;
>>  	int ret, feat;
>> @@ -619,7 +622,10 @@ int edac_dev_register(struct device *parent, char
>*name,
>>  	/* Double parse to make space for attributes */
>>  	for (feat = 0; feat < num_features; feat++) {
>>  		switch (ras_features[feat].ft_type) {
>> -		/* Add feature specific code */
>> +		case RAS_FEAT_SCRUB:
>> +			attr_gcnt++;
>> +			scrub_cnt++;
>> +			break;
>>  		default:
>>  			return -EINVAL;
>>  		}
>> @@ -635,13 +641,37 @@ int edac_dev_register(struct device *parent, char
>*name,
>>  		goto ctx_free;
>>  	}
>>
>> +	if (scrub_cnt) {
>> +		ctx->scrub = kcalloc(scrub_cnt, sizeof(*ctx->scrub),
>GFP_KERNEL);
>> +		if (!ctx->scrub) {
>> +			ret = -ENOMEM;
>> +			goto groups_free;
>> +		}
>> +	}
>> +
>>  	attr_gcnt = 0;
>
>If we use scrub_cnt the same way as we use attr_gcnt, we do not need
>scrub_inst.

Hi Fan,
Thanks for suggestion. Modified and done the same for EDAC memory repair feature as well. 
>
>Fan
>>  	for (feat = 0; feat < num_features; feat++, ras_features++) {
>>  		switch (ras_features->ft_type) {
>> -		/* Add feature specific code */
[...]
>--
>Fan Ni
>
Thanks,
Shiju