BUG? genirq: irq 14 uses trigger mode 8; requested 0

BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Mika Westerberg]

Hi,

I started seeing following messages on Intel Broxton when the
pinctrl/GPIO driver [1] loads:

  [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0

The driver shares interrupt with other GPIO "communities" or banks so it
uses request_irq() instead of irq_set_chained_handler_and_data(). The
driver does not specify IRQ flags as those come from ACPI resources.

This started happen after commit 4b357daed698 ("genirq: Look-up trigger
type if not specified by caller").

I think this is what happens:

  1. ACPI platform sets up the interrupt according what is in the _CRS
  of the GPIO device. This ends up setting trigger type for irq_data of
  the irq.

  2. First GPIO device is found and the driver calls request_irq() which
  calls __setup_irq() where shared == 0. 

  3. Since new->flags is read back from irq_data we call __irq_set_trigger()
  passing the flags.

  4. The parent IRQ chip, IO-APIC, does not have ->irq_set_type callback
  so __irq_set_trigger() never calls irq_settings_set_trigger_mask() for
  the desciptor.

  5. The second GPIO device is found and this time shared == 1 so we
  end up comparing nmsk with omsk where nmsk was read from irq_data
  and omsk is read using irq_settings_get_trigger_mask().

  6. Because we never called irq_settings_set_trigger_mask() for the
  descriptor, omsk is 0 and we print out a warning:

  [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0

If I revert commit 4b357daed698 the warning goes away.

Do you have any ideas how to get rid of the warning properly?

Thanks in advance.

[1] drivers/pinctrl/intel/pinctrl-intel.c

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Jon Hunter]

Hi Mika,

On 01/11/16 13:02, Mika Westerberg wrote:
> Hi,
>
> I started seeing following messages on Intel Broxton when the
> pinctrl/GPIO driver [1] loads:
>
>   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
>
> The driver shares interrupt with other GPIO "communities" or banks so it
> uses request_irq() instead of irq_set_chained_handler_and_data(). The
> driver does not specify IRQ flags as those come from ACPI resources.
>
> This started happen after commit 4b357daed698 ("genirq: Look-up trigger
> type if not specified by caller").
>
> I think this is what happens:
>
>   1. ACPI platform sets up the interrupt according what is in the _CRS
>   of the GPIO device. This ends up setting trigger type for irq_data of
>   the irq.
>
>   2. First GPIO device is found and the driver calls request_irq() which
>   calls __setup_irq() where shared == 0.
>
>   3. Since new->flags is read back from irq_data we call __irq_set_trigger()
>   passing the flags.
>
>   4. The parent IRQ chip, IO-APIC, does not have ->irq_set_type callback
>   so __irq_set_trigger() never calls irq_settings_set_trigger_mask() for
>   the desciptor.
 >
>   5. The second GPIO device is found and this time shared == 1 so we
>   end up comparing nmsk with omsk where nmsk was read from irq_data
>   and omsk is read using irq_settings_get_trigger_mask().
>
>   6. Because we never called irq_settings_set_trigger_mask() for the
>   descriptor, omsk is 0 and we print out a warning:
>
>   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
>
> If I revert commit 4b357daed698 the warning goes away.
>
> Do you have any ideas how to get rid of the warning properly?

May be I am misunderstanding something here, but if the parent does not 
have a ->irq_set_type callback, then it would seem that the type for the 
interrupt should be not specified/set in the ACPI _CRS for the GPIO 
device, right?

Thanks for the detailed description.

Cheers
Jon

-- 
nvpublic

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Mika Westerberg]

On Tue, Nov 01, 2016 at 02:24:38PM +0000, Jon Hunter wrote:
> Hi Mika,
> 
> On 01/11/16 13:02, Mika Westerberg wrote:
> > Hi,
> > 
> > I started seeing following messages on Intel Broxton when the
> > pinctrl/GPIO driver [1] loads:
> > 
> >   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
> > 
> > The driver shares interrupt with other GPIO "communities" or banks so it
> > uses request_irq() instead of irq_set_chained_handler_and_data(). The
> > driver does not specify IRQ flags as those come from ACPI resources.
> > 
> > This started happen after commit 4b357daed698 ("genirq: Look-up trigger
> > type if not specified by caller").
> > 
> > I think this is what happens:
> > 
> >   1. ACPI platform sets up the interrupt according what is in the _CRS
> >   of the GPIO device. This ends up setting trigger type for irq_data of
> >   the irq.
> > 
> >   2. First GPIO device is found and the driver calls request_irq() which
> >   calls __setup_irq() where shared == 0.
> > 
> >   3. Since new->flags is read back from irq_data we call __irq_set_trigger()
> >   passing the flags.
> > 
> >   4. The parent IRQ chip, IO-APIC, does not have ->irq_set_type callback
> >   so __irq_set_trigger() never calls irq_settings_set_trigger_mask() for
> >   the desciptor.
> >
> >   5. The second GPIO device is found and this time shared == 1 so we
> >   end up comparing nmsk with omsk where nmsk was read from irq_data
> >   and omsk is read using irq_settings_get_trigger_mask().
> > 
> >   6. Because we never called irq_settings_set_trigger_mask() for the
> >   descriptor, omsk is 0 and we print out a warning:
> > 
> >   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
> > 
> > If I revert commit 4b357daed698 the warning goes away.
> > 
> > Do you have any ideas how to get rid of the warning properly?
> 
> May be I am misunderstanding something here, but if the parent does not have
> a ->irq_set_type callback, then it would seem that the type for the
> interrupt should be not specified/set in the ACPI _CRS for the GPIO device,
> right?

Not sure.

Why the parent driver (IO-APIC) does not have ->irq_set_type callback is
beyond me. I guess it might have something to do with the IRQ hierarchy
domains it is part of.

When the ACPI core parses _CRS for the GPIO device it calls
acpi_register_gsi() with the triggering flags from _CRS and that ends up
calling acpi_register_gsi_ioapic() that programs the hardware
accordingly. So we definitely need to have the type in _CRS.

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Mika Westerberg]

On Tue, Nov 01, 2016 at 04:44:00PM +0200, Mika Westerberg wrote:
> On Tue, Nov 01, 2016 at 02:24:38PM +0000, Jon Hunter wrote:
> > Hi Mika,
> > 
> > On 01/11/16 13:02, Mika Westerberg wrote:
> > > Hi,
> > > 
> > > I started seeing following messages on Intel Broxton when the
> > > pinctrl/GPIO driver [1] loads:
> > > 
> > >   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
> > > 
> > > The driver shares interrupt with other GPIO "communities" or banks so it
> > > uses request_irq() instead of irq_set_chained_handler_and_data(). The
> > > driver does not specify IRQ flags as those come from ACPI resources.
> > > 
> > > This started happen after commit 4b357daed698 ("genirq: Look-up trigger
> > > type if not specified by caller").
> > > 
> > > I think this is what happens:
> > > 
> > >   1. ACPI platform sets up the interrupt according what is in the _CRS
> > >   of the GPIO device. This ends up setting trigger type for irq_data of
> > >   the irq.
> > > 
> > >   2. First GPIO device is found and the driver calls request_irq() which
> > >   calls __setup_irq() where shared == 0.
> > > 
> > >   3. Since new->flags is read back from irq_data we call __irq_set_trigger()
> > >   passing the flags.
> > > 
> > >   4. The parent IRQ chip, IO-APIC, does not have ->irq_set_type callback
> > >   so __irq_set_trigger() never calls irq_settings_set_trigger_mask() for
> > >   the desciptor.
> > >
> > >   5. The second GPIO device is found and this time shared == 1 so we
> > >   end up comparing nmsk with omsk where nmsk was read from irq_data
> > >   and omsk is read using irq_settings_get_trigger_mask().
> > > 
> > >   6. Because we never called irq_settings_set_trigger_mask() for the
> > >   descriptor, omsk is 0 and we print out a warning:
> > > 
> > >   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
> > > 
> > > If I revert commit 4b357daed698 the warning goes away.
> > > 
> > > Do you have any ideas how to get rid of the warning properly?
> > 
> > May be I am misunderstanding something here, but if the parent does not have
> > a ->irq_set_type callback, then it would seem that the type for the
> > interrupt should be not specified/set in the ACPI _CRS for the GPIO device,
> > right?
> 
> Not sure.
> 
> Why the parent driver (IO-APIC) does not have ->irq_set_type callback is
> beyond me. I guess it might have something to do with the IRQ hierarchy
> domains it is part of.
> 
> When the ACPI core parses _CRS for the GPIO device it calls
> acpi_register_gsi() with the triggering flags from _CRS and that ends up
> calling acpi_register_gsi_ioapic() that programs the hardware
> accordingly. So we definitely need to have the type in _CRS.

Jon, Marc, 

Do you have any suggestions how to fix this other than reverting
4b357daed698 ("genirq: Look-up trigger type if not specified by
caller")?

Before that commit everything works fine.

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Jon Hunter]

Hi Mika,

On 07/11/16 11:49, Mika Westerberg wrote:
> On Tue, Nov 01, 2016 at 04:44:00PM +0200, Mika Westerberg wrote:
>> On Tue, Nov 01, 2016 at 02:24:38PM +0000, Jon Hunter wrote:
>>> Hi Mika,
>>>
>>> On 01/11/16 13:02, Mika Westerberg wrote:
>>>> Hi,
>>>>
>>>> I started seeing following messages on Intel Broxton when the
>>>> pinctrl/GPIO driver [1] loads:
>>>>
>>>>   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
>>>>
>>>> The driver shares interrupt with other GPIO "communities" or banks so it
>>>> uses request_irq() instead of irq_set_chained_handler_and_data(). The
>>>> driver does not specify IRQ flags as those come from ACPI resources.
>>>>
>>>> This started happen after commit 4b357daed698 ("genirq: Look-up trigger
>>>> type if not specified by caller").
>>>>
>>>> I think this is what happens:
>>>>
>>>>   1. ACPI platform sets up the interrupt according what is in the _CRS
>>>>   of the GPIO device. This ends up setting trigger type for irq_data of
>>>>   the irq.
>>>>
>>>>   2. First GPIO device is found and the driver calls request_irq() which
>>>>   calls __setup_irq() where shared == 0.
>>>>
>>>>   3. Since new->flags is read back from irq_data we call __irq_set_trigger()
>>>>   passing the flags.
>>>>
>>>>   4. The parent IRQ chip, IO-APIC, does not have ->irq_set_type callback
>>>>   so __irq_set_trigger() never calls irq_settings_set_trigger_mask() for
>>>>   the desciptor.
>>>>
>>>>   5. The second GPIO device is found and this time shared == 1 so we
>>>>   end up comparing nmsk with omsk where nmsk was read from irq_data
>>>>   and omsk is read using irq_settings_get_trigger_mask().
>>>>
>>>>   6. Because we never called irq_settings_set_trigger_mask() for the
>>>>   descriptor, omsk is 0 and we print out a warning:
>>>>
>>>>   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
>>>>
>>>> If I revert commit 4b357daed698 the warning goes away.
>>>>
>>>> Do you have any ideas how to get rid of the warning properly?
>>>
>>> May be I am misunderstanding something here, but if the parent does not have
>>> a ->irq_set_type callback, then it would seem that the type for the
>>> interrupt should be not specified/set in the ACPI _CRS for the GPIO device,
>>> right?
>>
>> Not sure.
>>
>> Why the parent driver (IO-APIC) does not have ->irq_set_type callback is
>> beyond me. I guess it might have something to do with the IRQ hierarchy
>> domains it is part of.
>>
>> When the ACPI core parses _CRS for the GPIO device it calls
>> acpi_register_gsi() with the triggering flags from _CRS and that ends up
>> calling acpi_register_gsi_ioapic() that programs the hardware
>> accordingly. So we definitely need to have the type in _CRS.
> 
> Jon, Marc, 
> 
> Do you have any suggestions how to fix this other than reverting
> 4b357daed698 ("genirq: Look-up trigger type if not specified by
> caller")?
> 
> Before that commit everything works fine.

Sorry I forgot to respond again last week.

Marc, what do you think? Feels to me that this parent should have a
->irq_set_type callback even if it is just a dummy one and does nothing.

Cheers
Jon

-- 
nvpublic

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Marc Zyngier]

On 07/11/16 13:32, Jon Hunter wrote:
> Hi Mika,
> 
> On 07/11/16 11:49, Mika Westerberg wrote:
>> On Tue, Nov 01, 2016 at 04:44:00PM +0200, Mika Westerberg wrote:
>>> On Tue, Nov 01, 2016 at 02:24:38PM +0000, Jon Hunter wrote:
>>>> Hi Mika,
>>>>
>>>> On 01/11/16 13:02, Mika Westerberg wrote:
>>>>> Hi,
>>>>>
>>>>> I started seeing following messages on Intel Broxton when the
>>>>> pinctrl/GPIO driver [1] loads:
>>>>>
>>>>>   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
>>>>>
>>>>> The driver shares interrupt with other GPIO "communities" or banks so it
>>>>> uses request_irq() instead of irq_set_chained_handler_and_data(). The
>>>>> driver does not specify IRQ flags as those come from ACPI resources.
>>>>>
>>>>> This started happen after commit 4b357daed698 ("genirq: Look-up trigger
>>>>> type if not specified by caller").
>>>>>
>>>>> I think this is what happens:
>>>>>
>>>>>   1. ACPI platform sets up the interrupt according what is in the _CRS
>>>>>   of the GPIO device. This ends up setting trigger type for irq_data of
>>>>>   the irq.
>>>>>
>>>>>   2. First GPIO device is found and the driver calls request_irq() which
>>>>>   calls __setup_irq() where shared == 0.
>>>>>
>>>>>   3. Since new->flags is read back from irq_data we call __irq_set_trigger()
>>>>>   passing the flags.
>>>>>
>>>>>   4. The parent IRQ chip, IO-APIC, does not have ->irq_set_type callback
>>>>>   so __irq_set_trigger() never calls irq_settings_set_trigger_mask() for
>>>>>   the desciptor.
>>>>>
>>>>>   5. The second GPIO device is found and this time shared == 1 so we
>>>>>   end up comparing nmsk with omsk where nmsk was read from irq_data
>>>>>   and omsk is read using irq_settings_get_trigger_mask().
>>>>>
>>>>>   6. Because we never called irq_settings_set_trigger_mask() for the
>>>>>   descriptor, omsk is 0 and we print out a warning:
>>>>>
>>>>>   [    0.645786] genirq: irq 14 uses trigger mode 8; requested 0
>>>>>
>>>>> If I revert commit 4b357daed698 the warning goes away.
>>>>>
>>>>> Do you have any ideas how to get rid of the warning properly?
>>>>
>>>> May be I am misunderstanding something here, but if the parent does not have
>>>> a ->irq_set_type callback, then it would seem that the type for the
>>>> interrupt should be not specified/set in the ACPI _CRS for the GPIO device,
>>>> right?
>>>
>>> Not sure.
>>>
>>> Why the parent driver (IO-APIC) does not have ->irq_set_type callback is
>>> beyond me. I guess it might have something to do with the IRQ hierarchy
>>> domains it is part of.
>>>
>>> When the ACPI core parses _CRS for the GPIO device it calls
>>> acpi_register_gsi() with the triggering flags from _CRS and that ends up
>>> calling acpi_register_gsi_ioapic() that programs the hardware
>>> accordingly. So we definitely need to have the type in _CRS.
>>
>> Jon, Marc, 
>>
>> Do you have any suggestions how to fix this other than reverting
>> 4b357daed698 ("genirq: Look-up trigger type if not specified by
>> caller")?
>>
>> Before that commit everything works fine.
> 
> Sorry I forgot to respond again last week.
> 
> Marc, what do you think? Feels to me that this parent should have a
> ->irq_set_type callback even if it is just a dummy one and does nothing.

Sorry, missed this discussion entirely, as I was on the other side of
the world. Not having a irq_set_type definitely seems odd (and 
configuring the trigger outside of the IRQ framework is quite ugly).

Mika, can you please try the following (which is fully untested)?

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 48e6d84..822a6b8 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1868,6 +1868,15 @@ static int ioapic_set_affinity(struct irq_data *irq_data,
 	return ret;
 }
 
+static int ioapic_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	/*
+	 * The IOAPIC has already been programmed behind our back,
+	 * just pretend it all went OK, and too bad if it didn't.
+	 */
+	return 0;
+}
+
 static struct irq_chip ioapic_chip __read_mostly = {
 	.name			= "IO-APIC",
 	.irq_startup		= startup_ioapic_irq,
@@ -1876,6 +1885,7 @@ static struct irq_chip ioapic_chip __read_mostly = {
 	.irq_ack		= irq_chip_ack_parent,
 	.irq_eoi		= ioapic_ack_level,
 	.irq_set_affinity	= ioapic_set_affinity,
+	.irq_set_type		= ioapic_set_type,
 	.flags			= IRQCHIP_SKIP_SET_WAKE,
 };
 

Thanks,

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

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Mika Westerberg]

On Mon, Nov 07, 2016 at 02:40:47PM +0000, Marc Zyngier wrote:
> Sorry, missed this discussion entirely, as I was on the other side of
> the world. Not having a irq_set_type definitely seems odd (and 
> configuring the trigger outside of the IRQ framework is quite ugly).
> 
> Mika, can you please try the following (which is fully untested)?
> 
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 48e6d84..822a6b8 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1868,6 +1868,15 @@ static int ioapic_set_affinity(struct irq_data *irq_data,
>  	return ret;
>  }
>  
> +static int ioapic_set_type(struct irq_data *data, unsigned int flow_type)
> +{
> +	/*
> +	 * The IOAPIC has already been programmed behind our back,
> +	 * just pretend it all went OK, and too bad if it didn't.
> +	 */
> +	return 0;
> +}
> +
>  static struct irq_chip ioapic_chip __read_mostly = {
>  	.name			= "IO-APIC",
>  	.irq_startup		= startup_ioapic_irq,
> @@ -1876,6 +1885,7 @@ static struct irq_chip ioapic_chip __read_mostly = {
>  	.irq_ack		= irq_chip_ack_parent,
>  	.irq_eoi		= ioapic_ack_level,
>  	.irq_set_affinity	= ioapic_set_affinity,
> +	.irq_set_type		= ioapic_set_type,
>  	.flags			= IRQCHIP_SKIP_SET_WAKE,
>  };

Thanks! This fixes the problem for me.

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Jon Hunter]

On 07/11/16 14:59, Mika Westerberg wrote:
> On Mon, Nov 07, 2016 at 02:40:47PM +0000, Marc Zyngier wrote:
>> Sorry, missed this discussion entirely, as I was on the other side of
>> the world. Not having a irq_set_type definitely seems odd (and 
>> configuring the trigger outside of the IRQ framework is quite ugly).
>>
>> Mika, can you please try the following (which is fully untested)?
>>
>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>> index 48e6d84..822a6b8 100644
>> --- a/arch/x86/kernel/apic/io_apic.c
>> +++ b/arch/x86/kernel/apic/io_apic.c
>> @@ -1868,6 +1868,15 @@ static int ioapic_set_affinity(struct irq_data *irq_data,
>>  	return ret;
>>  }
>>  
>> +static int ioapic_set_type(struct irq_data *data, unsigned int flow_type)
>> +{
>> +	/*
>> +	 * The IOAPIC has already been programmed behind our back,
>> +	 * just pretend it all went OK, and too bad if it didn't.
>> +	 */
>> +	return 0;
>> +}
>> +
>>  static struct irq_chip ioapic_chip __read_mostly = {
>>  	.name			= "IO-APIC",
>>  	.irq_startup		= startup_ioapic_irq,
>> @@ -1876,6 +1885,7 @@ static struct irq_chip ioapic_chip __read_mostly = {
>>  	.irq_ack		= irq_chip_ack_parent,
>>  	.irq_eoi		= ioapic_ack_level,
>>  	.irq_set_affinity	= ioapic_set_affinity,
>> +	.irq_set_type		= ioapic_set_type,
>>  	.flags			= IRQCHIP_SKIP_SET_WAKE,
>>  };
> 
> Thanks! This fixes the problem for me.

Good to know. The more I think about this, the more I wonder if we
should have a dummy 'set_type' as this is just hiding the issue. If the
original warning does not impact the overall behaviour (ie. interrupts
still working) then may be it is legitimate as far as gen-irq is concerned?

Jon

-- 
nvpublic

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Thomas Gleixner]

On Mon, 7 Nov 2016, Jon Hunter wrote:
> On 07/11/16 14:59, Mika Westerberg wrote:
> > On Mon, Nov 07, 2016 at 02:40:47PM +0000, Marc Zyngier wrote:
> >> Sorry, missed this discussion entirely, as I was on the other side of
> >> the world. Not having a irq_set_type definitely seems odd (and 
> >> configuring the trigger outside of the IRQ framework is quite ugly).
> >>
> >> Mika, can you please try the following (which is fully untested)?
> >>
> >> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> >> index 48e6d84..822a6b8 100644
> >> --- a/arch/x86/kernel/apic/io_apic.c
> >> +++ b/arch/x86/kernel/apic/io_apic.c
> >> @@ -1868,6 +1868,15 @@ static int ioapic_set_affinity(struct irq_data *irq_data,
> >>  	return ret;
> >>  }
> >>  
> >> +static int ioapic_set_type(struct irq_data *data, unsigned int flow_type)
> >> +{
> >> +	/*
> >> +	 * The IOAPIC has already been programmed behind our back,
> >> +	 * just pretend it all went OK, and too bad if it didn't.
> >> +	 */
> >> +	return 0;
> >> +}
> >> +
> >>  static struct irq_chip ioapic_chip __read_mostly = {
> >>  	.name			= "IO-APIC",
> >>  	.irq_startup		= startup_ioapic_irq,
> >> @@ -1876,6 +1885,7 @@ static struct irq_chip ioapic_chip __read_mostly = {
> >>  	.irq_ack		= irq_chip_ack_parent,
> >>  	.irq_eoi		= ioapic_ack_level,
> >>  	.irq_set_affinity	= ioapic_set_affinity,
> >> +	.irq_set_type		= ioapic_set_type,
> >>  	.flags			= IRQCHIP_SKIP_SET_WAKE,
> >>  };
> > 
> > Thanks! This fixes the problem for me.
> 
> Good to know. The more I think about this, the more I wonder if we
> should have a dummy 'set_type' as this is just hiding the issue. If the
> original warning does not impact the overall behaviour (ie. interrupts
> still working) then may be it is legitimate as far as gen-irq is concerned?

No. We don't want a dummy set_type to paper over the issue. As Mika
correctly figured out that the ioapic is already configured, but it does
not propagate it to the irq descriptor. That wants to be fixed. I'll have a
look.

Thanks,

	tglx

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Thomas Gleixner]

On Mon, 7 Nov 2016, Thomas Gleixner wrote:
> No. We don't want a dummy set_type to paper over the issue. As Mika
> correctly figured out that the ioapic is already configured, but it does
> not propagate it to the irq descriptor. That wants to be fixed. I'll have a
> look.

Actually that's pointless. The ioapic code sets the type in irqdata. The
type storage in the irq descriptor is a historical left over and want's to
be removed. The last forgotten user of the type data in the irq descriptor
is __setup_irq().... Fix below.

Thanks,

	tglx

8<---------------------------
Subject: genirq: Use irq type from irqdata instead of irqdesc
From: Thomas Gleixner <tglx@linutronix.de>
Date: Mon, 07 Nov 2016 19:57:00 +0100

The type flags in the irq descriptor are there for historical reasons and
only updated via irq_modify_status() or irq_set_type(). Both functions also
update the type flags in irqdata. __setup_irq() is the only left over user
of the type flags in the irq descriptor.

If __setup_irq() is called with empty irq type flags, then the type flags
are retrieved from irqdata. If an interrupt is shared, then the type flags
are compared with the type flags stored in the irq descriptor. 

On x86 the ioapic does not have a irq_set_type() callback because the type
is defined in the BIOS tables and cannot be changed. The type is stored in
irqdata at setup time without updating the type data in the irq
descriptor. As a result the comparison described above fails.

There is no point in updating the irq descriptor flags because the only
relevant storage is irqdata. Use the type flags from irqdata for both
retrieval and comparison in __setup_irq() instead.

Aside of that the print out in case of non matching type flags has the old
and new type flags arguments flipped. Fix that as well.

For correctness sake the flags stored in the irq descriptor should be
removed, but this is beyond the scope of this bugfix and will be done in a
later patch.

Fixes: 4b357daed698 ("genirq: Look-up trigger type if not specified by caller")
Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/manage.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1341,12 +1341,12 @@ static int
 
 	} else if (new->flags & IRQF_TRIGGER_MASK) {
 		unsigned int nmsk = new->flags & IRQF_TRIGGER_MASK;
-		unsigned int omsk = irq_settings_get_trigger_mask(desc);
+		unsigned int omsk = irqd_get_trigger_type(&desc->irq_data);
 
 		if (nmsk != omsk)
 			/* hope the handler works with current  trigger mode */
 			pr_warn("irq %d uses trigger mode %u; requested %u\n",
-				irq, nmsk, omsk);
+				irq, omsk, nmsk);
 	}
 
 	*old_ptr = new;

Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0

[Mika Westerberg]

On Mon, Nov 07, 2016 at 08:29:03PM +0100, Thomas Gleixner wrote:
> Subject: genirq: Use irq type from irqdata instead of irqdesc
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Mon, 07 Nov 2016 19:57:00 +0100
> 
> The type flags in the irq descriptor are there for historical reasons and
> only updated via irq_modify_status() or irq_set_type(). Both functions also
> update the type flags in irqdata. __setup_irq() is the only left over user
> of the type flags in the irq descriptor.
> 
> If __setup_irq() is called with empty irq type flags, then the type flags
> are retrieved from irqdata. If an interrupt is shared, then the type flags
> are compared with the type flags stored in the irq descriptor. 
> 
> On x86 the ioapic does not have a irq_set_type() callback because the type
> is defined in the BIOS tables and cannot be changed. The type is stored in
> irqdata at setup time without updating the type data in the irq
> descriptor. As a result the comparison described above fails.
> 
> There is no point in updating the irq descriptor flags because the only
> relevant storage is irqdata. Use the type flags from irqdata for both
> retrieval and comparison in __setup_irq() instead.
> 
> Aside of that the print out in case of non matching type flags has the old
> and new type flags arguments flipped. Fix that as well.
> 
> For correctness sake the flags stored in the irq descriptor should be
> removed, but this is beyond the scope of this bugfix and will be done in a
> later patch.
> 
> Fixes: 4b357daed698 ("genirq: Look-up trigger type if not specified by caller")
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>

This fixes it, thanks a lot!

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[tip:irq/urgent] genirq: Use irq type from irqdata instead of irqdesc

[tip-bot for Thomas Gleixner]

Commit-ID:  7ee7e87dfb158e79019ea1d5ea1b0e6f2bc93ee4
Gitweb:     http://git.kernel.org/tip/7ee7e87dfb158e79019ea1d5ea1b0e6f2bc93ee4
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 7 Nov 2016 19:57:00 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 8 Nov 2016 15:15:19 +0100

genirq: Use irq type from irqdata instead of irqdesc

The type flags in the irq descriptor are there for historical reasons and
only updated via irq_modify_status() or irq_set_type(). Both functions also
update the type flags in irqdata. __setup_irq() is the only left over user
of the type flags in the irq descriptor.

If __setup_irq() is called with empty irq type flags, then the type flags
are retrieved from irqdata. If an interrupt is shared, then the type flags
are compared with the type flags stored in the irq descriptor. 

On x86 the ioapic does not have a irq_set_type() callback because the type
is defined in the BIOS tables and cannot be changed. The type is stored in
irqdata at setup time without updating the type data in the irq
descriptor. As a result the comparison described above fails.

There is no point in updating the irq descriptor flags because the only
relevant storage is irqdata. Use the type flags from irqdata for both
retrieval and comparison in __setup_irq() instead.

Aside of that the print out in case of non matching type flags has the old
and new type flags arguments flipped. Fix that as well.

For correctness sake the flags stored in the irq descriptor should be
removed, but this is beyond the scope of this bugfix and will be done in a
later patch.

Fixes: 4b357daed698 ("genirq: Look-up trigger type if not specified by caller")
Reported-and-tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Jon Hunter <jonathanh@nvidia.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1611072020360.3501@nanos
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/irq/manage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 9c4d304..6b66959 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1341,12 +1341,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 
 	} else if (new->flags & IRQF_TRIGGER_MASK) {
 		unsigned int nmsk = new->flags & IRQF_TRIGGER_MASK;
-		unsigned int omsk = irq_settings_get_trigger_mask(desc);
+		unsigned int omsk = irqd_get_trigger_type(&desc->irq_data);
 
 		if (nmsk != omsk)
 			/* hope the handler works with current  trigger mode */
 			pr_warn("irq %d uses trigger mode %u; requested %u\n",
-				irq, nmsk, omsk);
+				irq, omsk, nmsk);
 	}
 
 	*old_ptr = new;