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

[Marc Zyngier <maz@kernel.org>]

Hi Geert,

On Tue, 06 Apr 2021 11:51:25 +0100,
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> Hi Marc,
> 
> On Tue, Apr 6, 2021 at 12:40 PM Marc Zyngier <maz@kernel.org> wrote:
> > On Tue, 06 Apr 2021 11:20:57 +0100,
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Nov 17, 2020 at 10:37 PM Linus Walleij <linus.walleij@linaro.org> 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@sifive.com>
> > > > Cc: Wesley W. Terpstra <wesley@sifive.com>
> > > > Suggested-by: Marc Zyngier <maz@kernel.org>
> > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > >
> > > 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.

The "blind dereference" is a completely assumed design choice. That's
because when you instantiate an irqchip, you know whether there is
another chip on the IRQ path, or whether this is a root (or a mux,
which amounts to the same thing).

So in most cases, you shouldn't need to check for a parent. You know
there is one by construction, and if there isn't one, you don't call
the *_parent() anyway. So unless the HW is representative of what I
describe below, a static parent/no-parent setup is preferable.

> Or are all those drivers using such a wrapper wrong?

I only know of a few drivers that have some variability around that,
which resulted in some hacks similar to what you describe. See these
patches for example:

c351ab7bf2a5 soc/tegra: pmc: Don't create fake interrupt hierarchy levels
8681cc33f817 soc/tegra: pmc: Allow optional irq parent callbacks
986ec63d4482 gpio: tegra186: Allow optional irq parent callbacks
55567976629e genirq/irqdomain: Allow partial trimming of irq_data hierarchy

This could have been avoided by restructuring the driver, but would
also have had impacts on DT, resulting in something even more horrible.

QC's PDC also suffer from a similar hack, which I plan to address once
I get this !"£$% machine to boot...

But in general, if you need to check for a parent, that's because you
are doing something that is either a bit unexpected, or has a *very*
broad spectrum (doing something generic enough that it must cope with
all possible situations).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.