Re: [RFC PATCH] ACPI / amba: Skip creating amba device when associated cpu is not online

[chenxiang (M)]

Hi Suzuki,


在 2022/1/28 19:00, Suzuki K Poulose 写道:
> Hi Xiang
>
> On 07/01/2022 08:41, chenxiang wrote:
> > From: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
> >
> > If not up all the cpus with command line "maxcpus=x", system will be
> > blocked.
> > We find that some amba devices such as ETM devices, are associated with
> > special cpus, and if the cpu is not up, the register of associated 
> > device
> > is not allowed to access. BIOS reports all the ETM device nodes and a
> > amba device is created for every ETM device, so even if one cpu is 
> > not up,
> > the amba device will still be created for the associated device, and 
> > also
> > the register of device (pid and cid) will be accessed when adding amba
> > device which will cause the issue.
> > To fix it, skip creating amba device if it is associated with a cpu 
> > which
> > is not online.
>
> I understand the issue. We do not have an issue at least on DT based 
> platforms with a similar environment (Juno). The key is the power 
> management for the components.
>
> There are two separate issues at play here :
>
> 1) Power management with ACPI. I believe there is a solution in progress
> to address this.
>
> 2) The ETM is in the same power domain as that of the CPU and normal 
> device power management may not work without the CPU being online.
>
> 3) Additionally we have other issue of supporting system instructions
> with ACPI, which do not appear on the AMBA bus.
>
> Considering all of these, the ideal solution is to :
>
> 1) Implement power management for ACPI, which is anyway in progress
>   (at least for SCMI based systems)
> 2) Move the ETM driver away from the AMBA framework. That would make
>    the CPU online problem and the (3) above easier to solve.
>    Anshuman is going to look into this.
>
> In the meantime, we could have this temporary fix in and solve it
> forever by moving away from the AMBA.

Ok, thanks.
Looking forward to the ideal soluation and i am glad to test it after 
implemented.

> > Signed-off-by: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
> > ---
> >   drivers/acpi/acpi_amba.c | 36 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c
> > index ab8a4e0191b1..2369198f734b 100644
> > --- a/drivers/acpi/acpi_amba.c
> > +++ b/drivers/acpi/acpi_amba.c
> > @@ -16,6 +16,7 @@
> >   #include <linux/ioport.h>
> >   #include <linux/kernel.h>
> >   #include <linux/module.h>
> > +#include <acpi/processor.h>
> >     #include "internal.h"
> >   @@ -45,6 +46,35 @@ static void amba_register_dummy_clk(void)
> >       clk_register_clkdev(amba_dummy_clk, "apb_pclk", NULL);
> >   }
> >   +static int acpi_handle_to_cpuid(acpi_handle handle)
> > +{
> > +    int cpu = -1;
> > +    struct acpi_processor *pr;
> > +
> > +    for_each_possible_cpu(cpu) {
> > +        pr = per_cpu(processors, cpu);
> > +        if (pr && pr->handle == handle)
> > +            break;
> > +    }
> > +
> > +    return cpu;
> > +}
> > +
>
> Please could we reuse the function in coresight-platform.c ?
> i.e, move it to a generic location and share it, rather than
> duplicating it ?
>
>
> > +static int acpi_dev_get_cpu(struct acpi_device *adev)
> > +{
> > +    acpi_handle cpu_handle;
> > +    acpi_status status;
> > +    int cpu;
> > +
> > +    status = acpi_get_parent(adev->handle, &cpu_handle);
> > +    if (ACPI_FAILURE(status))
> > +        return -1;
> > +    cpu = acpi_handle_to_cpuid(cpu_handle);
> > +    if (cpu >= nr_cpu_ids)
> > +        return -1;
> > +    return cpu;
> > +}
> > +
> >   static int amba_handler_attach(struct acpi_device *adev,
> >                   const struct acpi_device_id *id)
> >   {
> > @@ -54,11 +84,17 @@ static int amba_handler_attach(struct acpi_device 
> > *adev,
> >       bool address_found = false;
> >       int irq_no = 0;
> >       int ret;
> > +    int cpu;
> >         /* If the ACPI node already has a physical device attached, 
> > skip it. */
> >       if (adev->physical_node_count)
> >           return 0;
> >   +    /* If the cpu associated with the device is not online, skip 
> > it. */
> > +    cpu = acpi_dev_get_cpu(adev);
> > +    if (cpu >= 0 && !cpu_online(cpu))
> > +        return 0;
> > +
>
> Except for the comment above, the patch looks good to me.
>
> Suzuki
> .