[PATCH 1/2] ACPI/AEST: Initial AEST driver

[ishii.shuuichir at fujitsu.com]

Hi, Tyler.

We would like to make a few comments.

> -----Original Message-----
> From: Tyler Baicar <baicar at os.amperecomputing.com>
> Sent: Thursday, November 25, 2021 2:07 AM
> To: patches at amperecomputing.com; abdulhamid at os.amperecomputing.com;
> darren at os.amperecomputing.com; catalin.marinas at arm.com; will at kernel.org;
> maz at kernel.org; james.morse at arm.com; alexandru.elisei at arm.com;
> suzuki.poulose at arm.com; lorenzo.pieralisi at arm.com; guohanjun at huawei.com;
> sudeep.holla at arm.com; rafael at kernel.org; lenb at kernel.org;
> tony.luck at intel.com; bp at alien8.de; mark.rutland at arm.com;
> anshuman.khandual at arm.com; vincenzo.frascino at arm.com;
> tabba at google.com; marcan at marcan.st; keescook at chromium.org;
> jthierry at redhat.com; masahiroy at kernel.org; samitolvanen at google.com;
> john.garry at huawei.com; daniel.lezcano at linaro.org; gor at linux.ibm.com;
> zhangshaokun at hisilicon.com; tmricht at linux.ibm.com; dchinner at redhat.com;
> tglx at linutronix.de; linux-kernel at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org; kvmarm at lists.cs.columbia.edu;
> linux-acpi at vger.kernel.org; linux-edac at vger.kernel.org; Ishii, Shuuichirou/石井
> 周一郎 <ishii.shuuichir at fujitsu.com>; Vineeth.Pillai at microsoft.com
> Cc: Tyler Baicar <baicar at os.amperecomputing.com>
> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
> 
> Add support for parsing the ARM Error Source Table and basic handling of
> errors reported through both memory mapped and system register interfaces.
> 
> Assume system register interfaces are only registered with private
> peripheral interrupts (PPIs); otherwise there is no guarantee the
> core handling the error is the core which took the error and has the
> syndrome info in its system registers.
> 
> Add logging for all detected errors and trigger a kernel panic if there is
> any uncorrected error present.
> 
> Signed-off-by: Tyler Baicar <baicar at os.amperecomputing.com>
> ---

[...]

> +static int __init aest_init_node(struct acpi_aest_hdr *node)
> +{
> +	union acpi_aest_processor_data *proc_data;
> +	union aest_node_spec *node_spec;
> +	struct aest_node_data *data;
> +	int ret;
> +
> +	data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->node_type = node->type;
> +
> +	node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
> node->node_specific_offset);
> +
> +	switch (node->type) {
> +	case ACPI_AEST_PROCESSOR_ERROR_NODE:
> +		memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_processor));
> +		break;
> +	case ACPI_AEST_MEMORY_ERROR_NODE:
> +		memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_memory));
> +		break;
> +	case ACPI_AEST_SMMU_ERROR_NODE:
> +		memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_smmu));
> +		break;
> +	case ACPI_AEST_VENDOR_ERROR_NODE:
> +		memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_vendor));
> +		break;
> +	case ACPI_AEST_GIC_ERROR_NODE:
> +		memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_gic));
> +		break;
> +	default:
> +		kfree(data);
> +		return -EINVAL;
> +	}
> +
> +	if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
> +		proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data,
> node_spec,
> +					 sizeof(acpi_aest_processor));
> +
> +		switch (data->data.processor.resource_type) {
> +		case ACPI_AEST_CACHE_RESOURCE:
> +			memcpy(&data->proc_data, proc_data,
> +			       sizeof(struct acpi_aest_processor_cache));
> +			break;
> +		case ACPI_AEST_TLB_RESOURCE:
> +			memcpy(&data->proc_data, proc_data,
> +			       sizeof(struct acpi_aest_processor_tlb));
> +			break;
> +		case ACPI_AEST_GENERIC_RESOURCE:
> +			memcpy(&data->proc_data, proc_data,
> +			       sizeof(struct acpi_aest_processor_generic));
> +			break;
> +		}
> +	}
> +
> +	ret = aest_init_interface(node, data);
> +	if (ret) {
> +		kfree(data);
> +		return ret;
> +	}
> +
> +	return aest_init_interrupts(node, data);

If aest_init_interrupts() failed, is it necessary to release
the data pointer acquired by kzalloc?

> +}
> +
> +static void aest_count_ppi(struct acpi_aest_hdr *node)
> +{
> +	struct acpi_aest_node_interrupt *interrupt;
> +	int i;
> +
> +	interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
> +				 node->node_interrupt_offset);
> +
> +	for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
> +		if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
> +			num_ppi++;
> +	}
> +}
> +
> +static int aest_starting_cpu(unsigned int cpu)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_ppi; i++)
> +		enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
> +
> +	return 0;
> +}
> +
> +static int aest_dying_cpu(unsigned int cpu)
> +{

Wouldn't it be better to execute disable_percpu_irq(), which is paired
with enable_percpu_irq(), in aest_dying_cpu()?

> +	return 0;
> +}
> +

[...]

Best regards, 
Shuuichirou.