| Message ID | 1455506732-26043-1-git-send-email-chen.fan.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Chen,
[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.5-rc4 next-20160212]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Chen-Fan/x86-PCI-Recognize-that-Interrupt-Line-255-means-not-connected/20160215-113403
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-x004-201607 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/acpi/pci_irq.c: In function 'acpi_pci_irq_valid':
>> drivers/acpi/pci_irq.c:401:14: error: 'pin' undeclared (first use in this function)
pin_name(pin));
^
drivers/acpi/pci_irq.c:401:14: note: each undeclared identifier is reported only once for each function it appears in
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/dmi.h:4,
from drivers/acpi/pci_irq.c:26:
drivers/acpi/pci_irq.c: In function 'acpi_pci_irq_enable':
>> drivers/acpi/pci_irq.c:457:8: error: too many arguments to function 'acpi_pci_irq_valid'
if (!acpi_pci_irq_valid(dev, pin))
^
include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
if (__builtin_constant_p((cond)) ? !!(cond) : \
^
>> drivers/acpi/pci_irq.c:457:3: note: in expansion of macro 'if'
if (!acpi_pci_irq_valid(dev, pin))
^
drivers/acpi/pci_irq.c:391:20: note: declared here
static inline bool acpi_pci_irq_valid(struct pci_dev *dev)
^
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/dmi.h:4,
from drivers/acpi/pci_irq.c:26:
>> drivers/acpi/pci_irq.c:457:8: error: too many arguments to function 'acpi_pci_irq_valid'
if (!acpi_pci_irq_valid(dev, pin))
^
include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
if (__builtin_constant_p((cond)) ? !!(cond) : \
^
>> drivers/acpi/pci_irq.c:457:3: note: in expansion of macro 'if'
if (!acpi_pci_irq_valid(dev, pin))
^
drivers/acpi/pci_irq.c:391:20: note: declared here
static inline bool acpi_pci_irq_valid(struct pci_dev *dev)
^
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/dmi.h:4,
from drivers/acpi/pci_irq.c:26:
>> drivers/acpi/pci_irq.c:457:8: error: too many arguments to function 'acpi_pci_irq_valid'
if (!acpi_pci_irq_valid(dev, pin))
^
include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^
>> drivers/acpi/pci_irq.c:457:3: note: in expansion of macro 'if'
if (!acpi_pci_irq_valid(dev, pin))
^
drivers/acpi/pci_irq.c:391:20: note: declared here
static inline bool acpi_pci_irq_valid(struct pci_dev *dev)
^
vim +/pin +401 drivers/acpi/pci_irq.c
395 * On x86 irq line 0xff means "unknown" or "no connection"
396 * (PCI 3.0, Section 6.2.4, footnote on page 223).
397 */
398 if (dev->irq == 0xff) {
399 dev->irq = IRQ_NOTCONNECTED;
400 dev_warn(&dev->dev, "PCI INT %c: not connected\n",
> 401 pin_name(pin));
402 return false;
403 }
404 #endif
405 return true;
406 }
407
408 int acpi_pci_irq_enable(struct pci_dev *dev)
409 {
410 struct acpi_prt_entry *entry;
411 int gsi;
412 u8 pin;
413 int triggering = ACPI_LEVEL_SENSITIVE;
414 int polarity = ACPI_ACTIVE_LOW;
415 char *link = NULL;
416 char link_desc[16];
417 int rc;
418
419 pin = dev->pin;
420 if (!pin) {
421 ACPI_DEBUG_PRINT((ACPI_DB_INFO,
422 "No interrupt pin configured for device %s\n",
423 pci_name(dev)));
424 return 0;
425 }
426
427 if (pci_has_managed_irq(dev))
428 return 0;
429
430 entry = acpi_pci_irq_lookup(dev, pin);
431 if (!entry) {
432 /*
433 * IDE legacy mode controller IRQs are magic. Why do compat
434 * extensions always make such a nasty mess.
435 */
436 if (dev->class >> 8 == PCI_CLASS_STORAGE_IDE &&
437 (dev->class & 0x05) == 0)
438 return 0;
439 }
440
441 if (entry) {
442 if (entry->link)
443 gsi = acpi_pci_link_allocate_irq(entry->link,
444 entry->index,
445 &triggering, &polarity,
446 &link);
447 else
448 gsi = entry->index;
449 } else
450 gsi = -1;
451
452 if (gsi < 0) {
453 /*
454 * No IRQ known to the ACPI subsystem - maybe the BIOS /
455 * driver reported one, then use it. Exit in any case.
456 */
> 457 if (!acpi_pci_irq_valid(dev, pin))
458 return 0;
459
460 if (acpi_isa_register_gsi(dev))
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Oh, it's my fault to send this wrong patch, so sorry for that. pls ignore this patch. I has tested the new patch, it works fine on my environment. I have resent it. Thanks, Chen On 02/15/2016 11:25 AM, Chen Fan wrote: > Per the x86-specific footnote to PCI spec r3.0, sec 6.2.4, the value 255 in > the Interrupt Line register means "unknown" or "no connection." > Previously, when we couldn't derive an IRQ from the _PRT, we fell back to > using the value from Interrupt Line as an IRQ. It's questionable whether > we should do that at all, but the spec clearly suggests we shouldn't do it > for the value 255 on x86. > > Calling request_irq() with IRQ 255 may succeed, but the driver won't > receive any interrupts. Or, if IRQ 255 is shared with another device, it > may succeed, and the driver's ISR will be called at random times when the > *other* device interrupts. Or it may fail if another device is using IRQ > 255 with incompatible flags. What we *want* is for request_irq() to fail > predictably so the driver can fall back to polling. > > On x86, assume 255 in the Interrupt Line means the INTx line is not > connected. In that case, set dev->irq to IRQ_NOTCONNECTED so request_irq() > will fail gracefully with -ENOTCONN. > > We found this problem on a system where Secure Boot firmware assigned > Interrupt Line 255 to an i801_smbus device and another device was already > using MSI-X IRQ 255. This was in v3.10, where i801_probe() fails if > request_irq() fails: > > i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143) > i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C > i801_smbus 0000:00:1f.3: PCI INT C: no GSI > genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa) > CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1 > Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5 > Call Trace: > dump_stack+0x19/0x1b > __setup_irq+0x54a/0x570 > request_threaded_irq+0xcc/0x170 > i801_probe+0x32f/0x508 [i2c_i801] > local_pci_probe+0x45/0xa0 > i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16 > i801_smbus: probe of 0000:00:1f.3 failed with error -16 > > After aeb8a3d16ae0 ("i2c: i801: Check if interrupts are disabled"), > i801_probe() will fall back to polling if request_irq() fails. But we > still need this patch because request_irq() may succeed or fail depending > on other devices in the system. If request_irq() fails, i801_smbus will > work by falling back to polling, but if it succeeds, i801_smbus won't work > because it expects interrupts that it may not receive. > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > Acked-by: Thomas Gleixner <tglx@linutronix.de> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/acpi/pci_irq.c | 29 +++++++++++++++++++++++++---- > include/linux/interrupt.h | 10 ++++++++++ > kernel/irq/manage.c | 9 ++++++++- > 3 files changed, 43 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > index d30184c..0227cc6 100644 > --- a/drivers/acpi/pci_irq.c > +++ b/drivers/acpi/pci_irq.c > @@ -33,6 +33,7 @@ > #include <linux/pci.h> > #include <linux/acpi.h> > #include <linux/slab.h> > +#include <linux/interrupt.h> > > #define PREFIX "ACPI: " > > @@ -387,6 +388,23 @@ static inline int acpi_isa_register_gsi(struct pci_dev *dev) > } > #endif > > +static inline bool acpi_pci_irq_valid(struct pci_dev *dev) > +{ > +#ifdef CONFIG_X86 > + /* > + * On x86 irq line 0xff means "unknown" or "no connection" > + * (PCI 3.0, Section 6.2.4, footnote on page 223). > + */ > + if (dev->irq == 0xff) { > + dev->irq = IRQ_NOTCONNECTED; > + dev_warn(&dev->dev, "PCI INT %c: not connected\n", > + pin_name(pin)); > + return false; > + } > +#endif > + return true; > +} > + > int acpi_pci_irq_enable(struct pci_dev *dev) > { > struct acpi_prt_entry *entry; > @@ -431,11 +449,14 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > } else > gsi = -1; > > - /* > - * No IRQ known to the ACPI subsystem - maybe the BIOS / > - * driver reported one, then use it. Exit in any case. > - */ > if (gsi < 0) { > + /* > + * No IRQ known to the ACPI subsystem - maybe the BIOS / > + * driver reported one, then use it. Exit in any case. > + */ > + if (!acpi_pci_irq_valid(dev, pin)) > + return 0; > + > if (acpi_isa_register_gsi(dev)) > dev_warn(&dev->dev, "PCI INT %c: no GSI\n", > pin_name(pin)); > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index cb30edb..12f7da4 100644 > --- a/include/linux/interrupt.h > +++ b/include/linux/interrupt.h > @@ -125,6 +125,16 @@ struct irqaction { > > extern irqreturn_t no_action(int cpl, void *dev_id); > > +/* > + * If a (PCI) device interrupt is not connected we set dev->irq to > + * IRQ_NOTCONNECTED. This causes request_irq() to fail with -ENOTCONN, so we > + * can distingiush that case from other error returns. > + * > + * 0x80000000 is guaranteed to be outside the available range of interrupts > + * and easy to distinguish from other possible incorrect values. > + */ > +#define IRQ_NOTCONNECTED (1U << 31) > + > extern int __must_check > request_threaded_irq(unsigned int irq, irq_handler_t handler, > irq_handler_t thread_fn, > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 8411872..e79e60f 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1609,6 +1609,9 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler, > struct irq_desc *desc; > int retval; > > + if (irq == IRQ_NOTCONNECTED) > + return -ENOTCONN; > + > /* > * Sanity-check: shared interrupts must pass in a real dev-ID, > * otherwise we'll have trouble later trying to figure out > @@ -1699,9 +1702,13 @@ EXPORT_SYMBOL(request_threaded_irq); > int request_any_context_irq(unsigned int irq, irq_handler_t handler, > unsigned long flags, const char *name, void *dev_id) > { > - struct irq_desc *desc = irq_to_desc(irq); > + struct irq_desc *desc; > int ret; > > + if (irq == IRQ_NOTCONNECTED) > + return -ENOTCONN; > + > + desc = irq_to_desc(irq); > if (!desc) > return -EINVAL; > -- 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 --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index d30184c..0227cc6 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -33,6 +33,7 @@ #include <linux/pci.h> #include <linux/acpi.h> #include <linux/slab.h> +#include <linux/interrupt.h> #define PREFIX "ACPI: " @@ -387,6 +388,23 @@ static inline int acpi_isa_register_gsi(struct pci_dev *dev) } #endif +static inline bool acpi_pci_irq_valid(struct pci_dev *dev) +{ +#ifdef CONFIG_X86 + /* + * On x86 irq line 0xff means "unknown" or "no connection" + * (PCI 3.0, Section 6.2.4, footnote on page 223). + */ + if (dev->irq == 0xff) { + dev->irq = IRQ_NOTCONNECTED; + dev_warn(&dev->dev, "PCI INT %c: not connected\n", + pin_name(pin)); + return false; + } +#endif + return true; +} + int acpi_pci_irq_enable(struct pci_dev *dev) { struct acpi_prt_entry *entry; @@ -431,11 +449,14 @@ int acpi_pci_irq_enable(struct pci_dev *dev) } else gsi = -1; - /* - * No IRQ known to the ACPI subsystem - maybe the BIOS / - * driver reported one, then use it. Exit in any case. - */ if (gsi < 0) { + /* + * No IRQ known to the ACPI subsystem - maybe the BIOS / + * driver reported one, then use it. Exit in any case. + */ + if (!acpi_pci_irq_valid(dev, pin)) + return 0; + if (acpi_isa_register_gsi(dev)) dev_warn(&dev->dev, "PCI INT %c: no GSI\n", pin_name(pin)); diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index cb30edb..12f7da4 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -125,6 +125,16 @@ struct irqaction { extern irqreturn_t no_action(int cpl, void *dev_id); +/* + * If a (PCI) device interrupt is not connected we set dev->irq to + * IRQ_NOTCONNECTED. This causes request_irq() to fail with -ENOTCONN, so we + * can distingiush that case from other error returns. + * + * 0x80000000 is guaranteed to be outside the available range of interrupts + * and easy to distinguish from other possible incorrect values. + */ +#define IRQ_NOTCONNECTED (1U << 31) + extern int __must_check request_threaded_irq(unsigned int irq, irq_handler_t handler, irq_handler_t thread_fn, diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 8411872..e79e60f 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1609,6 +1609,9 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler, struct irq_desc *desc; int retval; + if (irq == IRQ_NOTCONNECTED) + return -ENOTCONN; + /* * Sanity-check: shared interrupts must pass in a real dev-ID, * otherwise we'll have trouble later trying to figure out @@ -1699,9 +1702,13 @@ EXPORT_SYMBOL(request_threaded_irq); int request_any_context_irq(unsigned int irq, irq_handler_t handler, unsigned long flags, const char *name, void *dev_id) { - struct irq_desc *desc = irq_to_desc(irq); + struct irq_desc *desc; int ret; + if (irq == IRQ_NOTCONNECTED) + return -ENOTCONN; + + desc = irq_to_desc(irq); if (!desc) return -EINVAL;
Per the x86-specific footnote to PCI spec r3.0, sec 6.2.4, the value 255 in the Interrupt Line register means "unknown" or "no connection." Previously, when we couldn't derive an IRQ from the _PRT, we fell back to using the value from Interrupt Line as an IRQ. It's questionable whether we should do that at all, but the spec clearly suggests we shouldn't do it for the value 255 on x86. Calling request_irq() with IRQ 255 may succeed, but the driver won't receive any interrupts. Or, if IRQ 255 is shared with another device, it may succeed, and the driver's ISR will be called at random times when the *other* device interrupts. Or it may fail if another device is using IRQ 255 with incompatible flags. What we *want* is for request_irq() to fail predictably so the driver can fall back to polling. On x86, assume 255 in the Interrupt Line means the INTx line is not connected. In that case, set dev->irq to IRQ_NOTCONNECTED so request_irq() will fail gracefully with -ENOTCONN. We found this problem on a system where Secure Boot firmware assigned Interrupt Line 255 to an i801_smbus device and another device was already using MSI-X IRQ 255. This was in v3.10, where i801_probe() fails if request_irq() fails: i801_smbus 0000:00:1f.3: enabling device (0140 -> 0143) i801_smbus 0000:00:1f.3: can't derive routing for PCI INT C i801_smbus 0000:00:1f.3: PCI INT C: no GSI genirq: Flags mismatch irq 255. 00000080 (i801_smbus) vs. 00000000 (megasa) CPU: 0 PID: 2487 Comm: kworker/0:1 Not tainted 3.10.0-229.el7.x86_64 #1 Hardware name: FUJITSU PRIMEQUEST 2800E2/D3736, BIOS PRIMEQUEST 2000 Serie5 Call Trace: dump_stack+0x19/0x1b __setup_irq+0x54a/0x570 request_threaded_irq+0xcc/0x170 i801_probe+0x32f/0x508 [i2c_i801] local_pci_probe+0x45/0xa0 i801_smbus 0000:00:1f.3: Failed to allocate irq 255: -16 i801_smbus: probe of 0000:00:1f.3 failed with error -16 After aeb8a3d16ae0 ("i2c: i801: Check if interrupts are disabled"), i801_probe() will fall back to polling if request_irq() fails. But we still need this patch because request_irq() may succeed or fail depending on other devices in the system. If request_irq() fails, i801_smbus will work by falling back to polling, but if it succeeds, i801_smbus won't work because it expects interrupts that it may not receive. Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> Acked-by: Thomas Gleixner <tglx@linutronix.de> Acked-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/acpi/pci_irq.c | 29 +++++++++++++++++++++++++---- include/linux/interrupt.h | 10 ++++++++++ kernel/irq/manage.c | 9 ++++++++- 3 files changed, 43 insertions(+), 5 deletions(-)