From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752451AbdKHNJg (ORCPT ); Wed, 8 Nov 2017 08:09:36 -0500 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:33064 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751633AbdKHNJf (ORCPT ); Wed, 8 Nov 2017 08:09:35 -0500 Subject: Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs To: Petr Cvek , hdegoede@redhat.com, tglx@linutronix.de Cc: linux-kernel@vger.kernel.org, philipp.zabel@gmail.com References: <1415e4e2-3ee9-bc31-374c-ba64a506a622@gmail.com> From: Marc Zyngier Organization: ARM Ltd Message-ID: <4a6fb044-68b2-6564-b48d-2a6cefcb4655@arm.com> Date: Wed, 8 Nov 2017 13:09:32 +0000 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: <1415e4e2-3ee9-bc31-374c-ba64a506a622@gmail.com> Content-Type: text/plain; charset=iso-8859-2 Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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...