lkml.org 
[lkml]   [2021]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/2] ACPI/AEST: Initial AEST driver
Date
Hi, Tyler.

We would like to make a few comments.

> -----Original Message-----
> From: Tyler Baicar <baicar@os.amperecomputing.com>
> Sent: Thursday, November 25, 2021 2:07 AM
> To: patches@amperecomputing.com; abdulhamid@os.amperecomputing.com;
> darren@os.amperecomputing.com; catalin.marinas@arm.com; will@kernel.org;
> maz@kernel.org; james.morse@arm.com; alexandru.elisei@arm.com;
> suzuki.poulose@arm.com; lorenzo.pieralisi@arm.com; guohanjun@huawei.com;
> sudeep.holla@arm.com; rafael@kernel.org; lenb@kernel.org;
> tony.luck@intel.com; bp@alien8.de; mark.rutland@arm.com;
> anshuman.khandual@arm.com; vincenzo.frascino@arm.com;
> tabba@google.com; marcan@marcan.st; keescook@chromium.org;
> jthierry@redhat.com; masahiroy@kernel.org; samitolvanen@google.com;
> john.garry@huawei.com; daniel.lezcano@linaro.org; gor@linux.ibm.com;
> zhangshaokun@hisilicon.com; tmricht@linux.ibm.com; dchinner@redhat.com;
> tglx@linutronix.de; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> linux-acpi@vger.kernel.org; linux-edac@vger.kernel.org; Ishii, Shuuichirou/石井
> 周一郎 <ishii.shuuichir@fujitsu.com>; Vineeth.Pillai@microsoft.com
> Cc: Tyler Baicar <baicar@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@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.

\
 
 \ /
  Last update: 2021-12-09 09:18    [W:0.241 / U:0.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site