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

[ishii.shuuichir@xxxxxxxxxxx]

Hi, Tyler.

We would like to make a few comments.

> -----Original Message-----
> From: Tyler Baicar <baicar@xxxxxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, November 25, 2021 2:07 AM
> To: patches@xxxxxxxxxxxxxxxxxxx; abdulhamid@xxxxxxxxxxxxxxxxxxxxxx;
> darren@xxxxxxxxxxxxxxxxxxxxxx; catalin.marinas@xxxxxxx; will@xxxxxxxxxx;
> maz@xxxxxxxxxx; james.morse@xxxxxxx; alexandru.elisei@xxxxxxx;
> suzuki.poulose@xxxxxxx; lorenzo.pieralisi@xxxxxxx; guohanjun@xxxxxxxxxx;
> sudeep.holla@xxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx;
> tony.luck@xxxxxxxxx; bp@xxxxxxxxx; mark.rutland@xxxxxxx;
> anshuman.khandual@xxxxxxx; vincenzo.frascino@xxxxxxx;
> tabba@xxxxxxxxxx; marcan@xxxxxxxxx; keescook@xxxxxxxxxxxx;
> jthierry@xxxxxxxxxx; masahiroy@xxxxxxxxxx; samitolvanen@xxxxxxxxxx;
> john.garry@xxxxxxxxxx; daniel.lezcano@xxxxxxxxxx; gor@xxxxxxxxxxxxx;
> zhangshaokun@xxxxxxxxxxxxx; tmricht@xxxxxxxxxxxxx; dchinner@xxxxxxxxxx;
> tglx@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx;
> linux-acpi@xxxxxxxxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx; Ishii, Shuuichirou/石井
> 周一郎 <ishii.shuuichir@xxxxxxxxxxx>; Vineeth.Pillai@xxxxxxxxxxxxx
> Cc: Tyler Baicar <baicar@xxxxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxxx>
> ---

[...]

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