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

[Marc Zyngier]

On 09/08/16 14:20, Jon Hunter wrote:
> 
> On 09/08/16 05:25, John Stultz wrote:
> 
> ...
> 
>> So actually no. We usually call irqd_set_trigger_type() but something
>> still doesn't work.
>>
>> Interestingly, just adding irq_set_irq_type(virq, type); to the top of
>> that block (leaving the rest of the code) also works.
> 
> Interesting. By saving the trigger type during the mapping, we defer
> setting the interrupt type to when the interrupt is requested. So this
> would imply that the interrupt is not being setup as expected when its
> requested.
>  
>>> What is odd, is that the above sequence is only executed if a irq
>>> mapping exists and so really, AFAICT this should not happen. Ie. the irq
>>> descriptor should have been allocated for the mapping to exist. We
>>> should probably warn if this happens.
>>>
>>> Without reverting the above, can you add a print to show the
>>> domain->name, hwirq and virq information if !irq_data? That will confirm
>>> the domain for us.
>>
>> So I put some printk info in (in either case since I'm never seeing
>> the !irq_data case happen):
>>
>> [    1.514217] JDB: virq: 93  hwirq: 74 domain name: msmgpio
>> [    1.838342] JDB: virq: 25  hwirq: 6 domain name: msmgpio
>>
>> Which is odd, looking at:
>>
>> shell@flo:/ $ cat /proc/interrupts
>>            CPU0       CPU1       CPU2       CPU3
>>  16:       1159       1138       1332       1574     GIC-0  18 Edge
>>   gp_timer
>>  25:          0          0          0          0   msmgpio   6 Edge
>>   ekth3500
>> 111:          6          0          0          0     GIC-0  51 Edge
>>   qcom_rpm_ack
>> 112:          0          0          0          0     GIC-0  53 Edge
>>   qcom_rpm_err
>> 113:          0          0          0          0     GIC-0  54 Edge
>>   qcom_rpm_wakeup
>> 114:         48          0          0          0     GIC-0 132 Edge
>>   msm_otg, ci_hdrc_msm
>> 115:        796          0          0          0     GIC-0 130 Level     bam_dma
>> 116:          0          0          0          0     GIC-0 128 Level     bam_dma
>> 117:          0          0          0          0     GIC-0 127 Level     bam_dma
>> 118:       2627          0          0          0     GIC-0 136 Level
>>   mmci-pl18x (cmd)
>> 119:         54          0          0          0     GIC-0 226 Level     i2c_qup
>> 120:         21          0          0          0     GIC-0 183 Level     i2c_qup
>> 122:          0          0          0          0     GIC-0 189 Level     i2c_qup
>> 123:        202          0          0          0     GIC-0 190 Level
>>   msm_serial0
>> 124:          0          0          0          0     GIC-0  70 Edge      smsm
>> 125:          0          0          0          0     GIC-0 121 Edge      smsm
>> 126:          0          0          0          0     GIC-0 236 Edge      smsm
>> 127:          0          0          0          0     GIC-0 169 Edge      smsm
>> 131:          0          0          0          0    pm8xxx 195 Edge
>>   Volume Up
>> 165:          0          0          0          0    pm8xxx 229 Edge
>>   Volume Down
>> 184:          0          0          0          0    pm8xxx  39 Edge
>>   pm8xxx_rtc_alarm
>> 185:          0          0          0          0    pm8xxx  50 Edge
>>   pmic8xxx_pwrkey_release
>> 186:          0          0          0          0    pm8xxx  51 Edge
>>   pmic8xxx_pwrkey_press
>> IPI0:          0          1          1          1  CPU wakeup interrupts
>> IPI1:          0          0          0          0  Timer broadcast interrupts
>> IPI2:        944        539       1015        529  Rescheduling interrupts
>> IPI3:          1          4          6          4  Function call interrupts
>> IPI4:          0          0          0          0  CPU stop interrupts
>> IPI5:          0          0          0          0  IRQ work interrupts
>> IPI6:          0          0          0          0  completion interrupts
>> Err:          0
>>
>> Since 25 maps to the ekth3500 (touch panel, which is still working
>> fine), but 93/74 doesn't seem to map to anything, and the problematic
>> irqs are the volume keys 195/229 and power keys 50/51.
> 
> So looking at the DT source, I believe that hwirq 74 (virq 93) is the
> problem. This is the parent interrupt from the pm8xxx to the apq8064
> if it is not requested then the type is not set. It seems that for
> parent interrupts these are not typically requested, but enabled when
> an irqchip is chained.
> 
> To confirm and for testing purposes I am curious if this works ...
> 
> 	if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> 		irq_data = irq_get_irq_data(virq);
> 		if (!irq_data)
> 			return 0;
> 
> -		irqd_set_trigger_type(irq_data, type);
> + 		if (hwirq == 74)
> +			irq_set_irq_type(virq, type);
> +		else
> +			irqd_set_trigger_type(irq_data, type);
> 		return virq;
> 	}
> 
> If that works, then does the following also work (without the above) ...
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index b4c1bc7c9ca2..e111b72e3162 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -824,6 +824,7 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
>                 irq_settings_set_norequest(desc);
>                 irq_settings_set_nothread(desc);
>                 desc->action = &chained_action;
> +               __irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
>                 irq_startup(desc, true);
>         }
>  }
> 
> It looks like there is a path for parent interrupts where the type
> is not getting set. If the above works then we can discuss with Thomas
> and Marc on the correct fix.

This definitely looks like an something that is worth a patch anyway, as
I otherwise don't see how we configure cascaded interrupts with the new
deferred scheme.

Thanks,

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