From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752534AbdKHNfk (ORCPT ); Wed, 8 Nov 2017 08:35:40 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:43324 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752376AbdKHNfj (ORCPT ); Wed, 8 Nov 2017 08:35:39 -0500 X-Google-Smtp-Source: ABhQp+TMey9D7B34av6lLpavV8HHlg6QDOxGKlpuUvU71TGxLzGR51B0t80M4csJyPICtNaeXP5oTQ== Subject: Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs To: Marc Zyngier , hdegoede@redhat.com, tglx@linutronix.de Cc: linux-kernel@vger.kernel.org, philipp.zabel@gmail.com References: <1415e4e2-3ee9-bc31-374c-ba64a506a622@gmail.com> <4a6fb044-68b2-6564-b48d-2a6cefcb4655@arm.com> <7a3a89f5-41ea-6c15-72fc-0f62bf748074@arm.com> From: Petr Cvek Message-ID: <41355cbe-e27d-2710-9395-a1c765aa8906@gmail.com> Date: Wed, 8 Nov 2017 14:35:47 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <7a3a89f5-41ea-6c15-72fc-0f62bf748074@arm.com> Content-Type: text/plain; charset=iso-8859-2; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> + >> + return IRQF_SHARED | trig; >> } >> >> static struct device *dev; >> >> > > M. > cheers, Petr