| Message ID | alpine.DEB.2.11.1601270941190.3886@nanos (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Jan 27, 2016 at 10:13:36AM +0100, Thomas Gleixner wrote: > On Tue, 26 Jan 2016, Bjorn Helgaas wrote: > > On Tue, Jan 26, 2016 at 04:48:25PM +0100, Thomas Gleixner wrote: > > > Right. So we could certainly do something like this INVALID_IRQ thingy, but > > > that looks a bit weird. What would request_irq() return? > > > > > > If it returns success, then drivers might make the wrong decision. If it > > > returns an error code, then the i801 one works, but we might have to fix > > > others anyway. > > > > I was thinking request_irq() could return -EINVAL if the caller passed > > INVALID_IRQ. That should tell drivers that this interrupt won't work. > > > > We'd be making request_irq() return -EINVAL in some cases where it > > currently returns success. But even though it returns success today, > > I don't think the driver is getting interrupts, because the wire isn't > > connected. > > Correct. What I meant is that the i801 driver can handle the -EINVAL return > from request_irq() today, but other drivers don't. I agree that we need to fix > them anyway and a failure to request the interrupt is better than a silent 'no > interrupts delivered' issue. > > Though instead of returning -EINVAL I prefer an explicit error code for this > case. That way a driver can distinguish between the 'not connected' case and > other failure modes. Something like the patch below should work. This patch looks great to me, thanks for all your help! Chen, do you want to put all this together as formal patches with changelogs and post to the mailing lists? Bjorn > 8<------------------ > --- a/drivers/acpi/pci_irq.c > +++ b/drivers/acpi/pci_irq.c > @@ -387,6 +387,22 @@ static inline int acpi_isa_register_gsi( > } > #endif > > +static inline bool acpi_pci_irq_valid(struc 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 not connected\n"); > + return false; > + } > +#endif > + return true; > +} > + > int acpi_pci_irq_enable(struct pci_dev *dev) > { > struct acpi_prt_entry *entry; > @@ -409,6 +425,9 @@ int acpi_pci_irq_enable(struct pci_dev * > if (pci_has_managed_irq(dev)) > return 0; > > + if (!acpi_pci_irq_valid(dev)) > + return 0; > + > entry = acpi_pci_irq_lookup(dev, pin); > if (!entry) { > /* > --- 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, > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1609,6 +1609,9 @@ int request_threaded_irq(unsigned int ir > 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 -- 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 01/28/2016 06:32 AM, Bjorn Helgaas wrote: > On Wed, Jan 27, 2016 at 10:13:36AM +0100, Thomas Gleixner wrote: >> On Tue, 26 Jan 2016, Bjorn Helgaas wrote: >>> On Tue, Jan 26, 2016 at 04:48:25PM +0100, Thomas Gleixner wrote: >>>> Right. So we could certainly do something like this INVALID_IRQ thingy, but >>>> that looks a bit weird. What would request_irq() return? >>>> >>>> If it returns success, then drivers might make the wrong decision. If it >>>> returns an error code, then the i801 one works, but we might have to fix >>>> others anyway. >>> I was thinking request_irq() could return -EINVAL if the caller passed >>> INVALID_IRQ. That should tell drivers that this interrupt won't work. >>> >>> We'd be making request_irq() return -EINVAL in some cases where it >>> currently returns success. But even though it returns success today, >>> I don't think the driver is getting interrupts, because the wire isn't >>> connected. >> Correct. What I meant is that the i801 driver can handle the -EINVAL return >> from request_irq() today, but other drivers don't. I agree that we need to fix >> them anyway and a failure to request the interrupt is better than a silent 'no >> interrupts delivered' issue. >> >> Though instead of returning -EINVAL I prefer an explicit error code for this >> case. That way a driver can distinguish between the 'not connected' case and >> other failure modes. Something like the patch below should work. > This patch looks great to me, thanks for all your help! > > Chen, do you want to put all this together as formal patches with > changelogs and post to the mailing lists? With pleasure. I will test it on my environment first and send it out. thank all of you for this problem. Chen > > Bjorn > >> 8<------------------ >> --- a/drivers/acpi/pci_irq.c >> +++ b/drivers/acpi/pci_irq.c >> @@ -387,6 +387,22 @@ static inline int acpi_isa_register_gsi( >> } >> #endif >> >> +static inline bool acpi_pci_irq_valid(struc 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 not connected\n"); >> + return false; >> + } >> +#endif >> + return true; >> +} >> + >> int acpi_pci_irq_enable(struct pci_dev *dev) >> { >> struct acpi_prt_entry *entry; >> @@ -409,6 +425,9 @@ int acpi_pci_irq_enable(struct pci_dev * >> if (pci_has_managed_irq(dev)) >> return 0; >> >> + if (!acpi_pci_irq_valid(dev)) >> + return 0; >> + >> entry = acpi_pci_irq_lookup(dev, pin); >> if (!entry) { >> /* >> --- 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, >> --- a/kernel/irq/manage.c >> +++ b/kernel/irq/manage.c >> @@ -1609,6 +1609,9 @@ int request_threaded_irq(unsigned int ir >> 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 > > . > -- 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
--- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -387,6 +387,22 @@ static inline int acpi_isa_register_gsi( } #endif +static inline bool acpi_pci_irq_valid(struc 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 not connected\n"); + return false; + } +#endif + return true; +} + int acpi_pci_irq_enable(struct pci_dev *dev) { struct acpi_prt_entry *entry; @@ -409,6 +425,9 @@ int acpi_pci_irq_enable(struct pci_dev * if (pci_has_managed_irq(dev)) return 0; + if (!acpi_pci_irq_valid(dev)) + return 0; + entry = acpi_pci_irq_lookup(dev, pin); if (!entry) { /* --- 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, --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1609,6 +1609,9 @@ int request_threaded_irq(unsigned int ir 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;