| Message ID | 20240220114731.1898534-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens | expand |
Hi Marc Zyngier, > -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: Tuesday, February 20, 2024 11:48 AM > Subject: [PATCH] genirq/irqdomain: Don't call ops->select for > DOMAIN_BUS_ANY tokens > > Users of the IRQCHIP_PLATFORM_DRIVER_{BEGIN,END} helpers rely on a fwspec > containing only the fwnode (and crucially a number of parameters set to 0) > together with a DOMAIN_BUS_ANY token to check whether a parent irqchip has > probed and registered a domain. > > Since de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction > from select()"), we call ops->select unconditionally, meaning that > irqchips implementing select now need to handle ANY as a match. > > Instead of adding more esoteric checks to the individual drivers, add that > condition to irq_find_matching_fwspec(), and let it handle the corner > case, as per the comment in the function. > > This restores the functionnality of the above helpers. > > Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Reported-by: Biju Das <biju.das.jz@bp.renesas.com> Tested-by: Biju Das <biju.das.jz@bp.renesas.com> Cheers, Biju > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction > from select()") > Signed-off-by: Marc Zyngier <maz@kernel.org> > Link: > --- > kernel/irq/irqdomain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index > aeb41655d6de..3dd1c871e091 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct > irq_fwspec *fwspec, > */ > mutex_lock(&irq_domain_mutex); > list_for_each_entry(h, &irq_domain_list, link) { > - if (h->ops->select) > + if (h->ops->select && bus_token != DOMAIN_BUS_ANY) > rc = h->ops->select(h, fwspec, bus_token); > else if (h->ops->match) > rc = h->ops->match(h, to_of_node(fwnode), bus_token); > -- > 2.39.2
On Tue, Feb 20 2024 at 11:47, Marc Zyngier wrote: > Users of the IRQCHIP_PLATFORM_DRIVER_{BEGIN,END} helpers rely > on a fwspec containing only the fwnode (and crucially a number > of parameters set to 0) together with a DOMAIN_BUS_ANY token > to check whether a parent irqchip has probed and registered > a domain. > > Since de1ff306dcf4 ("genirq/irqdomain: Remove the param count > restriction from select()"), we call ops->select unconditionally, > meaning that irqchips implementing select now need to handle > ANY as a match. > > Instead of adding more esoteric checks to the individual drivers, > add that condition to irq_find_matching_fwspec(), and let it > handle the corner case, as per the comment in the function. > > This restores the functionnality of the above helpers. > > Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Reported-by: Biju Das <biju.das.jz@bp.renesas.com> > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()") > Signed-off-by: Marc Zyngier <maz@kernel.org> > Link: https://lore.kernel.org/r/20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org Bah. That breaks x86 because it uses DOMAIN_BUS_ANY to find the MSI parent for a fwspec (IOAPIC and HPET) which gets either picked up by the interrupt remapping or by the root vector domain. Fix below. Thanks, tglx --- Subject: x86/apic/msi: Use DOMAIN_BUS_GENERIC_MSI for HPET/IO-APIC domain search From: Thomas Gleixner <tglx@linutronix.de> Date: Sun, 25 Feb 2024 16:56:12 +0100 The recent restriction to invoke irqdomain_ops::select() only when the domain bus toke is DOMAIN_BUS_ANY breaks the search for the parent MSI domain of HPET and IO-APIC. The latter causes a full boot fail. The restriction itself makes sense to avoid adding DOMAIN_BUS_ANY matches into the various ARM specific select() callbacks. Reverting this change would obviously break ARM platforms again and require DOMAIN_BUS_ANY matches added to various places. A simpler solution is to use the DOMAIN_BUS_GENERIC_MSI token for the HPET and IO-APIC parent domain search. This works out of the box because the affected parent domains check only for the firmware specification content and not for the bus token. Fixes: 5aa3c0cf5bba ("genirq/irqdomain: Don't call ops->select for DOMAIN_BUS_ANY tokens") Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/apic/io_apic.c | 2 +- arch/x86/kernel/hpet.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -2354,7 +2354,7 @@ static int mp_irqdomain_create(int ioapi fwspec.param_count = 1; fwspec.param[0] = mpc_ioapic_id(ioapic); - parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_ANY); + parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_GENERIC_MSI); if (!parent) { if (!cfg->dev) irq_domain_free_fwnode(fn); --- a/arch/x86/kernel/hpet.c +++ b/arch/x86/kernel/hpet.c @@ -568,7 +568,7 @@ static struct irq_domain *hpet_create_ir fwspec.param_count = 1; fwspec.param[0] = hpet_id; - parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_ANY); + parent = irq_find_matching_fwspec(&fwspec, DOMAIN_BUS_GENERIC_MSI); if (!parent) { irq_domain_free_fwnode(fn); kfree(domain_info);
On Sun, Feb 25, 2024 at 05:19:52PM +0100, Thomas Gleixner wrote: > Bah. That breaks x86 because it uses DOMAIN_BUS_ANY to find the MSI > parent for a fwspec (IOAPIC and HPET) which gets either picked up by the > interrupt remapping or by the root vector domain. > > Fix below. The guest boots fine now, thx. Tested-by: Borislav Petkov (AMD) <bp@alien8.de>
On 2024-02-25 16:19, Thomas Gleixner wrote: > On Tue, Feb 20 2024 at 11:47, Marc Zyngier wrote: >> Users of the IRQCHIP_PLATFORM_DRIVER_{BEGIN,END} helpers rely >> on a fwspec containing only the fwnode (and crucially a number >> of parameters set to 0) together with a DOMAIN_BUS_ANY token >> to check whether a parent irqchip has probed and registered >> a domain. >> >> Since de1ff306dcf4 ("genirq/irqdomain: Remove the param count >> restriction from select()"), we call ops->select unconditionally, >> meaning that irqchips implementing select now need to handle >> ANY as a match. >> >> Instead of adding more esoteric checks to the individual drivers, >> add that condition to irq_find_matching_fwspec(), and let it >> handle the corner case, as per the comment in the function. >> >> This restores the functionnality of the above helpers. >> >> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Reported-by: Biju Das <biju.das.jz@bp.renesas.com> >> Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count >> restriction from select()") >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> Link: >> https://lore.kernel.org/r/20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org > > Bah. That breaks x86 because it uses DOMAIN_BUS_ANY to find the MSI > parent for a fwspec (IOAPIC and HPET) which gets either picked up by > the > interrupt remapping or by the root vector domain. > > Fix below. > > Thanks, > > tglx > --- > Subject: x86/apic/msi: Use DOMAIN_BUS_GENERIC_MSI for HPET/IO-APIC > domain search > From: Thomas Gleixner <tglx@linutronix.de> > Date: Sun, 25 Feb 2024 16:56:12 +0100 > > The recent restriction to invoke irqdomain_ops::select() only when the > domain bus toke is DOMAIN_BUS_ANY breaks the search for the parent MSI > domain of HPET and IO-APIC. The latter causes a full boot fail. > > The restriction itself makes sense to avoid adding DOMAIN_BUS_ANY > matches > into the various ARM specific select() callbacks. Reverting this change > would obviously break ARM platforms again and require DOMAIN_BUS_ANY > matches added to various places. > > A simpler solution is to use the DOMAIN_BUS_GENERIC_MSI token for the > HPET > and IO-APIC parent domain search. This works out of the box because the > affected parent domains check only for the firmware specification > content > and not for the bus token. > > Fixes: 5aa3c0cf5bba ("genirq/irqdomain: Don't call ops->select for > DOMAIN_BUS_ANY tokens") > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Looks good to me. Reviewed-by: Marc Zyngier <maz@kernel.org> M.
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index aeb41655d6de..3dd1c871e091 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -449,7 +449,7 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, */ mutex_lock(&irq_domain_mutex); list_for_each_entry(h, &irq_domain_list, link) { - if (h->ops->select) + if (h->ops->select && bus_token != DOMAIN_BUS_ANY) rc = h->ops->select(h, fwspec, bus_token); else if (h->ops->match) rc = h->ops->match(h, to_of_node(fwnode), bus_token);
Users of the IRQCHIP_PLATFORM_DRIVER_{BEGIN,END} helpers rely on a fwspec containing only the fwnode (and crucially a number of parameters set to 0) together with a DOMAIN_BUS_ANY token to check whether a parent irqchip has probed and registered a domain. Since de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()"), we call ops->select unconditionally, meaning that irqchips implementing select now need to handle ANY as a match. Instead of adding more esoteric checks to the individual drivers, add that condition to irq_find_matching_fwspec(), and let it handle the corner case, as per the comment in the function. This restores the functionnality of the above helpers. Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reported-by: Biju Das <biju.das.jz@bp.renesas.com> Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()") Signed-off-by: Marc Zyngier <maz@kernel.org> Link: https://lore.kernel.org/r/20240219-gic-fix-child-domain-v1-1-09f8fd2d9a8f@linaro.org --- kernel/irq/irqdomain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)