Message-ID: <f173395a-56d9-8963-a8ad-1379085169c9@gmail.com>
Date:   Thu, 9 Nov 2017 17:47:22 +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 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 <marc.zyngier@....com>
> 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@...r.kernel.org
> Signed-off-by: Marc Zyngier <marc.zyngier@....com>
> ---
>   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 <petrcvekcz@...il.com>

Powered by blists - more mailing lists