Re: [PATCH V1 1/5] irqchip/loongson-eiointc: Fix returned value on parsing MADT

[Jianmin Lv]

On 2023/3/24 下午11:36, Marc Zyngier wrote:
> On Fri, 24 Mar 2023 06:08:50 +0000,
> Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:
> > In pch_pic_parse_madt(), a NULL parent pointer will be
> > returned from acpi_get_vec_parent() for second pch-pic domain
> > related to second bridge while calling eiointc_acpi_init() at
> > first time, where the parent of it has not been initialized
> > yet, and will be initialized during second time calling
> > eiointc_acpi_init(). So, it's reasonable to return zero so
> > that failure of acpi_table_parse_madt() will be avoided, or else
> > acpi_cascade_irqdomain_init() will return and initialization of
> > followed pch_msi domain will be skipped.
> >
> > Although it does not matter when pch_msi_parse_madt() returns
> > -EINVAL if no invalid parent is found, it's also reasonable to
> > return zero for that.
> >
> > Change-Id: I4d278534999ec3e5c8db6d40155ba2665d9de86f
> > Signed-off-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
> > ---
> >   drivers/irqchip/irq-loongson-eiointc.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
> > index d15fd38c1756..62a632d73991 100644
> > --- a/drivers/irqchip/irq-loongson-eiointc.c
> > +++ b/drivers/irqchip/irq-loongson-eiointc.c
> > @@ -343,7 +343,7 @@ static int __init pch_pic_parse_madt(union acpi_subtable_headers *header,
> >   	if (parent)
> >   		return pch_pic_acpi_init(parent, pchpic_entry);
> >   
> > -	return -EINVAL;
> > +	return 0;
>
> Why can't you detect this particular case instead of blindly
> suppressing the error?

For dual-bridges scenario, parent handle for pch_pic IRQ domain will be 
set by acpi_set_vec_parent() during each eiointc initialization. And
the parent handle for one pch_pic will be set during *first* eiointc
entry parsing, and the parent handle for another pch_pic will be
set during *second* eiointc entry parsing. But two pch_pic entries
will be parsed during each eiointc entry parsing, so a NULL parent
pointer must be returned druing first eiointc initialization, which
is reasonable and should not be treated as an error.

The calling stack of pch_pic_parse_madt (where acpi_get_vec_parent is 
called to get parent handle) is like this:

cpuintc_acpi_init
  acpi_cascade_irqdomain_init(in cpuintc driver)
    acpi_table_parse_madt
      eiointc_parse_madt
        eiointc_acpi_init /* this will be called two times 
corresponding  to parsing two eiointc entries in MADT under dual-bridges 
scenario*/
          acpi_set_vec_parent /* set parent handle for one pch_pic 
during first eiointc entry parsing, and set parent handle for another 
pch_pic during second eiointc entry parsing*/
            acpi_cascade_irqdomain_init(in eiointc driver)
              acpi_table_parse_madt
                pch_pic_parse_madt /* this will be called twice because 
of two pchpic entries, and only one valid parent handle will be returned 
from acpi_get_vec_parent() during first eiointc entry parsing (another 
parent handle is not set yet), so a NULL parent pointer must be returned 
during first eiointc entry parsing, which is reasonable and should not 
be treated as an error.*/

Thanks!

> 	M.