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

[Jon Hunter]

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.

Cheers
Jon

-- 
nvpublic