Re: [PATCH v2 2/3] ACPI:RAS2: Add ACPI RAS2 driver

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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

> From: Shiju Jose <shiju.jose@huawei.com>
> 
> Add support for ACPI RAS2 feature table (RAS2) defined in the
> ACPI 6.5 Specification, section 5.2.21.
> Driver defines RAS2 Init, which extracts the RAS2 table and driver
> adds auxiliary device for each memory feature which binds to the
> RAS2 memory driver.
> 
> Driver uses PCC mailbox to communicate with the ACPI HW and the
> driver adds OSPM interfaces to send RAS2 commands.
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Co-developed-by: A Somasundaram <somasundaram.a@hpe.com>
> Signed-off-by: A Somasundaram <somasundaram.a@hpe.com>
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Daniel Ferguson <danielf@os.amperecomputing.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>

Hi Shiju,

I took another look through as it's been a while and I've
pretty much forgotten this code :(

Anyhow, a few minor comments inline.

Thanks,

Jonathan

> diff --git a/drivers/acpi/ras2.c b/drivers/acpi/ras2.c
> new file mode 100755
> index 000000000000..8831a2bd5fab
> --- /dev/null
> +++ b/drivers/acpi/ras2.c




> +static int ras2_register_pcc_channel(struct ras2_mem_ctx *ras2_ctx, int pcc_id)
> +{
> +	struct ras2_pcc_subspace *pcc_subspace;
> +	struct pcc_mbox_chan *pcc_chan;
> +	struct mbox_client *mbox_cl;
> +
> +	if (pcc_id < 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&ras2_pcc_lock);
> +	list_for_each_entry(pcc_subspace, &ras2_pcc_subspaces, elem) {
> +		if (pcc_subspace->pcc_id != pcc_id)
> +			continue;
> +		ras2_ctx->pcc_subspace = pcc_subspace;
> +		pcc_subspace->ref_count++;
> +		mutex_unlock(&ras2_pcc_lock);
> +		return 0;
> +	}
> +	mutex_unlock(&ras2_pcc_lock);
> +
> +	pcc_subspace = kcalloc(1, sizeof(*pcc_subspace), GFP_KERNEL);

if allocating a count of 1, why not kzalloc?

> +	if (!pcc_subspace)
> +		return -ENOMEM;




> +static int acpi_ras2_parse(void)
> +{
> +	struct acpi_ras2_pcc_desc *pcc_desc_list;
> +	int pcc_id;
> +	u8 count = 0;
> +	int rc, i;
> +
> +	if (ras2_tab->header.length  < sizeof(struct acpi_table_ras2)) {

extra space before <

Maybe sizeof(*ras2_tab) is cleaner.

> +		pr_warn(FW_WARN "ACPI RAS2 table present but broken (too short #1)\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!ras2_tab->num_pcc_descs) {
> +		pr_warn(FW_WARN "No PCC descs in ACPI RAS2 table\n");
> +		return -EINVAL;
> +	}
> +
> +	pcc_desc_list = (struct acpi_ras2_pcc_desc *)(ras2_tab + 1);
> +	/* Double scan for the case of only one actual controller */
> +	pcc_id = -1;
> +	count = 0;

Already set above, so no need to do it again.  I'd do it just here.  Can
put it in the loop init though.

> +	for (i = 0; i < ras2_tab->num_pcc_descs; i++, pcc_desc_list++) {
> +		if (pcc_desc_list->feature_type != RAS2_FEAT_TYPE_MEMORY)
> +			continue;
> +		if (pcc_id == -1) {
> +			pcc_id = pcc_desc_list->channel_id;
> +			count++;
> +		}
> +		if (pcc_desc_list->channel_id != pcc_id)
> +			count++;
> +	}
> +
> +	/*
> +	 * Workaround for the client platform with multiple scrub devices
> +	 * but uses single PCC subspace for communication.
> +	 */
> +	if (count == 1) {
> +		/* Add auxiliary device and bind ACPI RAS2 memory driver */
> +		rc = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_id);
> +		if (rc)
> +			return rc;
> +
> +		return 0;
> +	}
> +
> +	pcc_desc_list = (struct acpi_ras2_pcc_desc *)(ras2_tab + 1);
> +	count = 0;

Maybe set in loop init.

> +	for (i = 0; i < ras2_tab->num_pcc_descs; i++, pcc_desc_list++) {
> +		if (pcc_desc_list->feature_type != RAS2_FEAT_TYPE_MEMORY)
> +			continue;
> +		pcc_id = pcc_desc_list->channel_id;
> +		/* Add auxiliary device and bind ACPI RAS2 memory driver */
obvious enough to drop the comment I think.

> +		rc = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_id);
					 pcc_desc_list->channel_id);
and no local variable.

> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +void __init acpi_ras2_init(void)
> +{
> +	acpi_status status;
> +	int rc;
> +
> +	status = acpi_get_table(ACPI_SIG_RAS2, 0,
> +				(struct acpi_table_header **)&ras2_tab);
> +	if (ACPI_FAILURE(status) || !ras2_tab) {
> +		const char *msg = acpi_format_exception(status);
> +
> +		pr_err("Failed to get table, %s\n", msg);

If only going to use it here maybe
		pr_err("Failed to get table, %s\n",
		       acpi_format_exception(status));
and save on the local variable.

> +		return;
> +	}
> +
> +	rc = acpi_ras2_parse();
> +	if (rc) {
> +		acpi_put_table((struct acpi_table_header *)ras2_tab); 
> +		pr_err("Failed to parse RAS2 table\n");
> +	}
> +}
> diff --git a/include/acpi/ras2.h b/include/acpi/ras2.h
> new file mode 100644
> index 000000000000..5b27c1f30096
> --- /dev/null
> +++ b/include/acpi/ras2.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * ACPI RAS2 driver header file
> + *
> + * Copyright (c) 2024-2025 HiSilicon Limited
> + */
> +
> +#ifndef _ACPI_RAS2_H
> +#define _ACPI_RAS2_H
> +
> +#include <linux/acpi.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +#define RAS2_PCC_CMD_COMPLETE	BIT(0)
> +#define RAS2_PCC_CMD_ERROR	BIT(2)
> +
I think these bits are from table 14.11 and
generic to all PCC status registers? Should these
have more generic names rather than ras2 specific ones?

> +/* RAS2 specific PCC commands */
> +#define RAS2_PCC_CMD_EXEC 0x01
Are we mixing commands and field definitions both
with prefix RAS2_PCC_CMD_ ?  That is somewhat
confusing. 

> +
> +#define RAS2_AUX_DEV_NAME "ras2"
> +#define RAS2_MEM_DEV_ID_NAME "acpi_ras2_mem"
> +
I would add a forwards def
struct device;  

whilst it is really unlikely that headers would ever be reorganized
such that auxiliary_bus.h would not include device.h given the embedded
device we shouldn't rely on that here.

> +/* Data structure RAS2 table */
> +struct ras2_mem_ctx {
> +	struct auxiliary_device adev;
> +	/* Lock to provide mutually exclusive access to PCC channel */
> +	struct mutex lock;
> +	struct device *dev;
> +	struct acpi_ras2_shmem __iomem *comm_addr;
> +	void *pcc_subspace;
> +	int id;
> +};
> +
> +#ifdef CONFIG_ACPI_RAS2
> +void __init acpi_ras2_init(void);
> +int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 cmd);
> +#else
> +static inline void acpi_ras2_init(void) { }
> +
> +static inline int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 cmd)

Is this stub ever needed?  To me it seems unlikely
we would have a user that is built without a dependency
on CONFIG_ACPI_RAS2.  This is different from acpi_ras2_init()
which makes much more sense to me.

> +{
> +	return -EOPNOTSUPP;
> +}
> +#endif
> +#endif /* _ACPI_RAS2_H */