lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240717150807.70c9ef90@foz.lan>
Date: Wed, 17 Jul 2024 15:08:07 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: <shiju.jose@...wei.com>
Cc: <linux-edac@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
 <linux-acpi@...r.kernel.org>, <linux-mm@...ck.org>,
 <linux-kernel@...r.kernel.org>, <bp@...en8.de>, <tony.luck@...el.com>,
 <rafael@...nel.org>, <lenb@...nel.org>, <mchehab@...nel.org>,
 <dan.j.williams@...el.com>, <dave@...olabs.net>,
 <jonathan.cameron@...wei.com>, <dave.jiang@...el.com>,
 <alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
 <ira.weiny@...el.com>, <david@...hat.com>, <Vilas.Sridharan@....com>,
 <leo.duran@....com>, <Yazen.Ghannam@....com>, <rientjes@...gle.com>,
 <jiaqiyan@...gle.com>, <Jon.Grimm@....com>, <dave.hansen@...ux.intel.com>,
 <naoya.horiguchi@....com>, <james.morse@....com>, <jthoughton@...gle.com>,
 <somasundaram.a@....com>, <erdemaktas@...gle.com>, <pgonda@...gle.com>,
 <duenwen@...gle.com>, <mike.malvestuto@...el.com>, <gthelen@...gle.com>,
 <wschwartz@...erecomputing.com>, <dferguson@...erecomputing.com>,
 <wbs@...amperecomputing.com>, <nifan.cxl@...il.com>,
 <tanxiaofei@...wei.com>, <prime.zeng@...ilicon.com>,
 <roberto.sassu@...wei.com>, <kangkang.shen@...urewei.com>,
 <wanghuiqiang@...wei.com>, <linuxarm@...wei.com>
Subject: Re: [RFC PATCH v9 03/11] EDAC: Add EDAC ECS control driver

Em Tue, 16 Jul 2024 16:03:27 +0100
<shiju.jose@...wei.com> escreveu:

> From: Shiju Jose <shiju.jose@...wei.com>
> 
> Add EDAC ECS (Error Check Scrub) control driver supports configuring
> the memory device's ECS feature.
> 
> The Error Check Scrub (ECS) is a feature defined in JEDEC DDR5 SDRAM
> Specification (JESD79-5) and allows the DRAM to internally read, correct
> single-bit errors, and write back corrected data bits to the DRAM array
> while providing transparency to error counts.
> 
> The DDR5 device contains number of memory media FRUs per device. The
> DDR5 ECS feature and thus the ECS control driver supports configuring
> the ECS parameters per FRU.
> 
> The memory devices supports ECS feature register with the EDAC ECS driver

typo:
	supports -> support

> and thus with the generic EDAC RAS feature driver, which adds the sysfs
> ECS control interface. The ECS control attributes are exposed to the
> userspace in /sys/bus/edac/devices/<dev-name>/ecs_fruX/.
> 
> Generic EDAC ECS driver and the common sysfs ECS interface promotes
> unambiguous control from the userspace irrespective of the underlying
> devices, support ECS feature.
> 
> The support for ECS feature is added separately because the DDR5 ECS
> feature's control attributes are dissimilar from those of the scrub
> feature.
> 
> Note: Documentation can be added if necessary.

Please document.

> 
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
> ---
>  drivers/edac/Makefile            |   2 +-
>  drivers/edac/edac_ecs.c          | 396 +++++++++++++++++++++++++++++++
>  drivers/edac/edac_ras_feature.c  |   5 +
>  include/linux/edac_ras_feature.h |  36 +++
>  4 files changed, 438 insertions(+), 1 deletion(-)
>  create mode 100755 drivers/edac/edac_ecs.c
> 
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index de56cbd039eb..c1412c7d3efb 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -10,7 +10,7 @@ obj-$(CONFIG_EDAC)			:= edac_core.o
>  
>  edac_core-y	:= edac_mc.o edac_device.o edac_mc_sysfs.o
>  edac_core-y	+= edac_module.o edac_device_sysfs.o wq.o
> -edac_core-y	+= edac_ras_feature.o edac_scrub.o
> +edac_core-y	+= edac_ras_feature.o edac_scrub.o edac_ecs.o
>  
>  edac_core-$(CONFIG_EDAC_DEBUG)		+= debugfs.o
>  
> diff --git a/drivers/edac/edac_ecs.c b/drivers/edac/edac_ecs.c
> new file mode 100755
> index 000000000000..37dabd053c36
> --- /dev/null
> +++ b/drivers/edac/edac_ecs.c
> @@ -0,0 +1,396 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ECS driver supporting controlling on die error check scrub
> + * (e.g. DDR5 ECS). The common sysfs ECS interface promotes
> + * unambiguous access from the userspace.
> + *
> + * Copyright (c) 2024 HiSilicon Limited.
> + */
> +
> +#define pr_fmt(fmt)     "EDAC ECS: " fmt
> +
> +#include <linux/edac_ras_feature.h>
> +
> +#define EDAC_ECS_FRU_NAME "ecs_fru"
> +
> +enum edac_ecs_attributes {
> +	ecs_log_entry_type,
> +	ecs_log_entry_type_per_dram,
> +	ecs_log_entry_type_per_memory_media,
> +	ecs_mode,
> +	ecs_mode_counts_rows,
> +	ecs_mode_counts_codewords,
> +	ecs_reset,
> +	ecs_name,
> +	ecs_threshold,
> +	ecs_max_attrs
> +};

Please use uppercase for enums.

> +
> +struct edac_ecs_dev_attr {
> +	struct device_attribute dev_attr;
> +	int fru_id;
> +};
> +
> +struct edac_ecs_fru_context {
> +	char name[EDAC_RAS_NAME_LEN];
> +	struct edac_ecs_dev_attr ecs_dev_attr[ecs_max_attrs];
> +	struct attribute *ecs_attrs[ecs_max_attrs + 1];
> +	struct attribute_group group;
> +};
> +
> +struct edac_ecs_context {
> +	u16 num_media_frus;
> +	struct edac_ecs_fru_context *fru_ctxs;
> +};
> +
> +#define to_ecs_dev_attr(_dev_attr)	\
> +	container_of(_dev_attr, struct edac_ecs_dev_attr, dev_attr)
> +
> +static ssize_t log_entry_type_show(struct device *ras_feat_dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct edac_ecs_dev_attr *ecs_dev_attr = to_ecs_dev_attr(attr);
> +	struct edac_ras_feat_ctx *ctx = dev_get_drvdata(ras_feat_dev);
> +	const struct edac_ecs_ops *ops = ctx->ecs.ops;
> +	u64 val;
> +	int ret;
> +
> +	ret = ops->get_log_entry_type(ras_feat_dev->parent, ctx->ecs.private,
> +				      ecs_dev_attr->fru_id, &val);
> +	if (ret)
> +		return ret;

Same notes as patch 2/11 with regards to sysfs documentation/store/show.

Also, it is hard to review this patch without the ABI documentation.

Regards,
Mauro

Thanks,
Mauro

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ