lkml.org 
[lkml]   [2022]   [Apr]   [20]   [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.

When do you plan to post the v2 patch series?
Please let me know if you don't mind.

Best regards.

> -----Original Message-----
> From: Tyler Baicar <baicar@amperemail.onmicrosoft.com>
> Sent: Friday, December 17, 2021 8:33 AM
> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>; 'Tyler Baicar'
> <baicar@os.amperecomputing.com>; 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;
> Vineeth.Pillai@microsoft.com
> Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>
> Hi Shuuichirou,
>
> Thank you for your feedback!
>
> On 12/9/2021 3:10 AM, ishii.shuuichir@fujitsu.com wrote:
> > 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?
> aest_init_interrupts() returns an error if any of the interrupts in the interrupt list
> fails, but it's possible that some interrupts in the list registered successfully. So
> we attempt to keep chugging along in that scenario because some interrupts may
> be enabled and registered with the interface successfully. If any interrupt
> registration fails, there will be a print notifying that there was a failure when
> initializing that node.
> >> +}
> >> +
> >> +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()?
>
> Good point. I will add that in the next version.
>
> Thanks,
>
> Tyler

\
 
 \ /
  Last update: 2022-04-20 09:58    [W:0.154 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site