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

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

[Prarit Bhargava]

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(+)

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;
-- 
1.7.9.3

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Rafael J. Wysocki]

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;
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Prarit Bhargava]

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.

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Prarit Bhargava]

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.

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Prarit Bhargava]

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.

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Rafael J. Wysocki]

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

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

Hi,

On Mon, Aug 05, 2013 at 03:30:16PM +0200, Rafael J. Wysocki wrote:
> On Sunday, August 04, 2013 07:35:37 PM Prarit Bhargava wrote:
> > 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).

This breaks my xHCI :(

If the issues is that you are not getting any interrupt, then should
it not be checked separately and only fail in that case? I'm attaching
a patch that I made on top of linux-next, where I do just that. Would
that work for you?

Thanks,

-- 
heikki

[-- Attachment #2: 0001-ACPI-fix-the-behaviour-of-acpi_pci_irq_enable.patch --]
[-- Type: text/x-diff, Size: 1596 bytes --]

>From fdf8fbe4e75e7e320eba86ec51c9ede2495f30cd Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 5 Aug 2013 15:48:43 +0300
Subject: [PATCH] ACPI: fix the behaviour of acpi_pci_irq_enable()

acpi_pci_irq_enable() should not return error when PCI
provides an IRQ for the device even if ACPI does not know
the IRQ. This will change this case so that the function
only return error when there is no IRQ at all.

Without this the xHCI controller on my PC is allocated an
IRQ but still fails to be probed as pci_enable_device()
returns an error.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/pci_irq.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 9681847..3c0b474 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -419,8 +419,15 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
 	 */
 	if (gsi < 0) {
 		u32 dev_gsi;
+
+		if (!dev->irq) {
+			dev_err(&dev->dev, "PCI INT %c: no IRQ\n",
+				pin_name(pin));
+			return -ENOENT;
+		}
+
 		/* Interrupt Line values above 0xF are forbidden */
-		if (dev->irq > 0 && (dev->irq <= 0xF) &&
+		if ((dev->irq <= 0xF) &&
 		    (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0)) {
 			dev_warn(&dev->dev, "PCI INT %c: no GSI - using ISA IRQ %d\n",
 				 pin_name(pin), dev->irq);
@@ -430,7 +437,6 @@ 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;
-- 
1.8.3.2

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Prarit Bhargava]

On 08/05/2013 10:04 AM, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Aug 05, 2013 at 03:30:16PM +0200, Rafael J. Wysocki wrote:
>> On Sunday, August 04, 2013 07:35:37 PM Prarit Bhargava wrote:
>>> 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).
> 
> This breaks my xHCI :(
> 
> If the issues is that you are not getting any interrupt, then should
> it not be checked separately and only fail in that case? I'm attaching
> a patch that I made on top of linux-next, where I do just that. Would
> that work for you?
> 

Testing this right now ... I'll get back to everyone when I have results.

P.

> Thanks,
> 

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Rafael J. Wysocki]

On Tuesday, August 06, 2013 10:37:52 AM Prarit Bhargava wrote:
> On 08/05/2013 10:04 AM, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Mon, Aug 05, 2013 at 03:30:16PM +0200, Rafael J. Wysocki wrote:
> >> On Sunday, August 04, 2013 07:35:37 PM Prarit Bhargava wrote:
> >>> 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).
> > 
> > This breaks my xHCI :(
> > 
> > If the issues is that you are not getting any interrupt, then should
> > it not be checked separately and only fail in that case? I'm attaching
> > a patch that I made on top of linux-next, where I do just that. Would
> > that work for you?
> > 
> 
> Testing this right now ... I'll get back to everyone when I have results.

In the meantime I've dropped that commit from linux-next as it caused the xHCI
breakage to happen.

Thanks,
Rafael

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Prarit Bhargava]

On 08/06/2013 06:44 PM, Rafael J. Wysocki wrote:
> On Tuesday, August 06, 2013 10:37:52 AM Prarit Bhargava wrote:
>> On 08/05/2013 10:04 AM, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Mon, Aug 05, 2013 at 03:30:16PM +0200, Rafael J. Wysocki wrote:
>>>> On Sunday, August 04, 2013 07:35:37 PM Prarit Bhargava wrote:
>>>>> 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).
>>>
>>> This breaks my xHCI :(
>>>
>>> If the issues is that you are not getting any interrupt, then should
>>> it not be checked separately and only fail in that case? I'm attaching
>>> a patch that I made on top of linux-next, where I do just that. Would
>>> that work for you?
>>>
>>
>> Testing this right now ... I'll get back to everyone when I have results.
> 
> In the meantime I've dropped that commit from linux-next as it caused the xHCI
> breakage to happen.

Thanks Rafael.

Heikki, I tested your patch and found that it solves my problem.  Sorry for the
breakage :(

Can you add a

Tested-by: Prarit Bhargava <prarit@redhat.com>

to your patch?

Thanks,

P.

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Heikki Krogerus]

Hi,

On Wed, Aug 07, 2013 at 08:39:02AM -0400, Prarit Bhargava wrote:
> On 08/06/2013 06:44 PM, Rafael J. Wysocki wrote:
> > In the meantime I've dropped that commit from linux-next as it caused the xHCI
> > breakage to happen.
> 
> Thanks Rafael.
> 
> Heikki, I tested your patch and found that it solves my problem.  Sorry for the
> breakage :(
> 
> Can you add a
> 
> Tested-by: Prarit Bhargava <prarit@redhat.com>
> 
> to your patch?

Well, it would be more clear to me if you send new version of the
patch that combines the two, since Rafael dropped the original.

Thanks,

-- 
heikki

Re: [PATCH] acpi, acpi_pci_irq_enable must return an error if ACPI cannot map an IRQ.

[Rafael J. Wysocki]

On Wednesday, August 07, 2013 04:29:20 PM Heikki Krogerus wrote:
> Hi,
> 
> On Wed, Aug 07, 2013 at 08:39:02AM -0400, Prarit Bhargava wrote:
> > On 08/06/2013 06:44 PM, Rafael J. Wysocki wrote:
> > > In the meantime I've dropped that commit from linux-next as it caused the xHCI
> > > breakage to happen.
> > 
> > Thanks Rafael.
> > 
> > Heikki, I tested your patch and found that it solves my problem.  Sorry for the
> > breakage :(
> > 
> > Can you add a
> > 
> > Tested-by: Prarit Bhargava <prarit@redhat.com>
> > 
> > to your patch?
> 
> Well, it would be more clear to me if you send new version of the
> patch that combines the two, since Rafael dropped the original.

Well, could you please rebase your patch on top of the Linus' tree and resend
it?

Rafael