regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

[Petr Cvek]

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.

Best regards,
Petr

Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

[Marc Zyngier]

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...

Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

[Marc Zyngier]

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.
> 
> 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);

> +
> +	return IRQF_SHARED | trig;
>  }
>  
>  static struct device *dev;
> 
> 

	M.
-- 
Jazz is not dead. It just smells funny...

Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

[Petr Cvek]

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

Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

[Marc Zyngier]

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.
Thanks for having tested it. Now we need to teach the core code that the
lack of a trigger means that we need to blindly trust the requester. But
it'd be good if we could do it only in the legacy case...

Head scratching time...

	M.
-- 
Jazz is not dead. It just smells funny...

Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

[Marc Zyngier]

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@arm.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@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.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)) ||
-- 
2.14.2

-- 
Jazz is not dead. It just smells funny...

Re: regression in irq sharing caused by genirq: Use irqd_get_trigger_type to compare the trigger type for shared IRQs

[Petr Cvek]

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@arm.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@vger.kernel.org
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.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@gmail.com>