Messages in this thread | | | From | "" <> | Subject | RE: [PATCH 1/2] ACPI/AEST: Initial AEST driver | Date | Thu, 9 Dec 2021 08:10:50 +0000 |
| |
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.
| |