Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On 2019-11-20 13:34, Andreas FÃrber wrote:
> Am 20.11.19 um 11:20 schrieb Marc Zyngier:
> > On 2019-11-19 23:33, Andreas FÃrber wrote:
> > > Am 19.11.19 um 23:29 schrieb Marc Zyngier:
> > > > On Tue, 19 Nov 2019 21:56:48 +0100
> > > > Andreas FÃrber <afaerber@xxxxxxx> wrote:
> > > > > Am 19.11.19 um 13:01 schrieb Marc Zyngier:
> > > > > > On 2019-11-19 02:19, Andreas FÃrber wrote:
> > > > > > > +static void rtd1195_mux_enable_irq(struct irq_data *data)
> > > > > > > +{
> > > > > > > +ÂÂÂ struct rtd1195_irq_mux_data *mux_data =
> > > > > > > irq_data_get_irq_chip_data(data);
> > > > > > > +ÂÂÂ unsigned long flags;
> > > > > > > +ÂÂÂ u32 mask;
> > > > > > > +
> > > > > > > +ÂÂÂ mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
> > > > > > > +ÂÂÂ if (!mask)
> > > > > > > +ÂÂÂÂÂÂÂ return;
> > > > > >
> > > > > > How can this happen? You've mapped the interrupt, so it exists.
> > > > > > I can't see how you can decide to fail such enable.
> > > > >
> > > > > The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the 
> > > > > same.
> > > > >
> > > > > My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) 
> > > > > lookup, but
> > > > > are sparsely populated. So there are circumstances such as 
> > > > > WDOG_NMI as
> > > > > well as reserved bits that we cannot enable.
> > > >
> > > > But the you should have failed the map. The moment you allow the
> > > > mapping to occur, you have accepted the contract that this 
> > > > interrupt is
> > > > usable.
> > > >
> > > > > This check should be
> > > > > identical to v3; the equivalent mask check inside the interrupt 
> > > > > handler
> > > > > was extended with "mask &&" to do the same in this v4.
> > > >
> > > > Spurious interrupts are a different matter. What I'm objecting to 
> > > > here
> > > > is a simple question of logic, whether or not you are allowed to 
> > > > fail
> > > > enabling an interrupt that you've otherwise allowed to be 
> > > > populated.
> > >
> > > Then what are you suggesting instead? I don't see how my array map
> > > lookup could fail other than returning a zero value, given its 
> > > static
> > > initialization. Check for a zero mask in 
> > > rtd1195_mux_irq_domain_map()?
> > > Then we wouldn't be able to use the mentioned WDOG_NMI. Add another
> > > per-mux info field for which interrupts are valid to map?
> >
> > I'm suggesting that you fail the map if you're unable to allow the
> > interrupt to be enabled.
>
> The NMI will always be enabled, it just can't be disabled.

If I really cared, I'd cry. This HW is useless.

> I have added a check to suppress a zero hwirq. Suppressing reserved 
> IRQ
> bits will take some more effort to distinguish from NMIs. In 
> particular
> if we flag this in the ..._isr_to_scpu_int_en array by some magic 
> mask
> value like 0xffffffff then all users need to check for two rather 
> than
> one value - but if we reduce the users, it shouldn't matter too much.

1) you can't suppress a level interrupt that cannot be disabled. It 
will
fire back at you.
2) given that you have to demux things using MMIO accesses, performance
is the least of anybody's worry.

> With contract I assume you're referring to these callbacks having a 
> void
> return type, unable to return an error to the caller, and there being 
> no
> is_enabled/is_masked callbacks for anyone to discover this.
>
> Unfortunately NMI handling appears to be only used in GICv3 and is 
> not
> very intuitive for me: Apparently I can only flag the whole irq_chip 
> as
> being NMI but not individual IRQs? Would that mean that this driver
> would need to instantiate a second irq_chip for that one IRQ? How 
> would
> that work for mapping from DT? Given that this mux relies on a 
> maskable
> GICv2 IRQ, it's not a "true" NMI in the Linux sense anyway, other 
> than
> the .irq_mask callback not being applicable. While I don't need that 
> NMI
> immediately, I would prefer not to merge a driver that by design 
> can't
> cope with it later.

You are missing the point of the pseudo-NMI infrastructure. To be 
useful,
it *must* be the root interrupt controller. Otherwise, you cannot 
distinguish
it from the other interrupts it is muxed with. Your 'NMI' is absolutely
unusable, and whoever designed this HW should be actively prevented
from ever designing another interrupt controller again.

> I'll try to post a v5 with rsv and nmi blocked in map for further
> discussion tonight.

I don't plan to review any of this until after the merge window, so 
please
take as long as you want.

        M.
--
Jazz is not dead. It just smells funny...