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.
