lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 8 Nov 2017 14:35:47 +0100 From: Petr Cvek <petrcvekcz@...il.com> To: Marc Zyngier <marc.zyngier@....com>, hdegoede@...hat.com, tglx@...utronix.de Cc: linux-kernel@...r.kernel.org, philipp.zabel@...il.com Subject: Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs Dne 8.11.2017 v 14:11 Marc Zyngier napsal(a): > On 08/11/17 13:09, Marc Zyngier wrote: >> 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. Hi, yeah it works now and the assert is oldtype == 3 and old->flags == 3 so neither versions of condition won't trigger goto mismatch. >> >> 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); > > Of course, this should be > > irqd_set_trigger_type(d, trig); > and (in case anyone other want to try) #include <linux/irq.h> >> + >> + return IRQF_SHARED | trig; >> } >> >> static struct device *dev; >> >> > > M. > cheers, Petr
Powered by blists - more mailing lists