From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753752AbdKIQrM (ORCPT ); Thu, 9 Nov 2017 11:47:12 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:54486 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750909AbdKIQrL (ORCPT ); Thu, 9 Nov 2017 11:47:11 -0500 X-Google-Smtp-Source: ABhQp+Rf/oHt+TlSGqcAM3f1rkopaFFbsiiUy4mJlDu+/piwMBrymbieC02YhY20Qrk1oDi4aEhxpg== 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> <41355cbe-e27d-2710-9395-a1c765aa8906@gmail.com> <5a27b82e-1bce-85d7-32dc-ec1ddfac1b57@arm.com> From: Petr Cvek Message-ID: Date: Thu, 9 Nov 2017 17:47:22 +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: <5a27b82e-1bce-85d7-32dc-ec1ddfac1b57@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 9.11.2017 v 15:50 Marc Zyngier napsal(a): > On 08/11/17 13:35, Petr Cvek wrote: >> >> >> 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. > > Hi Petr, > > Can you please give this patch a go (in place of the previous hack)? > > Thanks, > > M. > > From 4092382626c5697e950b424a195be4aadd330c63 Mon Sep 17 00:00:00 2001 > From: Marc Zyngier > Date: Thu, 9 Nov 2017 14:17:59 +0000 > Subject: [PATCH] genirq: Track whether the trigger type has been set > > When requesting a shared interrupt, we assume that the firmware > support code (DT or ACPI) has called irqd_set_trigger_type > already, so that we can retrieve it and check that the requester > is being reasonnable. > > Unfortunately, we still have non-DT, non-ACPI systems around, > and these guys won't call irqd_set_trigger_type before requesting > the interrupt. The consequence is that we fail the request that > would have worked before. > > We can either chase all these use cases (boring), or address it > in core code (easier). Let's have a per-irq_desc flag that > indicates whether irqd_set_trigger_type has been called, and > let's just check it when checking for a shared interrupt. > If it hasn't been set, just take whatever the interrupt > requester asks. > > Fixes: 382bd4de6182 ("genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs") > Cc: stable@vger.kernel.org > Signed-off-by: Marc Zyngier > --- > include/linux/irq.h | 11 ++++++++++- > kernel/irq/manage.c | 13 ++++++++++++- > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/include/linux/irq.h b/include/linux/irq.h > index fda8da7c45e7..73f61eeb152e 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -210,6 +210,7 @@ struct irq_data { > * IRQD_MANAGED_SHUTDOWN - Interrupt was shutdown due to empty affinity > * mask. Applies only to affinity managed irqs. > * IRQD_SINGLE_TARGET - IRQ allows only a single affinity target > + * IRQD_DEFAULT_TRIGGER_SET - Expected trigger already been set > */ > enum { > IRQD_TRIGGER_MASK = 0xf, > @@ -230,6 +231,7 @@ enum { > IRQD_IRQ_STARTED = (1 << 22), > IRQD_MANAGED_SHUTDOWN = (1 << 23), > IRQD_SINGLE_TARGET = (1 << 24), > + IRQD_DEFAULT_TRIGGER_SET = (1 << 25), > }; > > #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors) > @@ -259,18 +261,25 @@ static inline void irqd_mark_affinity_was_set(struct irq_data *d) > __irqd_to_state(d) |= IRQD_AFFINITY_SET; > } > > +static inline bool irqd_trigger_type_was_set(struct irq_data *d) > +{ > + return __irqd_to_state(d) & IRQD_DEFAULT_TRIGGER_SET; > +} > + > static inline u32 irqd_get_trigger_type(struct irq_data *d) > { > return __irqd_to_state(d) & IRQD_TRIGGER_MASK; > } > > /* > - * Must only be called inside irq_chip.irq_set_type() functions. > + * Must only be called inside irq_chip.irq_set_type() functions or > + * from the DT/ACPI setup code. > */ > static inline void irqd_set_trigger_type(struct irq_data *d, u32 type) > { > __irqd_to_state(d) &= ~IRQD_TRIGGER_MASK; > __irqd_to_state(d) |= type & IRQD_TRIGGER_MASK; > + __irqd_to_state(d) |= IRQD_DEFAULT_TRIGGER_SET; > } > > static inline bool irqd_is_level_type(struct irq_data *d) > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index e667912d0e9c..21e04e780be4 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1228,7 +1228,18 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > * set the trigger type must match. Also all must > * agree on ONESHOT. > */ > - unsigned int oldtype = irqd_get_trigger_type(&desc->irq_data); > + unsigned int oldtype; > + > + /* > + * If nobody did set the configuration before, inherit > + * the one provided by the requester. > + */ > + if (irqd_trigger_type_was_set(&desc->irq_data)) { > + oldtype = irqd_get_trigger_type(&desc->irq_data); > + } else { > + oldtype = new->flags & IRQF_TRIGGER_MASK; > + irqd_set_trigger_type(&desc->irq_data, oldtype); > + } > > if (!((old->flags & new->flags) & IRQF_SHARED) || > (oldtype != (new->flags & IRQF_TRIGGER_MASK)) || > Hi, Seems to work, thanks for fix. Tested-by: Petr Cvek