Re: [PATCH 1/2 v1] gpio: sifive: Set affinity callback to parent

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Marc,

On Tue, Apr 6, 2021 at 12:40 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> On Tue, 06 Apr 2021 11:20:57 +0100,
> Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > On Tue, Nov 17, 2020 at 10:37 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> > > This assigns the .irq_set_affinity to the parent callback.
> > > I assume the sifive GPIO can be used in systems with
> > > SMP.
> > >
> > > I used the pattern making the hirerarchy tolerant for missing
> > > parent as in Marc's earlier patches.
> > >
> > > Cc: Yash Shah <yash.shah@xxxxxxxxxx>
> > > Cc: Wesley W. Terpstra <wesley@xxxxxxxxxx>
> > > Suggested-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> >
> > Thanks for your patch!
> >
> > > ---
> > > ChangeLog RFT->v1:
> > > - Make the affinity setting call return -EINVAL if there
> > >   is no parent.
> >
> > Would it make sense to incorporate this check into
> > irq_chip_set_affinity_parent(), so drivers can just point
> > .irq_set_affinity to the latter, without having to provide (duplicate)
> > the same wrapper over and over?
>
> Calling one of the irq_chip_*_parent() functions assumes that there
> *is* a parent at all times, because you really need to know what
> context you are in by construction. There are a couple of exceptions
> to this rule (irqchip state, retriggering), but overall I'd like to
> stick to it and leave the checks on the implementations that have
> weird setups.
>
> I would assume that it is possible to know at the point where you map
> the interrupt whether it has a parent or not, and use a different
> irqchip. Is that doable in this case?

I think you're missing my point (or I'm missing yours ;-)

I don't mean to set up .irq_set_affinity = irq_chip_set_affinity_parent()
by default.

Right now, several drivers do this:

    static int foo_irq_set_affinity(struct irq_data *data,
                                       const struct cpumask *dest,
                                       bool force)
    {
           if (data->parent_data)
                   return irq_chip_set_affinity_parent(data, dest, force);

           return -EINVAL;
    }

    .irq_set_affinity = foo_irq_set_affinity,

If irq_chip_set_affinity_parent() would not blindly dereference
data->parent_data, there would be no need for the
foo_irq_set_affinity() wrappers.

Or are all those drivers using such a wrapper wrong?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds