| Message ID | 1373900260-1599-1-git-send-email-prarit@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Monday, July 15, 2013 10:57:40 AM Prarit Bhargava wrote: > Driver probe's currently do the following > > pci_enable_device(); > /* ... do some other init stuff, and eventually call ... */ > request_irq(); > > After pci_enable_device() is called it is assumed that the device's irq > value (pci_dev->irq) has been appropriately set on success. This value > is passed into the request_irq() call. > > In the case that ACPI is used to determine the irq value, it is possible > that the ACPI IRQ look up for a specific device fails and success is > returned by pci_enable_device(). > > The call sequence is: > > pci_enable_device(); > -> pci_enable_device_flags(); > ->do_pci_enable_device(); > -> pcibios_enable_device() which, if the device > does not use MSI calls > -> pcibios_enable_irq() which maps to > acpi_pci_irq_enable() > -> acpi_pci_irq_lookup() > > If acpi_pci_irq_lookup() cannot map the device's IRQ value it returns NULL > as an error. The error is returned to acpi_pci_irq_enable(), but is not > propagated further. This can result in the driver returning success for > pci_enable_device() and the driver probe attempting to call request_irq() > with dev->irq = 0. > > This patch modifies acpi_pci_irq_enable() to return an error in the case > that an entry is not found in the ACPI tables. Is there any known system on which this leads to observable misbehavior? If so, what's that system? Rafael > Signed-off-by: Prarit Bhargava <prarit@redhat.com> > Cc: Len Brown <lenb@kernel.org> > Cc: "Rafael J. Wysocki" <rjw@sisk.pl> > Cc: "Bjorn Helgaas" <bhelgaas@google.com> > Cc: "Myron Stowe" <mstowe@redhat.com> > Cc: linux-pci@vger.kernel.org > --- > drivers/acpi/pci_irq.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > index 41c5e1b..9681847 100644 > --- a/drivers/acpi/pci_irq.c > +++ b/drivers/acpi/pci_irq.c > @@ -430,6 +430,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > } else { > dev_warn(&dev->dev, "PCI INT %c: no GSI\n", > pin_name(pin)); > + return -ENOENT; > } > > return 0; >
On 07/24/2013 07:35 PM, Rafael J. Wysocki wrote: > On Monday, July 15, 2013 10:57:40 AM Prarit Bhargava wrote: >> Driver probe's currently do the following >> >> pci_enable_device(); >> /* ... do some other init stuff, and eventually call ... */ >> request_irq(); >> >> After pci_enable_device() is called it is assumed that the device's irq >> value (pci_dev->irq) has been appropriately set on success. This value >> is passed into the request_irq() call. >> >> In the case that ACPI is used to determine the irq value, it is possible >> that the ACPI IRQ look up for a specific device fails and success is >> returned by pci_enable_device(). >> >> The call sequence is: >> >> pci_enable_device(); >> -> pci_enable_device_flags(); >> ->do_pci_enable_device(); >> -> pcibios_enable_device() which, if the device >> does not use MSI calls >> -> pcibios_enable_irq() which maps to >> acpi_pci_irq_enable() >> -> acpi_pci_irq_lookup() >> >> If acpi_pci_irq_lookup() cannot map the device's IRQ value it returns NULL >> as an error. The error is returned to acpi_pci_irq_enable(), but is not >> propagated further. This can result in the driver returning success for >> pci_enable_device() and the driver probe attempting to call request_irq() >> with dev->irq = 0. >> >> This patch modifies acpi_pci_irq_enable() to return an error in the case >> that an entry is not found in the ACPI tables. > > Is there any known system on which this leads to observable misbehavior? > > If so, what's that system? > Dell PowerEdge 840. The end result is an genirq warning about the irq flags of irq 0: [ 14.126845] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT B [ 14.133473] i801_smbus 0000:00:1f.3: PCI INT B: no GSI [ 14.795122] genirq: Flags mismatch irq 0. 00000080 (i801_smbus) vs. 00015a20 (timer) [ 14.796037] CPU: 0 PID: 427 Comm: systemd-udevd Tainted: G W -------------- 3.10.0-0.rc4.60.el7.x86_64.debug #1 [ 14.796037] Hardware name: Dell Computer Corporation PowerEdge 840/0RH822, BIOS A05 10/04/2007 [ 14.796037] ffff880075643b80 ffff88007a743aa8 ffffffff816b8bbd ffff88007a743af8 [ 14.796037] ffffffff8111ed8d ffffffff811bf5d1 0000000000000296 ffff880075b93800 [ 14.796037] ffffffffa03d2240 0000000000000080 0000000000000000 ffff88007ad7e000 [ 14.796037] Call Trace: [ 14.796037] [<ffffffff816b8bbd>] dump_stack+0x19/0x1b [ 14.796037] [<ffffffff8111ed8d>] __setup_irq+0x51d/0x550 [ 14.796037] [<ffffffff811bf5d1>] ? kmem_cache_alloc_trace+0x111/0x340 [ 14.796037] [<ffffffffa03d2240>] ? i801_check_pre.isra.6+0xe0/0xe0 [i2c_i801] [ 14.796037] [<ffffffff8111ef1c>] request_threaded_irq+0xcc/0x170 [ 14.796037] [<ffffffffa03d388d>] i801_probe+0x34d/0x52c [i2c_i801] [ 14.796037] [<ffffffff810ddecd>] ? trace_hardirqs_on+0xd/0x10 [ 14.796037] [<ffffffff813876be>] local_pci_probe+0x3e/0x70 [ 14.796037] [<ffffffff813889a1>] pci_device_probe+0x111/0x120 [ 14.796037] [<ffffffff8144ff77>] driver_probe_device+0x87/0x390 [ 14.796037] [<ffffffff81450353>] __driver_attach+0x93/0xa0 [ 14.796037] [<ffffffff814502c0>] ? __device_attach+0x40/0x40 [ 14.796037] [<ffffffff8144de93>] bus_for_each_dev+0x63/0xa0 [ 14.796037] [<ffffffff8144f9ae>] driver_attach+0x1e/0x20 [ 14.796037] [<ffffffff8144f540>] bus_add_driver+0x1f0/0x2a0 [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff [ 14.796037] [<ffffffff814509d1>] driver_register+0x71/0x150 [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff [ 14.796037] [<ffffffff81387540>] __pci_register_driver+0x60/0x70 [ 14.796037] [<ffffffffa03d80af>] i2c_i801_init+0xaf/0x1000 [i2c_i801] [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff [ 14.796037] [<ffffffff810020e2>] do_one_initcall+0xe2/0x1a0 [ 14.796037] [<ffffffff810ee09f>] load_module+0xf4f/0x14d0 [ 14.796037] [<ffffffff81375d20>] ? ddebug_proc_write+0xf0/0xf0 [ 14.796037] [<ffffffff8136088d>] ? trace_hardirqs_off_thunk+0x3a/0x3c [ 14.796037] [<ffffffff810ee6e1>] SyS_init_module+0xc1/0x110 [ 14.796037] [<ffffffff816ca299>] system_call_fastpath+0x16/0x1b P. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/24/2013 08:01 PM, Prarit Bhargava wrote: > > > On 07/24/2013 07:35 PM, Rafael J. Wysocki wrote: >> On Monday, July 15, 2013 10:57:40 AM Prarit Bhargava wrote: >>> Driver probe's currently do the following >>> >>> pci_enable_device(); >>> /* ... do some other init stuff, and eventually call ... */ >>> request_irq(); >>> >>> After pci_enable_device() is called it is assumed that the device's irq >>> value (pci_dev->irq) has been appropriately set on success. This value >>> is passed into the request_irq() call. >>> >>> In the case that ACPI is used to determine the irq value, it is possible >>> that the ACPI IRQ look up for a specific device fails and success is >>> returned by pci_enable_device(). >>> >>> The call sequence is: >>> >>> pci_enable_device(); >>> -> pci_enable_device_flags(); >>> ->do_pci_enable_device(); >>> -> pcibios_enable_device() which, if the device >>> does not use MSI calls >>> -> pcibios_enable_irq() which maps to >>> acpi_pci_irq_enable() >>> -> acpi_pci_irq_lookup() >>> >>> If acpi_pci_irq_lookup() cannot map the device's IRQ value it returns NULL >>> as an error. The error is returned to acpi_pci_irq_enable(), but is not >>> propagated further. This can result in the driver returning success for >>> pci_enable_device() and the driver probe attempting to call request_irq() >>> with dev->irq = 0. >>> >>> This patch modifies acpi_pci_irq_enable() to return an error in the case >>> that an entry is not found in the ACPI tables. >> >> Is there any known system on which this leads to observable misbehavior? >> >> If so, what's that system? >> > > Dell PowerEdge 840. The end result is an genirq warning about the irq flags of > irq 0: > > [ 14.126845] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT B > [ 14.133473] i801_smbus 0000:00:1f.3: PCI INT B: no GSI > [ 14.795122] genirq: Flags mismatch irq 0. 00000080 (i801_smbus) vs. 00015a20 > (timer) > [ 14.796037] CPU: 0 PID: 427 Comm: systemd-udevd Tainted: G W > -------------- 3.10.0-0.rc4.60.el7.x86_64.debug #1 > [ 14.796037] Hardware name: Dell Computer Corporation PowerEdge 840/0RH822, > BIOS A05 10/04/2007 > [ 14.796037] ffff880075643b80 ffff88007a743aa8 ffffffff816b8bbd ffff88007a743af8 > [ 14.796037] ffffffff8111ed8d ffffffff811bf5d1 0000000000000296 ffff880075b93800 > [ 14.796037] ffffffffa03d2240 0000000000000080 0000000000000000 ffff88007ad7e000 > [ 14.796037] Call Trace: > [ 14.796037] [<ffffffff816b8bbd>] dump_stack+0x19/0x1b > [ 14.796037] [<ffffffff8111ed8d>] __setup_irq+0x51d/0x550 > [ 14.796037] [<ffffffff811bf5d1>] ? kmem_cache_alloc_trace+0x111/0x340 > [ 14.796037] [<ffffffffa03d2240>] ? i801_check_pre.isra.6+0xe0/0xe0 [i2c_i801] > [ 14.796037] [<ffffffff8111ef1c>] request_threaded_irq+0xcc/0x170 > [ 14.796037] [<ffffffffa03d388d>] i801_probe+0x34d/0x52c [i2c_i801] > [ 14.796037] [<ffffffff810ddecd>] ? trace_hardirqs_on+0xd/0x10 > [ 14.796037] [<ffffffff813876be>] local_pci_probe+0x3e/0x70 > [ 14.796037] [<ffffffff813889a1>] pci_device_probe+0x111/0x120 > [ 14.796037] [<ffffffff8144ff77>] driver_probe_device+0x87/0x390 > [ 14.796037] [<ffffffff81450353>] __driver_attach+0x93/0xa0 > [ 14.796037] [<ffffffff814502c0>] ? __device_attach+0x40/0x40 > [ 14.796037] [<ffffffff8144de93>] bus_for_each_dev+0x63/0xa0 > [ 14.796037] [<ffffffff8144f9ae>] driver_attach+0x1e/0x20 > [ 14.796037] [<ffffffff8144f540>] bus_add_driver+0x1f0/0x2a0 > [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > [ 14.796037] [<ffffffff814509d1>] driver_register+0x71/0x150 > [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > [ 14.796037] [<ffffffff81387540>] __pci_register_driver+0x60/0x70 > [ 14.796037] [<ffffffffa03d80af>] i2c_i801_init+0xaf/0x1000 [i2c_i801] > [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > [ 14.796037] [<ffffffff810020e2>] do_one_initcall+0xe2/0x1a0 > [ 14.796037] [<ffffffff810ee09f>] load_module+0xf4f/0x14d0 > [ 14.796037] [<ffffffff81375d20>] ? ddebug_proc_write+0xf0/0xf0 > [ 14.796037] [<ffffffff8136088d>] ? trace_hardirqs_off_thunk+0x3a/0x3c > [ 14.796037] [<ffffffff810ee6e1>] SyS_init_module+0xc1/0x110 > [ 14.796037] [<ffffffff816ca299>] system_call_fastpath+0x16/0x1b > Rafael, One other thing -- the core issue here is that the BIOS is broken and does not supply an ACPI entry for the IRQ. I think the kernel should do a better job of reacting to that instead of just continuing down a broken path. In either case, the end result is the same -- the i801 driver does not load, but with my patch the warning is not displayed, which IMO is the correct way to fail. P. > P. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, July 25, 2013 05:57:19 AM Prarit Bhargava wrote: > > On 07/24/2013 08:01 PM, Prarit Bhargava wrote: > > > > > > On 07/24/2013 07:35 PM, Rafael J. Wysocki wrote: > >> On Monday, July 15, 2013 10:57:40 AM Prarit Bhargava wrote: > >>> Driver probe's currently do the following > >>> > >>> pci_enable_device(); > >>> /* ... do some other init stuff, and eventually call ... */ > >>> request_irq(); > >>> > >>> After pci_enable_device() is called it is assumed that the device's irq > >>> value (pci_dev->irq) has been appropriately set on success. This value > >>> is passed into the request_irq() call. > >>> > >>> In the case that ACPI is used to determine the irq value, it is possible > >>> that the ACPI IRQ look up for a specific device fails and success is > >>> returned by pci_enable_device(). > >>> > >>> The call sequence is: > >>> > >>> pci_enable_device(); > >>> -> pci_enable_device_flags(); > >>> ->do_pci_enable_device(); > >>> -> pcibios_enable_device() which, if the device > >>> does not use MSI calls > >>> -> pcibios_enable_irq() which maps to > >>> acpi_pci_irq_enable() > >>> -> acpi_pci_irq_lookup() > >>> > >>> If acpi_pci_irq_lookup() cannot map the device's IRQ value it returns NULL > >>> as an error. The error is returned to acpi_pci_irq_enable(), but is not > >>> propagated further. This can result in the driver returning success for > >>> pci_enable_device() and the driver probe attempting to call request_irq() > >>> with dev->irq = 0. > >>> > >>> This patch modifies acpi_pci_irq_enable() to return an error in the case > >>> that an entry is not found in the ACPI tables. > >> > >> Is there any known system on which this leads to observable misbehavior? > >> > >> If so, what's that system? > >> > > > > Dell PowerEdge 840. The end result is an genirq warning about the irq flags of > > irq 0: > > > > [ 14.126845] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT B > > [ 14.133473] i801_smbus 0000:00:1f.3: PCI INT B: no GSI > > [ 14.795122] genirq: Flags mismatch irq 0. 00000080 (i801_smbus) vs. 00015a20 > > (timer) > > [ 14.796037] CPU: 0 PID: 427 Comm: systemd-udevd Tainted: G W > > -------------- 3.10.0-0.rc4.60.el7.x86_64.debug #1 > > [ 14.796037] Hardware name: Dell Computer Corporation PowerEdge 840/0RH822, > > BIOS A05 10/04/2007 > > [ 14.796037] ffff880075643b80 ffff88007a743aa8 ffffffff816b8bbd ffff88007a743af8 > > [ 14.796037] ffffffff8111ed8d ffffffff811bf5d1 0000000000000296 ffff880075b93800 > > [ 14.796037] ffffffffa03d2240 0000000000000080 0000000000000000 ffff88007ad7e000 > > [ 14.796037] Call Trace: > > [ 14.796037] [<ffffffff816b8bbd>] dump_stack+0x19/0x1b > > [ 14.796037] [<ffffffff8111ed8d>] __setup_irq+0x51d/0x550 > > [ 14.796037] [<ffffffff811bf5d1>] ? kmem_cache_alloc_trace+0x111/0x340 > > [ 14.796037] [<ffffffffa03d2240>] ? i801_check_pre.isra.6+0xe0/0xe0 [i2c_i801] > > [ 14.796037] [<ffffffff8111ef1c>] request_threaded_irq+0xcc/0x170 > > [ 14.796037] [<ffffffffa03d388d>] i801_probe+0x34d/0x52c [i2c_i801] > > [ 14.796037] [<ffffffff810ddecd>] ? trace_hardirqs_on+0xd/0x10 > > [ 14.796037] [<ffffffff813876be>] local_pci_probe+0x3e/0x70 > > [ 14.796037] [<ffffffff813889a1>] pci_device_probe+0x111/0x120 > > [ 14.796037] [<ffffffff8144ff77>] driver_probe_device+0x87/0x390 > > [ 14.796037] [<ffffffff81450353>] __driver_attach+0x93/0xa0 > > [ 14.796037] [<ffffffff814502c0>] ? __device_attach+0x40/0x40 > > [ 14.796037] [<ffffffff8144de93>] bus_for_each_dev+0x63/0xa0 > > [ 14.796037] [<ffffffff8144f9ae>] driver_attach+0x1e/0x20 > > [ 14.796037] [<ffffffff8144f540>] bus_add_driver+0x1f0/0x2a0 > > [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > > [ 14.796037] [<ffffffff814509d1>] driver_register+0x71/0x150 > > [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > > [ 14.796037] [<ffffffff81387540>] __pci_register_driver+0x60/0x70 > > [ 14.796037] [<ffffffffa03d80af>] i2c_i801_init+0xaf/0x1000 [i2c_i801] > > [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > > [ 14.796037] [<ffffffff810020e2>] do_one_initcall+0xe2/0x1a0 > > [ 14.796037] [<ffffffff810ee09f>] load_module+0xf4f/0x14d0 > > [ 14.796037] [<ffffffff81375d20>] ? ddebug_proc_write+0xf0/0xf0 > > [ 14.796037] [<ffffffff8136088d>] ? trace_hardirqs_off_thunk+0x3a/0x3c > > [ 14.796037] [<ffffffff810ee6e1>] SyS_init_module+0xc1/0x110 > > [ 14.796037] [<ffffffff816ca299>] system_call_fastpath+0x16/0x1b > > > > Rafael, > > One other thing -- the core issue here is that the BIOS is broken and does not > supply an ACPI entry for the IRQ. I think the kernel should do a better job of > reacting to that instead of just continuing down a broken path. > > In either case, the end result is the same -- the i801 driver does not load, but > with my patch the warning is not displayed, which IMO is the correct way to fail. I agree. I wanted to know how serious the problem was so as to determine the urgency of the fix. Thanks, Rafael
On 07/25/2013 05:57 AM, Prarit Bhargava wrote: > > > On 07/24/2013 08:01 PM, Prarit Bhargava wrote: >> >> >> On 07/24/2013 07:35 PM, Rafael J. Wysocki wrote: >>> On Monday, July 15, 2013 10:57:40 AM Prarit Bhargava wrote: >>>> Driver probe's currently do the following >>>> >>>> pci_enable_device(); >>>> /* ... do some other init stuff, and eventually call ... */ >>>> request_irq(); >>>> >>>> After pci_enable_device() is called it is assumed that the device's irq >>>> value (pci_dev->irq) has been appropriately set on success. This value >>>> is passed into the request_irq() call. >>>> >>>> In the case that ACPI is used to determine the irq value, it is possible >>>> that the ACPI IRQ look up for a specific device fails and success is >>>> returned by pci_enable_device(). >>>> >>>> The call sequence is: >>>> >>>> pci_enable_device(); >>>> -> pci_enable_device_flags(); >>>> ->do_pci_enable_device(); >>>> -> pcibios_enable_device() which, if the device >>>> does not use MSI calls >>>> -> pcibios_enable_irq() which maps to >>>> acpi_pci_irq_enable() >>>> -> acpi_pci_irq_lookup() >>>> >>>> If acpi_pci_irq_lookup() cannot map the device's IRQ value it returns NULL >>>> as an error. The error is returned to acpi_pci_irq_enable(), but is not >>>> propagated further. This can result in the driver returning success for >>>> pci_enable_device() and the driver probe attempting to call request_irq() >>>> with dev->irq = 0. >>>> >>>> This patch modifies acpi_pci_irq_enable() to return an error in the case >>>> that an entry is not found in the ACPI tables. >>> >>> Is there any known system on which this leads to observable misbehavior? >>> >>> If so, what's that system? >>> >> >> Dell PowerEdge 840. The end result is an genirq warning about the irq flags of >> irq 0: >> >> [ 14.126845] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT B >> [ 14.133473] i801_smbus 0000:00:1f.3: PCI INT B: no GSI >> [ 14.795122] genirq: Flags mismatch irq 0. 00000080 (i801_smbus) vs. 00015a20 >> (timer) >> [ 14.796037] CPU: 0 PID: 427 Comm: systemd-udevd Tainted: G W >> -------------- 3.10.0-0.rc4.60.el7.x86_64.debug #1 >> [ 14.796037] Hardware name: Dell Computer Corporation PowerEdge 840/0RH822, >> BIOS A05 10/04/2007 >> [ 14.796037] ffff880075643b80 ffff88007a743aa8 ffffffff816b8bbd ffff88007a743af8 >> [ 14.796037] ffffffff8111ed8d ffffffff811bf5d1 0000000000000296 ffff880075b93800 >> [ 14.796037] ffffffffa03d2240 0000000000000080 0000000000000000 ffff88007ad7e000 >> [ 14.796037] Call Trace: >> [ 14.796037] [<ffffffff816b8bbd>] dump_stack+0x19/0x1b >> [ 14.796037] [<ffffffff8111ed8d>] __setup_irq+0x51d/0x550 >> [ 14.796037] [<ffffffff811bf5d1>] ? kmem_cache_alloc_trace+0x111/0x340 >> [ 14.796037] [<ffffffffa03d2240>] ? i801_check_pre.isra.6+0xe0/0xe0 [i2c_i801] >> [ 14.796037] [<ffffffff8111ef1c>] request_threaded_irq+0xcc/0x170 >> [ 14.796037] [<ffffffffa03d388d>] i801_probe+0x34d/0x52c [i2c_i801] >> [ 14.796037] [<ffffffff810ddecd>] ? trace_hardirqs_on+0xd/0x10 >> [ 14.796037] [<ffffffff813876be>] local_pci_probe+0x3e/0x70 >> [ 14.796037] [<ffffffff813889a1>] pci_device_probe+0x111/0x120 >> [ 14.796037] [<ffffffff8144ff77>] driver_probe_device+0x87/0x390 >> [ 14.796037] [<ffffffff81450353>] __driver_attach+0x93/0xa0 >> [ 14.796037] [<ffffffff814502c0>] ? __device_attach+0x40/0x40 >> [ 14.796037] [<ffffffff8144de93>] bus_for_each_dev+0x63/0xa0 >> [ 14.796037] [<ffffffff8144f9ae>] driver_attach+0x1e/0x20 >> [ 14.796037] [<ffffffff8144f540>] bus_add_driver+0x1f0/0x2a0 >> [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff >> [ 14.796037] [<ffffffff814509d1>] driver_register+0x71/0x150 >> [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff >> [ 14.796037] [<ffffffff81387540>] __pci_register_driver+0x60/0x70 >> [ 14.796037] [<ffffffffa03d80af>] i2c_i801_init+0xaf/0x1000 [i2c_i801] >> [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff >> [ 14.796037] [<ffffffff810020e2>] do_one_initcall+0xe2/0x1a0 >> [ 14.796037] [<ffffffff810ee09f>] load_module+0xf4f/0x14d0 >> [ 14.796037] [<ffffffff81375d20>] ? ddebug_proc_write+0xf0/0xf0 >> [ 14.796037] [<ffffffff8136088d>] ? trace_hardirqs_off_thunk+0x3a/0x3c >> [ 14.796037] [<ffffffff810ee6e1>] SyS_init_module+0xc1/0x110 >> [ 14.796037] [<ffffffff816ca299>] system_call_fastpath+0x16/0x1b >> > > Rafael, > > One other thing -- the core issue here is that the BIOS is broken and does not > supply an ACPI entry for the IRQ. I think the kernel should do a better job of > reacting to that instead of just continuing down a broken path. > > In either case, the end result is the same -- the i801 driver does not load, but > with my patch the warning is not displayed, which IMO is the correct way to fail. > > P. Hey Rafael, I know you're busy but I was just wondering if this was queued up anywhere or if you had any other questions? Thanks, P. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday, August 04, 2013 07:35:37 PM Prarit Bhargava wrote: > > On 07/25/2013 05:57 AM, Prarit Bhargava wrote: > > > > > > On 07/24/2013 08:01 PM, Prarit Bhargava wrote: > >> > >> > >> On 07/24/2013 07:35 PM, Rafael J. Wysocki wrote: > >>> On Monday, July 15, 2013 10:57:40 AM Prarit Bhargava wrote: > >>>> Driver probe's currently do the following > >>>> > >>>> pci_enable_device(); > >>>> /* ... do some other init stuff, and eventually call ... */ > >>>> request_irq(); > >>>> > >>>> After pci_enable_device() is called it is assumed that the device's irq > >>>> value (pci_dev->irq) has been appropriately set on success. This value > >>>> is passed into the request_irq() call. > >>>> > >>>> In the case that ACPI is used to determine the irq value, it is possible > >>>> that the ACPI IRQ look up for a specific device fails and success is > >>>> returned by pci_enable_device(). > >>>> > >>>> The call sequence is: > >>>> > >>>> pci_enable_device(); > >>>> -> pci_enable_device_flags(); > >>>> ->do_pci_enable_device(); > >>>> -> pcibios_enable_device() which, if the device > >>>> does not use MSI calls > >>>> -> pcibios_enable_irq() which maps to > >>>> acpi_pci_irq_enable() > >>>> -> acpi_pci_irq_lookup() > >>>> > >>>> If acpi_pci_irq_lookup() cannot map the device's IRQ value it returns NULL > >>>> as an error. The error is returned to acpi_pci_irq_enable(), but is not > >>>> propagated further. This can result in the driver returning success for > >>>> pci_enable_device() and the driver probe attempting to call request_irq() > >>>> with dev->irq = 0. > >>>> > >>>> This patch modifies acpi_pci_irq_enable() to return an error in the case > >>>> that an entry is not found in the ACPI tables. > >>> > >>> Is there any known system on which this leads to observable misbehavior? > >>> > >>> If so, what's that system? > >>> > >> > >> Dell PowerEdge 840. The end result is an genirq warning about the irq flags of > >> irq 0: > >> > >> [ 14.126845] i801_smbus 0000:00:1f.3: can't derive routing for PCI INT B > >> [ 14.133473] i801_smbus 0000:00:1f.3: PCI INT B: no GSI > >> [ 14.795122] genirq: Flags mismatch irq 0. 00000080 (i801_smbus) vs. 00015a20 > >> (timer) > >> [ 14.796037] CPU: 0 PID: 427 Comm: systemd-udevd Tainted: G W > >> -------------- 3.10.0-0.rc4.60.el7.x86_64.debug #1 > >> [ 14.796037] Hardware name: Dell Computer Corporation PowerEdge 840/0RH822, > >> BIOS A05 10/04/2007 > >> [ 14.796037] ffff880075643b80 ffff88007a743aa8 ffffffff816b8bbd ffff88007a743af8 > >> [ 14.796037] ffffffff8111ed8d ffffffff811bf5d1 0000000000000296 ffff880075b93800 > >> [ 14.796037] ffffffffa03d2240 0000000000000080 0000000000000000 ffff88007ad7e000 > >> [ 14.796037] Call Trace: > >> [ 14.796037] [<ffffffff816b8bbd>] dump_stack+0x19/0x1b > >> [ 14.796037] [<ffffffff8111ed8d>] __setup_irq+0x51d/0x550 > >> [ 14.796037] [<ffffffff811bf5d1>] ? kmem_cache_alloc_trace+0x111/0x340 > >> [ 14.796037] [<ffffffffa03d2240>] ? i801_check_pre.isra.6+0xe0/0xe0 [i2c_i801] > >> [ 14.796037] [<ffffffff8111ef1c>] request_threaded_irq+0xcc/0x170 > >> [ 14.796037] [<ffffffffa03d388d>] i801_probe+0x34d/0x52c [i2c_i801] > >> [ 14.796037] [<ffffffff810ddecd>] ? trace_hardirqs_on+0xd/0x10 > >> [ 14.796037] [<ffffffff813876be>] local_pci_probe+0x3e/0x70 > >> [ 14.796037] [<ffffffff813889a1>] pci_device_probe+0x111/0x120 > >> [ 14.796037] [<ffffffff8144ff77>] driver_probe_device+0x87/0x390 > >> [ 14.796037] [<ffffffff81450353>] __driver_attach+0x93/0xa0 > >> [ 14.796037] [<ffffffff814502c0>] ? __device_attach+0x40/0x40 > >> [ 14.796037] [<ffffffff8144de93>] bus_for_each_dev+0x63/0xa0 > >> [ 14.796037] [<ffffffff8144f9ae>] driver_attach+0x1e/0x20 > >> [ 14.796037] [<ffffffff8144f540>] bus_add_driver+0x1f0/0x2a0 > >> [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > >> [ 14.796037] [<ffffffff814509d1>] driver_register+0x71/0x150 > >> [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > >> [ 14.796037] [<ffffffff81387540>] __pci_register_driver+0x60/0x70 > >> [ 14.796037] [<ffffffffa03d80af>] i2c_i801_init+0xaf/0x1000 [i2c_i801] > >> [ 14.796037] [<ffffffffa03d8000>] ? 0xffffffffa03d7fff > >> [ 14.796037] [<ffffffff810020e2>] do_one_initcall+0xe2/0x1a0 > >> [ 14.796037] [<ffffffff810ee09f>] load_module+0xf4f/0x14d0 > >> [ 14.796037] [<ffffffff81375d20>] ? ddebug_proc_write+0xf0/0xf0 > >> [ 14.796037] [<ffffffff8136088d>] ? trace_hardirqs_off_thunk+0x3a/0x3c > >> [ 14.796037] [<ffffffff810ee6e1>] SyS_init_module+0xc1/0x110 > >> [ 14.796037] [<ffffffff816ca299>] system_call_fastpath+0x16/0x1b > >> > > > > Rafael, > > > > One other thing -- the core issue here is that the BIOS is broken and does not > > supply an ACPI entry for the IRQ. I think the kernel should do a better job of > > reacting to that instead of just continuing down a broken path. > > > > In either case, the end result is the same -- the i801 driver does not load, but > > with my patch the warning is not displayed, which IMO is the correct way to fail. > > > > P. > > Hey Rafael, > > I know you're busy but I was just wondering if this was queued up anywhere or if > you had any other questions? Should be qeued up for 3.12 (currently in linux-next). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index 41c5e1b..9681847 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -430,6 +430,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev) } else { dev_warn(&dev->dev, "PCI INT %c: no GSI\n", pin_name(pin)); + return -ENOENT; } return 0;
Driver probe's currently do the following pci_enable_device(); /* ... do some other init stuff, and eventually call ... */ request_irq(); After pci_enable_device() is called it is assumed that the device's irq value (pci_dev->irq) has been appropriately set on success. This value is passed into the request_irq() call. In the case that ACPI is used to determine the irq value, it is possible that the ACPI IRQ look up for a specific device fails and success is returned by pci_enable_device(). The call sequence is: pci_enable_device(); -> pci_enable_device_flags(); ->do_pci_enable_device(); -> pcibios_enable_device() which, if the device does not use MSI calls -> pcibios_enable_irq() which maps to acpi_pci_irq_enable() -> acpi_pci_irq_lookup() If acpi_pci_irq_lookup() cannot map the device's IRQ value it returns NULL as an error. The error is returned to acpi_pci_irq_enable(), but is not propagated further. This can result in the driver returning success for pci_enable_device() and the driver probe attempting to call request_irq() with dev->irq = 0. This patch modifies acpi_pci_irq_enable() to return an error in the case that an entry is not found in the ACPI tables. Signed-off-by: Prarit Bhargava <prarit@redhat.com> Cc: Len Brown <lenb@kernel.org> Cc: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: "Bjorn Helgaas" <bhelgaas@google.com> Cc: "Myron Stowe" <mstowe@redhat.com> Cc: linux-pci@vger.kernel.org --- drivers/acpi/pci_irq.c | 1 + 1 file changed, 1 insertion(+)