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

[Jonathan Cameron]

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

> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Tested-by: Daniel Ferguson <danielf@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx>



> 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.

> +}