diff mbox

acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

Message ID 1373900260-1599-1-git-send-email-prarit@redhat.com (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Prarit Bhargava July 15, 2013, 2:57 p.m. UTC
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(+)

Comments

Rafael Wysocki July 24, 2013, 11:35 p.m. UTC | #1
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;
>
Prarit Bhargava July 25, 2013, 12:01 a.m. UTC | #2
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Prarit Bhargava July 25, 2013, 9:57 a.m. UTC | #3
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki July 25, 2013, 12:10 p.m. UTC | #4
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
Prarit Bhargava Aug. 4, 2013, 11:35 p.m. UTC | #5
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Aug. 5, 2013, 1:30 p.m. UTC | #6
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;