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

[Ruidong Tian]

Hi, Tyler.

I am very interested in your work about AEST.
When do you plan to update the v2 patch series?

Best regards.

在 2022/5/9 21:37, Tyler Baicar 写道:
> Hi Shuuichirou,
>
> I should be able to get a v2 patch series out by the end of the month.
>
> Thanks,
> Tyler
>
> On 4/20/2022 3:54 AM, ishii.shuuichir@xxxxxxxxxxx wrote:
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> > > Sent: Friday, December 17, 2021 8:33 AM
> > > To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@xxxxxxxxxxx>; 'Tyler 
> > > Baicar'
> > > <baicar@xxxxxxxxxxxxxxxxxxxxxx>; 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;
> > > Vineeth.Pillai@xxxxxxxxxxxxx
> > > 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@xxxxxxxxxxx wrote:
> > > > 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?
> > > 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
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm