Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

[Marc Zyngier <marc.zyngier@arm.com>]

On 07/11/17 23:41, Petr Cvek wrote:
> Hello,
> 
> Commit 382bd4de61827 ("genirq: Use irqd_get_trigger_type to compare the 
> trigger type for shared IRQs") causes a regression for pda-power driver 
> and Magician machine (mach-pxa/magician.c).
> 
>    unsigned int oldtype = irqd_get_trigger_type(&desc->irq_data);
> 
>    ... assert:
> 	oldtype == 0	//new code
> 	old->flags == 0x83	//old code
> 	new->flags & IRQF_TRIGGER_MASK == 3
> 
>    if (!((old->flags & new->flags) & IRQF_SHARED) ||
> 	(oldtype != (new->flags & IRQF_TRIGGER_MASK)) ||
> 	((old->flags ^ new->flags) & IRQF_ONESHOT))
> 		goto mismatch;
> 
> The assert shows the new code will trigger the jump for "mismatch" error 
> the old variant of code works fine.
> 
> The case for Magician machine is specific as the same interrupt line is 
> requested twice from the same driver (pda-power). But it still could 
> point to some hidden problem in the IRQ setup code.
> 
> I wasn't able to trace the code when desc->irq_data is getting set. The 
> flags values should be (as with old->flags):
> 
> 	IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
> 
> It could be a problem with a weird IRQ sharing in magician code, but 
> it's still failing the driver and the start of the charging system.
> 
> IRQ definition in arch/arm/mach-pxa/magician.c looks like this:
> 
> static struct resource power_supply_resources[] = {
> 	[0] = {
> 		.name	= "ac",
> 		.flags	= IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
> 			IORESOURCE_IRQ_LOWEDGE,
> 		.start	= IRQ_MAGICIAN_VBUS,
> 		.end	= IRQ_MAGICIAN_VBUS,
> 	},
> 	[1] = {
> 		.name	= "usb",
> 		.flags	= IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
> 			IORESOURCE_IRQ_LOWEDGE,
> 		.start	= IRQ_MAGICIAN_VBUS,
> 		.end	= IRQ_MAGICIAN_VBUS,
> 	},
> };
> 
> and IRQ requests from drivers/power/supply/pda_power.c look like this:
> 
> if (ac_irq) {
> 	ret = request_irq(ac_irq->start, power_changed_isr,
> 			  get_irq_flags(ac_irq), ac_irq->name,
> 			  pda_psy_ac);
> ...
> if (usb_irq) {
> 	ret = request_irq(usb_irq->start, power_changed_isr,
> 			  get_irq_flags(usb_irq),
> 			  usb_irq->name, pda_psy_usb);
> 
> I could rewrite the part in the magician code so it would use only one 
> interrupt, but it doesn't solve why oldtype == 0 problem.

Yeah, this is a pretty ugly corner case that crops up because we more
and more assume things like DT, which is going to configure the expected
trigger type ahead of the interrupt being requested... Of course, PXA is
not converted to DT, and unlikely to ever be.

Could you please give the following hack a go and let us know if it
solves your problem? If it does, I'll think of a more generic solution.

Thanks,

	M.

diff --git a/drivers/power/supply/pda_power.c b/drivers/power/supply/pda_power.c
index 922a86787c5c..a32ae240ef7d 100644
--- a/drivers/power/supply/pda_power.c
+++ b/drivers/power/supply/pda_power.c
@@ -24,7 +24,15 @@
 
 static inline unsigned int get_irq_flags(struct resource *res)
 {
-	return IRQF_SHARED | (res->flags & IRQF_TRIGGER_MASK);
+	struct irq_data *d = irq_get_irq_data(res->start);
+	unsigned int trig = res->flags & IRQF_TRIGGER_MASK;
+
+	BUG_ON(!d);
+
+	if (!irqd_get_trigger_type(d))
+		irqd_set_trigger_type(trig);
+
+	return IRQF_SHARED | trig;
 }
 
 static struct device *dev;


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