Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons

[Marc Zyngier]

On Thu, 11 Aug 2016 14:23:53 -0700
Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:

Hi Bjorn,

> On Thu 11 Aug 05:46 PDT 2016, Marc Zyngier wrote:
> 
> > On 11/08/16 10:47, Jon Hunter wrote:  
> > > 
> > > On 11/08/16 09:37, Marc Zyngier wrote:  
> > >> On 08/08/16 22:48, Linus Walleij wrote:  
> > >>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> > >>>  
> > >>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
> > >>>> irq_fwspec *fwspec)
> > >>>>                  * it now and return the interrupt number.
> > >>>>                  */
> > >>>>                 if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> > >>>> -                       irq_set_irq_type(virq, type);
> > >>>> +                       irq_data = irq_get_irq_data(virq);
> > >>>> +                       if (!irq_data)
> > >>>> +                               return 0;
> > >>>> +
> > >>>> +                       irqd_set_trigger_type(irq_data, type);
> > >>>>                         return virq;
> > >>>>                 }
> > >>>>
> > >>>> If I revert just that, it works again.  
> > >>>
> > >>> This makes my platform work too.
> > >>> Tested-by: Linus Walleij <linus.walleij@xxxxxxxxxx>  
> > >>
> > >> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
> > >> hunk doesn't fix it for me. I'm confused...
> > >>
> > >> The interesting part is this:
> > >> 109:     100000          0   msmgpio  88 Level     (null)  
> > > 
> > > 88 is the pm8058 parent interrupt and so I am surprised you would even
> > > see this in /proc/interrupts as it should be a chained interrupt, right?
> > > 
> > > Are you seeing this with all the ethernet updates for the APQ8060 in
> > > Linus' branch? I am curious what you see with stock v4.8-rc1 and if
> > > interrupts work ok with the change I had proposed. Hard to tell if there
> > > is more than one issue here.  
> > 
> > Nailed the sucker:
> > 
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index b4c1bc7..9d7284a 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
> >  	desc->name = name;
> >  
> >  	if (handle != handle_bad_irq && is_chained) {
> > +		int ret;
> > +
> > +		ret = __irq_set_trigger(desc,
> > +					irqd_get_trigger_type(&desc->irq_data));
> > +		WARN_ON(ret);
> > +		/*
> > +		 * This is beyond ugly: .set_type may have overridden
> > +		 * the flow, not not knowing that we're dealing with a
> > +		 * chained handler. Reset it here because we know
> > +		 * better.
> > +		 */  
> 
> Thanks for this Marc!
> 
> But it makes me (author of pinctrl-msm) wonder, am I supposed to not
> implement .set_type like this for handling the transition between edge
> and level handlers?

You definitely need to implement .set_type and set the flow handler
the way you do it, and there is hardly anything an irqchip driver can do
to detect that case.

The main issue is that as far as the core code is concerned, the
chained handler is just another flow handler. It just happen to be
provided by another irqchip.

We used to call .set_type early, and a driver like yours would set the
flow corresponding to the trigger of the interrupt. Later, the
secondary irqchip would then call irq_set_chained_handler_and_data(),
which would override the flow with the custom one. Now, we call it much
later, after the custom flow handler has been assigned. Kaboom, we
end-up calling the chained_action handler through one of the normal
flows, instead of going into our special flow.

We *could* make irq_set_handler_locked() check for this condition
though (testing for the chained_action pointer), probably at the cost of
un-inlining it.

I'll try to put something together next week, and see what sticks.

Thanks,

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