Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param
count restriction from select()") the irq_find_matching_fwspec() was
handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of
the select() callback. However the callback of GICv3 (even though it got
modified to handle zero param_count) wasn't prepared to return true for
DOMAIN_BUS_ANY bus_token.
This breaks probing of any of the child IRQ domains, since
platform_irqchip_probe() uses irq_find_matching_host(par_np,
DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain.
Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count")
Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
drivers/irqchip/irq-gic-v3.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 6fb276504bcc..e9e9643c653f 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d,
/* Handle pure domain searches */
if (!fwspec->param_count)
- return d->bus_token == bus_token;
+ return d->bus_token == bus_token ||
+ bus_token == DOMAIN_BUS_ANY;
/* If this is not DT, then we have a single domain */
if (!is_of_node(fwspec->fwnode))
---
base-commit: 35a4fdde2466b9d90af297f249436a270ef9d30e
change-id: 20240219-gic-fix-child-domain-8a1840be9ff5
Best regards,
--
Dmitry Baryshkov <[email protected]>
On Mon, 19 Feb 2024 14:47:37 +0000,
Dmitry Baryshkov <[email protected]> wrote:
>
> Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param
> count restriction from select()") the irq_find_matching_fwspec() was
> handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of
> the select() callback. However the callback of GICv3 (even though it got
> modified to handle zero param_count) wasn't prepared to return true for
> DOMAIN_BUS_ANY bus_token.
>
> This breaks probing of any of the child IRQ domains, since
> platform_irqchip_probe() uses irq_find_matching_host(par_np,
> DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain.
>
> Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count")
> Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 6fb276504bcc..e9e9643c653f 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d,
>
> /* Handle pure domain searches */
> if (!fwspec->param_count)
> - return d->bus_token == bus_token;
> + return d->bus_token == bus_token ||
> + bus_token == DOMAIN_BUS_ANY;
>
> /* If this is not DT, then we have a single domain */
> if (!is_of_node(fwspec->fwnode))
>
I really dislike the look of this. If that's the case, any irqchip
that has a 'select' method (such as imx-intmux) should be similarly
hacked. And at this point, this should be handled by the core code.
Can you try this instead? I don't have any HW that relies on
behaviour, but I'd expect this to work.
Thanks,
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);
--
Without deviation from the norm, progress is not possible.
On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <[email protected]> wrote:
>
> On Mon, 19 Feb 2024 14:47:37 +0000,
> Dmitry Baryshkov <[email protected]> wrote:
> >
> > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param
> > count restriction from select()") the irq_find_matching_fwspec() was
> > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of
> > the select() callback. However the callback of GICv3 (even though it got
> > modified to handle zero param_count) wasn't prepared to return true for
> > DOMAIN_BUS_ANY bus_token.
> >
> > This breaks probing of any of the child IRQ domains, since
> > platform_irqchip_probe() uses irq_find_matching_host(par_np,
> > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain.
> >
> > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count")
> > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
> > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > ---
> > drivers/irqchip/irq-gic-v3.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 6fb276504bcc..e9e9643c653f 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d,
> >
> > /* Handle pure domain searches */
> > if (!fwspec->param_count)
> > - return d->bus_token == bus_token;
> > + return d->bus_token == bus_token ||
> > + bus_token == DOMAIN_BUS_ANY;
> >
> > /* If this is not DT, then we have a single domain */
> > if (!is_of_node(fwspec->fwnode))
> >
>
> I really dislike the look of this. If that's the case, any irqchip
> that has a 'select' method (such as imx-intmux) should be similarly
> hacked. And at this point, this should be handled by the core code.
>
> Can you try this instead? I don't have any HW that relies on
> behaviour, but I'd expect this to work.
>
> Thanks,
>
> 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);
This works. But I wonder if the following change is even better. WDYT?
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index aeb41655d6de..2f0d2700709e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -449,14 +449,17 @@ 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 (fwnode != NULL &&
+ h->fwnode == fwnode &&
+ bus_token == DOMAIN_BUS_ANY)
+ rc = true;
+ else if (h->ops->select)
rc = h->ops->select(h, fwspec, bus_token);
else if (h->ops->match)
rc = h->ops->match(h, to_of_node(fwnode), bus_token);
else
rc = ((fwnode != NULL) && (h->fwnode == fwnode) &&
- ((bus_token == DOMAIN_BUS_ANY) ||
- (h->bus_token == bus_token)));
+ (h->bus_token == bus_token));
if (rc) {
found = h;
--
With best wishes
Dmitry
On Mon, 19 Feb 2024 16:21:06 +0000,
Dmitry Baryshkov <[email protected]> wrote:
>
> On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <[email protected]> wrote:
> >
> > On Mon, 19 Feb 2024 14:47:37 +0000,
> > Dmitry Baryshkov <[email protected]> wrote:
> > >
> > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param
> > > count restriction from select()") the irq_find_matching_fwspec() was
> > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of
> > > the select() callback. However the callback of GICv3 (even though it got
> > > modified to handle zero param_count) wasn't prepared to return true for
> > > DOMAIN_BUS_ANY bus_token.
> > >
> > > This breaks probing of any of the child IRQ domains, since
> > > platform_irqchip_probe() uses irq_find_matching_host(par_np,
> > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain.
> > >
> > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count")
> > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
> > > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > > ---
> > > drivers/irqchip/irq-gic-v3.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > index 6fb276504bcc..e9e9643c653f 100644
> > > --- a/drivers/irqchip/irq-gic-v3.c
> > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d,
> > >
> > > /* Handle pure domain searches */
> > > if (!fwspec->param_count)
> > > - return d->bus_token == bus_token;
> > > + return d->bus_token == bus_token ||
> > > + bus_token == DOMAIN_BUS_ANY;
> > >
> > > /* If this is not DT, then we have a single domain */
> > > if (!is_of_node(fwspec->fwnode))
> > >
> >
> > I really dislike the look of this. If that's the case, any irqchip
> > that has a 'select' method (such as imx-intmux) should be similarly
> > hacked. And at this point, this should be handled by the core code.
> >
> > Can you try this instead? I don't have any HW that relies on
> > behaviour, but I'd expect this to work.
> >
> > Thanks,
> >
> > 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);
>
> This works. But I wonder if the following change is even better. WDYT?
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index aeb41655d6de..2f0d2700709e 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -449,14 +449,17 @@ 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 (fwnode != NULL &&
> + h->fwnode == fwnode &&
> + bus_token == DOMAIN_BUS_ANY)
> + rc = true;
> + else if (h->ops->select)
> rc = h->ops->select(h, fwspec, bus_token);
> else if (h->ops->match)
> rc = h->ops->match(h, to_of_node(fwnode), bus_token);
> else
> rc = ((fwnode != NULL) && (h->fwnode == fwnode) &&
> - ((bus_token == DOMAIN_BUS_ANY) ||
> - (h->bus_token == bus_token)));
> + (h->bus_token == bus_token));
>
> if (rc) {
> found = h;
>
Can't say I like it either. It duplicates the existing check without
any obvious benefit. Honestly, this code is shit enough that we should
try to make it simpler, not more complex...
I'd rather we keep the impact as minimal as possible, and use the
upcoming weeks to weed out the effects of these changes (there is
another report of some Renesas machine falling over itself here[1]).
Thanks,
M.
[1] https://lore.kernel.org/all/170802702416.398.14922976721740218856.tip-bot2@tip-bot2
--
Without deviation from the norm, progress is not possible.
On Mon, 19 Feb 2024 17:41:37 +0000,
Dmitry Baryshkov <[email protected]> wrote:
>
> On Mon, 19 Feb 2024 at 18:37, Marc Zyngier <[email protected]> wrote:
> >
> > On Mon, 19 Feb 2024 16:21:06 +0000,
> > Dmitry Baryshkov <[email protected]> wrote:
> > >
> > > On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <[email protected]> wrote:
> > > >
> > > > On Mon, 19 Feb 2024 14:47:37 +0000,
> > > > Dmitry Baryshkov <[email protected]> wrote:
> > > > >
> > > > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param
> > > > > count restriction from select()") the irq_find_matching_fwspec() was
> > > > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of
> > > > > the select() callback. However the callback of GICv3 (even though it got
> > > > > modified to handle zero param_count) wasn't prepared to return true for
> > > > > DOMAIN_BUS_ANY bus_token.
> > > > >
> > > > > This breaks probing of any of the child IRQ domains, since
> > > > > platform_irqchip_probe() uses irq_find_matching_host(par_np,
> > > > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain.
> > > > >
> > > > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count")
> > > > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
> > > > > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > > > > ---
> > > > > drivers/irqchip/irq-gic-v3.c | 3 ++-
> > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > > > index 6fb276504bcc..e9e9643c653f 100644
> > > > > --- a/drivers/irqchip/irq-gic-v3.c
> > > > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d,
> > > > >
> > > > > /* Handle pure domain searches */
> > > > > if (!fwspec->param_count)
> > > > > - return d->bus_token == bus_token;
> > > > > + return d->bus_token == bus_token ||
> > > > > + bus_token == DOMAIN_BUS_ANY;
> > > > >
> > > > > /* If this is not DT, then we have a single domain */
> > > > > if (!is_of_node(fwspec->fwnode))
> > > > >
> > > >
> > > > I really dislike the look of this. If that's the case, any irqchip
> > > > that has a 'select' method (such as imx-intmux) should be similarly
> > > > hacked. And at this point, this should be handled by the core code.
> > > >
> > > > Can you try this instead? I don't have any HW that relies on
> > > > behaviour, but I'd expect this to work.
> > > >
> > > > Thanks,
> > > >
> > > > 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);
> > >
> > > This works. But I wonder if the following change is even better. WDYT?
> > >
> > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > > index aeb41655d6de..2f0d2700709e 100644
> > > --- a/kernel/irq/irqdomain.c
> > > +++ b/kernel/irq/irqdomain.c
> > > @@ -449,14 +449,17 @@ 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 (fwnode != NULL &&
> > > + h->fwnode == fwnode &&
> > > + bus_token == DOMAIN_BUS_ANY)
> > > + rc = true;
> > > + else if (h->ops->select)
> > > rc = h->ops->select(h, fwspec, bus_token);
> > > else if (h->ops->match)
> > > rc = h->ops->match(h, to_of_node(fwnode), bus_token);
> > > else
> > > rc = ((fwnode != NULL) && (h->fwnode == fwnode) &&
> > > - ((bus_token == DOMAIN_BUS_ANY) ||
> > > - (h->bus_token == bus_token)));
> > > + (h->bus_token == bus_token));
> > >
> > > if (rc) {
> > > found = h;
> > >
> >
> > Can't say I like it either. It duplicates the existing check without
> > any obvious benefit. Honestly, this code is shit enough that we should
> > try to make it simpler, not more complex...
>
> Only the fwnode conditions are duplicated. And it makes sense: we
> should check for the DOMAIN_BUS_ANY first, before going to select. I'm
> not sure whether at some point we'd have to add (&& bus_token !=
> DOMAIN_BUS_ANY) to the ops->match check.
ops->match should just *die*, and it is not going to see any sort of
semantic upgrade. Ever. No new code should be added using match.
And look at what my change does. It checks for DOMAIN_BUS_ANY before
doing anything else, ensuring that the default clause does the job. So
no, your suggestion doesn't make much sense to me.
M.
--
Without deviation from the norm, progress is not possible.
On Mon, 19 Feb 2024 at 18:37, Marc Zyngier <[email protected]> wrote:
>
> On Mon, 19 Feb 2024 16:21:06 +0000,
> Dmitry Baryshkov <[email protected]> wrote:
> >
> > On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <[email protected]> wrote:
> > >
> > > On Mon, 19 Feb 2024 14:47:37 +0000,
> > > Dmitry Baryshkov <[email protected]> wrote:
> > > >
> > > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param
> > > > count restriction from select()") the irq_find_matching_fwspec() was
> > > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of
> > > > the select() callback. However the callback of GICv3 (even though it got
> > > > modified to handle zero param_count) wasn't prepared to return true for
> > > > DOMAIN_BUS_ANY bus_token.
> > > >
> > > > This breaks probing of any of the child IRQ domains, since
> > > > platform_irqchip_probe() uses irq_find_matching_host(par_np,
> > > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain.
> > > >
> > > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count")
> > > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
> > > > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > > > ---
> > > > drivers/irqchip/irq-gic-v3.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > > index 6fb276504bcc..e9e9643c653f 100644
> > > > --- a/drivers/irqchip/irq-gic-v3.c
> > > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d,
> > > >
> > > > /* Handle pure domain searches */
> > > > if (!fwspec->param_count)
> > > > - return d->bus_token == bus_token;
> > > > + return d->bus_token == bus_token ||
> > > > + bus_token == DOMAIN_BUS_ANY;
> > > >
> > > > /* If this is not DT, then we have a single domain */
> > > > if (!is_of_node(fwspec->fwnode))
> > > >
> > >
> > > I really dislike the look of this. If that's the case, any irqchip
> > > that has a 'select' method (such as imx-intmux) should be similarly
> > > hacked. And at this point, this should be handled by the core code.
> > >
> > > Can you try this instead? I don't have any HW that relies on
> > > behaviour, but I'd expect this to work.
> > >
> > > Thanks,
> > >
> > > 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);
> >
> > This works. But I wonder if the following change is even better. WDYT?
> >
> > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > index aeb41655d6de..2f0d2700709e 100644
> > --- a/kernel/irq/irqdomain.c
> > +++ b/kernel/irq/irqdomain.c
> > @@ -449,14 +449,17 @@ 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 (fwnode != NULL &&
> > + h->fwnode == fwnode &&
> > + bus_token == DOMAIN_BUS_ANY)
> > + rc = true;
> > + else if (h->ops->select)
> > rc = h->ops->select(h, fwspec, bus_token);
> > else if (h->ops->match)
> > rc = h->ops->match(h, to_of_node(fwnode), bus_token);
> > else
> > rc = ((fwnode != NULL) && (h->fwnode == fwnode) &&
> > - ((bus_token == DOMAIN_BUS_ANY) ||
> > - (h->bus_token == bus_token)));
> > + (h->bus_token == bus_token));
> >
> > if (rc) {
> > found = h;
> >
>
> Can't say I like it either. It duplicates the existing check without
> any obvious benefit. Honestly, this code is shit enough that we should
> try to make it simpler, not more complex...
Only the fwnode conditions are duplicated. And it makes sense: we
should check for the DOMAIN_BUS_ANY first, before going to select. I'm
not sure whether at some point we'd have to add (&& bus_token !=
DOMAIN_BUS_ANY) to the ops->match check.
>
> I'd rather we keep the impact as minimal as possible, and use the
> upcoming weeks to weed out the effects of these changes (there is
> another report of some Renesas machine falling over itself here[1]).
>
> Thanks,
>
> M.
>
> [1] https://lore.kernel.org/all/170802702416.398.14922976721740218856.tip-bot2@tip-bot2
>
> --
> Without deviation from the norm, progress is not possible.
--
With best wishes
Dmitry
On Mon, 19 Feb 2024 at 19:51, Marc Zyngier <[email protected]> wrote:
>
> On Mon, 19 Feb 2024 17:41:37 +0000,
> Dmitry Baryshkov <[email protected]> wrote:
> >
> > On Mon, 19 Feb 2024 at 18:37, Marc Zyngier <[email protected]> wrote:
> > >
> > > On Mon, 19 Feb 2024 16:21:06 +0000,
> > > Dmitry Baryshkov <[email protected]> wrote:
> > > >
> > > > On Mon, 19 Feb 2024 at 17:53, Marc Zyngier <[email protected]> wrote:
> > > > >
> > > > > On Mon, 19 Feb 2024 14:47:37 +0000,
> > > > > Dmitry Baryshkov <[email protected]> wrote:
> > > > > >
> > > > > > Before the commit de1ff306dcf4 ("genirq/irqdomain: Remove the param
> > > > > > count restriction from select()") the irq_find_matching_fwspec() was
> > > > > > handling the DOMAIN_BUS_ANY on its own. After this commit it is a job of
> > > > > > the select() callback. However the callback of GICv3 (even though it got
> > > > > > modified to handle zero param_count) wasn't prepared to return true for
> > > > > > DOMAIN_BUS_ANY bus_token.
> > > > > >
> > > > > > This breaks probing of any of the child IRQ domains, since
> > > > > > platform_irqchip_probe() uses irq_find_matching_host(par_np,
> > > > > > DOMAIN_BUS_ANY) to check for the presence of the parent IRQ domain.
> > > > > >
> > > > > > Fixes: 151378251004 ("irqchip/gic-v3: Make gic_irq_domain_select() robust for zero parameter count")
> > > > > > Fixes: de1ff306dcf4 ("genirq/irqdomain: Remove the param count restriction from select()")
> > > > > > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > > > > > ---
> > > > > > drivers/irqchip/irq-gic-v3.c | 3 ++-
> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > > > > index 6fb276504bcc..e9e9643c653f 100644
> > > > > > --- a/drivers/irqchip/irq-gic-v3.c
> > > > > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > > > > @@ -1696,7 +1696,8 @@ static int gic_irq_domain_select(struct irq_domain *d,
> > > > > >
> > > > > > /* Handle pure domain searches */
> > > > > > if (!fwspec->param_count)
> > > > > > - return d->bus_token == bus_token;
> > > > > > + return d->bus_token == bus_token ||
> > > > > > + bus_token == DOMAIN_BUS_ANY;
> > > > > >
> > > > > > /* If this is not DT, then we have a single domain */
> > > > > > if (!is_of_node(fwspec->fwnode))
> > > > > >
> > > > >
> > > > > I really dislike the look of this. If that's the case, any irqchip
> > > > > that has a 'select' method (such as imx-intmux) should be similarly
> > > > > hacked. And at this point, this should be handled by the core code.
> > > > >
> > > > > Can you try this instead? I don't have any HW that relies on
> > > > > behaviour, but I'd expect this to work.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > 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);
> > > >
> > > > This works. But I wonder if the following change is even better. WDYT?
> > > >
> > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > > > index aeb41655d6de..2f0d2700709e 100644
> > > > --- a/kernel/irq/irqdomain.c
> > > > +++ b/kernel/irq/irqdomain.c
> > > > @@ -449,14 +449,17 @@ 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 (fwnode != NULL &&
> > > > + h->fwnode == fwnode &&
> > > > + bus_token == DOMAIN_BUS_ANY)
> > > > + rc = true;
> > > > + else if (h->ops->select)
> > > > rc = h->ops->select(h, fwspec, bus_token);
> > > > else if (h->ops->match)
> > > > rc = h->ops->match(h, to_of_node(fwnode), bus_token);
> > > > else
> > > > rc = ((fwnode != NULL) && (h->fwnode == fwnode) &&
> > > > - ((bus_token == DOMAIN_BUS_ANY) ||
> > > > - (h->bus_token == bus_token)));
> > > > + (h->bus_token == bus_token));
> > > >
> > > > if (rc) {
> > > > found = h;
> > > >
> > >
> > > Can't say I like it either. It duplicates the existing check without
> > > any obvious benefit. Honestly, this code is shit enough that we should
> > > try to make it simpler, not more complex...
> >
> > Only the fwnode conditions are duplicated. And it makes sense: we
> > should check for the DOMAIN_BUS_ANY first, before going to select. I'm
> > not sure whether at some point we'd have to add (&& bus_token !=
> > DOMAIN_BUS_ANY) to the ops->match check.
>
> ops->match should just *die*, and it is not going to see any sort of
> semantic upgrade. Ever. No new code should be added using match.
>
> And look at what my change does. It checks for DOMAIN_BUS_ANY before
> doing anything else, ensuring that the default clause does the job. So
> no, your suggestion doesn't make much sense to me.
Yeah, I was worried about the DOMAIN_BUS_ANY vs match call. If that's
not an issue, your patch looks fine to me.
Please use 'Tested-by: Dmitry Baryshkov <[email protected]>'
with your patch.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
--
With best wishes
Dmitry