linux-kernel - Re: [PATCH v2 3/3] ras: mem: Add memory ACPI RAS2 driver

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: <20250306173214.0000204e@huawei.com>
Date: Thu, 6 Mar 2025 17:32:14 +0800
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <shiju.jose@...wei.com>
CC: <linux-edac@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
	<bp@...en8.de>, <tony.luck@...el.com>, <rafael@...nel.org>,
	<lenb@...nel.org>, <mchehab@...nel.org>, <leo.duran@....com>,
	<Yazen.Ghannam@....com>, <linux-cxl@...r.kernel.org>,
	<dan.j.williams@...el.com>, <dave@...olabs.net>, <dave.jiang@...el.com>,
	<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
	<ira.weiny@...el.com>, <david@...hat.com>, <Vilas.Sridharan@....com>,
	<linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>, <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>, <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: [PATCH v2 3/3] ras: mem: Add memory ACPI RAS2 driver

On Wed, 5 Mar 2025 18:02:24 +0000
<shiju.jose@...wei.com> wrote:

> From: Shiju Jose <shiju.jose@...wei.com>
> 
> Memory ACPI RAS2 auxiliary driver binds to the auxiliary device
> add by the ACPI RAS2 table parser.
> 
> Driver uses a PCC subspace for communicating with the ACPI compliant
> platform.
> 
> Device with ACPI RAS2 scrub feature registers with EDAC device driver,
> which retrieves the scrub descriptor from EDAC scrub and exposes
> the scrub control attributes for RAS2 scrub instance to userspace in
> /sys/bus/edac/devices/acpi_ras_mem0/scrubX/.
> 
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Tested-by: Daniel Ferguson <danielf@...amperecomputing.com>
> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>



> diff --git a/Documentation/edac/scrub.rst b/Documentation/edac/scrub.rst
> index daab929cdba1..fc8dcbd13f91 100644
> --- a/Documentation/edac/scrub.rst
> +++ b/Documentation/edac/scrub.rst
> @@ -264,3 +264,76 @@ Sysfs files are documented in

...

> +1.2.4. Program background scrubbing for RAS2 device to repeat in every 21600 seconds (quarter of a day).

wrap to 80 chars.  I think that is fine for titles in sphinx.

> +
> +# echo 21600 > /sys/bus/edac/devices/acpi_ras_mem0/scrub0/current_cycle_duration
> +
> +1.2.5. Start 'background scrubbing'.
> +
> +# echo 1 > /sys/bus/edac/devices/acpi_ras_mem0/scrub0/enable_background



> diff --git a/drivers/ras/acpi_ras2.c b/drivers/ras/acpi_ras2.c
> new file mode 100644
> index 000000000000..2f9317aa7b81
> --- /dev/null
> +++ b/drivers/ras/acpi_ras2.c
> @@ -0,0 +1,391 @@


> +struct acpi_ras2_ps_shared_mem {
> +	struct acpi_ras2_shmem common;
> +	struct acpi_ras2_patrol_scrub_param params;
> +};

...

> +static int ras2_update_patrol_scrub_params_cache(struct ras2_mem_ctx *ras2_ctx)
> +{
> +	struct acpi_ras2_ps_shared_mem __iomem *ps_sm =
> +		(void *)ras2_ctx->comm_addr;

Would a container_of() be better here given the type cast is doing
that with the assumption of it being first element of ps_shared_mem.
Same in other places, so maybe a macro.

> +	int ret;
> +
> +	ps_sm->common.set_caps[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
> +	ps_sm->params.cmd = RAS2_GET_PATROL_PARAMETERS;
...


> +
> +static int ras2_hw_scrub_set_enabled_bg(struct device *dev, void *drv_data, bool enable)
> +{
> +	struct ras2_mem_ctx *ras2_ctx = drv_data;
> +	struct acpi_ras2_ps_shared_mem __iomem *ps_sm =
> +		(void *)ras2_ctx->comm_addr;

As above, maybe container_of appropriate as we have
a definition of what we are casting it to that has the thing
we are casting from as first element.

> +	bool running;
> +	int ret;
> +

...

> +
> +static int ras2_probe(struct auxiliary_device *auxdev,
> +		      const struct auxiliary_device_id *id)
> +{
> +	struct ras2_mem_ctx *ras2_ctx = container_of(auxdev, struct ras2_mem_ctx, adev);
> +	struct edac_dev_feature ras_features[RAS2_DEV_NUM_RAS_FEATURES];
Given we only have 1 RAS2 feature I'd be tempted to leave
making this flexible for some future series that adds a second one.
So maybe just have a single feature rather than array of 1.
> +	char scrub_name[RAS2_SCRUB_NAME_LEN];
> +	int num_ras_features = 0;

With change below this isn't needed.

> +	int ret;
> +
> +	if (!ras2_is_patrol_scrub_support(ras2_ctx))
> +		return -EOPNOTSUPP;
> +
> +	ret = ras2_update_patrol_scrub_params_cache(ras2_ctx);
> +	if (ret)
> +		return ret;
> +
> +	snprintf(scrub_name, sizeof(scrub_name), "acpi_ras_mem%d",
> +		 ras2_ctx->id);
> +
> +	ras_features[num_ras_features].ft_type = RAS_FEAT_SCRUB;
> +	ras_features[num_ras_features].instance = ras2_ctx->instance;
> +	ras_features[num_ras_features].scrub_ops = &ras2_scrub_ops;
> +	ras_features[num_ras_features].ctx = ras2_ctx;
> +	num_ras_features++;
As above, can also just assume this is 1 becasue it always is.
> +
> +	return edac_dev_register(&auxdev->dev, scrub_name, NULL,
> +				 num_ras_features, ras_features);
here pass in &ras_feature after making it not be an array.

> +}