* BUG? genirq: irq 14 uses trigger mode 8; requested 0
@ 2016-11-01 13:02 Mika Westerberg
2016-11-01 14:24 ` Jon Hunter
0 siblings, 1 reply; 12+ messages in thread
From: Mika Westerberg @ 2016-11-01 13:02 UTC (permalink / raw)
To: Jon Hunter; +Cc: Marc Zyngier, Thomas Gleixner, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-01 13:02 BUG? genirq: irq 14 uses trigger mode 8; requested 0 Mika Westerberg @ 2016-11-01 14:24 ` Jon Hunter 2016-11-01 14:44 ` Mika Westerberg 0 siblings, 1 reply; 12+ messages in thread From: Jon Hunter @ 2016-11-01 14:24 UTC (permalink / raw) To: Mika Westerberg; +Cc: Marc Zyngier, Thomas Gleixner, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-01 14:24 ` Jon Hunter @ 2016-11-01 14:44 ` Mika Westerberg 2016-11-07 11:49 ` Mika Westerberg 0 siblings, 1 reply; 12+ messages in thread From: Mika Westerberg @ 2016-11-01 14:44 UTC (permalink / raw) To: Jon Hunter; +Cc: Marc Zyngier, Thomas Gleixner, linux-kernel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-01 14:44 ` Mika Westerberg @ 2016-11-07 11:49 ` Mika Westerberg 2016-11-07 13:32 ` Jon Hunter 0 siblings, 1 reply; 12+ messages in thread From: Mika Westerberg @ 2016-11-07 11:49 UTC (permalink / raw) To: Jon Hunter; +Cc: Marc Zyngier, Thomas Gleixner, linux-kernel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-07 11:49 ` Mika Westerberg @ 2016-11-07 13:32 ` Jon Hunter 2016-11-07 14:40 ` Marc Zyngier 0 siblings, 1 reply; 12+ messages in thread From: Jon Hunter @ 2016-11-07 13:32 UTC (permalink / raw) To: Mika Westerberg, Marc Zyngier; +Cc: Thomas Gleixner, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-07 13:32 ` Jon Hunter @ 2016-11-07 14:40 ` Marc Zyngier 2016-11-07 14:59 ` Mika Westerberg 0 siblings, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2016-11-07 14:40 UTC (permalink / raw) To: Jon Hunter, Mika Westerberg; +Cc: Thomas Gleixner, linux-kernel 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... ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-07 14:40 ` Marc Zyngier @ 2016-11-07 14:59 ` Mika Westerberg 2016-11-07 15:50 ` Jon Hunter 0 siblings, 1 reply; 12+ messages in thread From: Mika Westerberg @ 2016-11-07 14:59 UTC (permalink / raw) To: Marc Zyngier; +Cc: Jon Hunter, Thomas Gleixner, linux-kernel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-07 14:59 ` Mika Westerberg @ 2016-11-07 15:50 ` Jon Hunter 2016-11-07 16:00 ` Thomas Gleixner 0 siblings, 1 reply; 12+ messages in thread From: Jon Hunter @ 2016-11-07 15:50 UTC (permalink / raw) To: Mika Westerberg, Marc Zyngier; +Cc: Thomas Gleixner, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-07 15:50 ` Jon Hunter @ 2016-11-07 16:00 ` Thomas Gleixner 2016-11-07 19:29 ` Thomas Gleixner 0 siblings, 1 reply; 12+ messages in thread From: Thomas Gleixner @ 2016-11-07 16:00 UTC (permalink / raw) To: Jon Hunter; +Cc: Mika Westerberg, Marc Zyngier, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-07 16:00 ` Thomas Gleixner @ 2016-11-07 19:29 ` Thomas Gleixner 2016-11-08 9:17 ` Mika Westerberg 2016-11-08 14:22 ` [tip:irq/urgent] genirq: Use irq type from irqdata instead of irqdesc tip-bot for Thomas Gleixner 0 siblings, 2 replies; 12+ messages in thread From: Thomas Gleixner @ 2016-11-07 19:29 UTC (permalink / raw) To: Jon Hunter; +Cc: Mika Westerberg, Marc Zyngier, linux-kernel 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; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: BUG? genirq: irq 14 uses trigger mode 8; requested 0 2016-11-07 19:29 ` Thomas Gleixner @ 2016-11-08 9:17 ` Mika Westerberg 2016-11-08 14:22 ` [tip:irq/urgent] genirq: Use irq type from irqdata instead of irqdesc tip-bot for Thomas Gleixner 1 sibling, 0 replies; 12+ messages in thread From: Mika Westerberg @ 2016-11-08 9:17 UTC (permalink / raw) To: Thomas Gleixner; +Cc: Jon Hunter, Marc Zyngier, linux-kernel 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tip:irq/urgent] genirq: Use irq type from irqdata instead of irqdesc 2016-11-07 19:29 ` Thomas Gleixner 2016-11-08 9:17 ` Mika Westerberg @ 2016-11-08 14:22 ` tip-bot for Thomas Gleixner 1 sibling, 0 replies; 12+ messages in thread From: tip-bot for Thomas Gleixner @ 2016-11-08 14:22 UTC (permalink / raw) To: linux-tip-commits Cc: mika.westerberg, jonathanh, marc.zyngier, hpa, mingo, linux-kernel, tglx 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; ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-11-08 14:22 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-01 13:02 BUG? genirq: irq 14 uses trigger mode 8; requested 0 Mika Westerberg 2016-11-01 14:24 ` Jon Hunter 2016-11-01 14:44 ` Mika Westerberg 2016-11-07 11:49 ` Mika Westerberg 2016-11-07 13:32 ` Jon Hunter 2016-11-07 14:40 ` Marc Zyngier 2016-11-07 14:59 ` Mika Westerberg 2016-11-07 15:50 ` Jon Hunter 2016-11-07 16:00 ` Thomas Gleixner 2016-11-07 19:29 ` Thomas Gleixner 2016-11-08 9:17 ` Mika Westerberg 2016-11-08 14:22 ` [tip:irq/urgent] genirq: Use irq type from irqdata instead of irqdesc tip-bot for Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).