| Message ID | d004975f-03a8-59f5-56ef-f4e1f8e12dfb@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: let pci_request_irq properly deal with threaded interrupts | expand |
[+cc Thomas, Christoph, LKML] On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > If we have a threaded interrupt with the handler being NULL, then > request_threaded_irq() -> __setup_irq() will complain and bail out > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > being NULL and set IRQF_ONESHOT in this case. > > This change is needed to migrate the mei_me driver to > pci_alloc_irq_vectors() and pci_request_irq(). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT usage isn't mentioned in the request_threaded_irq() function doc or Documentation/ > --- > drivers/pci/irq.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c > index 2a808e10..a1de501a 100644 > --- a/drivers/pci/irq.c > +++ b/drivers/pci/irq.c > @@ -86,13 +86,17 @@ int pci_request_irq(struct pci_dev *dev, unsigned int nr, irq_handler_t handler, > va_list ap; > int ret; > char *devname; > + unsigned long irqflags = IRQF_SHARED; > + > + if (!handler) > + irqflags |= IRQF_ONESHOT; > > va_start(ap, fmt); > devname = kvasprintf(GFP_KERNEL, fmt, ap); > va_end(ap); > > ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn, > - IRQF_SHARED, devname, dev_id); > + irqflags, devname, dev_id); > if (ret) > kfree(devname); > return ret; > -- > 2.18.0 >
On 30.07.2018 23:30, Bjorn Helgaas wrote: > [+cc Thomas, Christoph, LKML] > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: >> If we have a threaded interrupt with the handler being NULL, then >> request_threaded_irq() -> __setup_irq() will complain and bail out >> if the IRQF_ONESHOT flag isn't set. Therefore check for the handler >> being NULL and set IRQF_ONESHOT in this case. >> >> This change is needed to migrate the mei_me driver to >> pci_alloc_irq_vectors() and pci_request_irq(). >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > usage isn't mentioned in the request_threaded_irq() function doc or > Documentation/ > Sure. That's the related comment in __setup_irq(): * The interrupt was requested with handler = NULL, so * we use the default primary handler for it. But it * does not have the oneshot flag set. In combination * with level interrupts this is deadly, because the * default primary handler just wakes the thread, then * the irq lines is reenabled, but the device still * has the level irq asserted. Rinse and repeat.... * * While this works for edge type interrupts, we play * it safe and reject unconditionally because we can't * say for sure which type this interrupt really * has. The type flags are unreliable as the * underlying chip implementation can override them. */ >> --- >> drivers/pci/irq.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c >> index 2a808e10..a1de501a 100644 >> --- a/drivers/pci/irq.c >> +++ b/drivers/pci/irq.c >> @@ -86,13 +86,17 @@ int pci_request_irq(struct pci_dev *dev, unsigned int nr, irq_handler_t handler, >> va_list ap; >> int ret; >> char *devname; >> + unsigned long irqflags = IRQF_SHARED; >> + >> + if (!handler) >> + irqflags |= IRQF_ONESHOT; >> >> va_start(ap, fmt); >> devname = kvasprintf(GFP_KERNEL, fmt, ap); >> va_end(ap); >> >> ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn, >> - IRQF_SHARED, devname, dev_id); >> + irqflags, devname, dev_id); >> if (ret) >> kfree(devname); >> return ret; >> -- >> 2.18.0 >> >
On Tue, Jul 31, 2018 at 12:30 AM, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Thomas, Christoph, LKML] > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: >> If we have a threaded interrupt with the handler being NULL, then >> request_threaded_irq() -> __setup_irq() will complain and bail out >> if the IRQF_ONESHOT flag isn't set. Therefore check for the handler >> being NULL and set IRQF_ONESHOT in this case. >> >> This change is needed to migrate the mei_me driver to >> pci_alloc_irq_vectors() and pci_request_irq(). >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > usage isn't mentioned in the request_threaded_irq() function doc or > Documentation/ * IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished. * Used by threaded interrupts which need to keep the * irq line disabled until the threaded handler has been run.
On Mon, 30 Jul 2018, Bjorn Helgaas wrote: > [+cc Thomas, Christoph, LKML] + Marc > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > > If we have a threaded interrupt with the handler being NULL, then > > request_threaded_irq() -> __setup_irq() will complain and bail out > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > > being NULL and set IRQF_ONESHOT in this case. > > > > This change is needed to migrate the mei_me driver to > > pci_alloc_irq_vectors() and pci_request_irq(). > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > usage isn't mentioned in the request_threaded_irq() function doc or > Documentation/ Right. The documentation really needs some love and care. :( Yes, request for pure threaded interrupts are rejected if the oneshot flag is not set. The reason is that this would be deadly especially with level triggered interrupts because the primary default handler just wakes the thread and then reenables interrupts, which will make the interrupt come back immediately and the thread won't have a chance to actually shut it up in the device. That made me look into that code again and I found that we added a flag for irq chips to tell the core that the interrupt is one shot safe, i.e. that it can be requested w/o IRQF_ONESHOT. That was initially added to optimize MSI based interrupts which are oneshot safe by implementation. dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe") The original patch added that flag to the x86 MSI irqchip code, but that part was not applied for reasons which slipped from memory. It might be worthwhile to revisit that in order to avoid the mask/unmask overhead for such cases. Anyway for the patch in question: Reviewed-by: Thomas Gleixner <tglx@linutronix.de> Thanks, tglx
[+cc maintainers of possibly erroneous callers of request_threaded_irq()] On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote: > [+cc Thomas, Christoph, LKML] > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > > If we have a threaded interrupt with the handler being NULL, then > > request_threaded_irq() -> __setup_irq() will complain and bail out > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > > being NULL and set IRQF_ONESHOT in this case. > > > > This change is needed to migrate the mei_me driver to > > pci_alloc_irq_vectors() and pci_request_irq(). > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > usage isn't mentioned in the request_threaded_irq() function doc or > Documentation/ Possibly these other request_threaded_irq() callers are similarly broken? I can't tell for sure about tda998x_create(), but all the others certainly call request_threaded_irq() with "handler == NULL" and irqflags that do not contain IRQF_ONESHOT: max8997_muic_probe() request_threaded_irq(virq, NULL, ..., IRQF_NO_SUSPEND, ...) tda998x_create() request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...) (I can't tell what irqd_get_trigger_type() does) ab8500_btemp_probe() ab8500_charger_probe() request_threaded_irq(irq, NULL, ..., IRQF_SHARED | IRQF_NO_SUSPEND, ...) lp8788_set_irqs() request_threaded_irq(virq, NULL, ..., 0, ...) max77686_rtc_probe() request_threaded_irq(info->virq, NULL, ..., 0, ...) wm8350_register_irq() request_threaded_irq(irq + wm8350->irq_base, NULL, ..., flags, ...) (I think all callers of wm8350_register_irq() supply 0 for "flags") > > --- > > drivers/pci/irq.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c > > index 2a808e10..a1de501a 100644 > > --- a/drivers/pci/irq.c > > +++ b/drivers/pci/irq.c > > @@ -86,13 +86,17 @@ int pci_request_irq(struct pci_dev *dev, unsigned int nr, irq_handler_t handler, > > va_list ap; > > int ret; > > char *devname; > > + unsigned long irqflags = IRQF_SHARED; > > + > > + if (!handler) > > + irqflags |= IRQF_ONESHOT; > > > > va_start(ap, fmt); > > devname = kvasprintf(GFP_KERNEL, fmt, ap); > > va_end(ap); > > > > ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn, > > - IRQF_SHARED, devname, dev_id); > > + irqflags, devname, dev_id); > > if (ret) > > kfree(devname); > > return ret; > > -- > > 2.18.0 > >
On Mon, 30 Jul 2018, Bjorn Helgaas wrote: > [+cc maintainers of possibly erroneous callers of request_threaded_irq()] > > On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote: > > [+cc Thomas, Christoph, LKML] > > > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > > > If we have a threaded interrupt with the handler being NULL, then > > > request_threaded_irq() -> __setup_irq() will complain and bail out > > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > > > being NULL and set IRQF_ONESHOT in this case. > > > > > > This change is needed to migrate the mei_me driver to > > > pci_alloc_irq_vectors() and pci_request_irq(). > > > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > > usage isn't mentioned in the request_threaded_irq() function doc or > > Documentation/ > > Possibly these other request_threaded_irq() callers are similarly > broken? I can't tell for sure about tda998x_create(), but all the > others certainly call request_threaded_irq() with "handler == NULL" > and irqflags that do not contain IRQF_ONESHOT: > > max8997_muic_probe() > request_threaded_irq(virq, NULL, ..., IRQF_NO_SUSPEND, ...) > > tda998x_create() > request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...) > (I can't tell what irqd_get_trigger_type() does) It reads the trigger type back from the irq chip (level/edge/polarity) but does not return with the ONESHOT bit set. > ab8500_btemp_probe() > ab8500_charger_probe() > request_threaded_irq(irq, NULL, ..., IRQF_SHARED | IRQF_NO_SUSPEND, ...) SHARED is interesting .... > lp8788_set_irqs() > request_threaded_irq(virq, NULL, ..., 0, ...) > > max77686_rtc_probe() > request_threaded_irq(info->virq, NULL, ..., 0, ...) > > wm8350_register_irq() > request_threaded_irq(irq + wm8350->irq_base, NULL, ..., flags, ...) > (I think all callers of wm8350_register_irq() supply 0 for "flags") Indeed. This all looks pretty much wrong. No idea why nobody ever noticed. Thanks, tglx
On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > If we have a threaded interrupt with the handler being NULL, then > request_threaded_irq() -> __setup_irq() will complain and bail out > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > being NULL and set IRQF_ONESHOT in this case. > > This change is needed to migrate the mei_me driver to > pci_alloc_irq_vectors() and pci_request_irq(). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied with Thomas' reviewed-by to pci/misc for v4.19, thanks! > --- > drivers/pci/irq.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c > index 2a808e10..a1de501a 100644 > --- a/drivers/pci/irq.c > +++ b/drivers/pci/irq.c > @@ -86,13 +86,17 @@ int pci_request_irq(struct pci_dev *dev, unsigned int nr, irq_handler_t handler, > va_list ap; > int ret; > char *devname; > + unsigned long irqflags = IRQF_SHARED; > + > + if (!handler) > + irqflags |= IRQF_ONESHOT; > > va_start(ap, fmt); > devname = kvasprintf(GFP_KERNEL, fmt, ap); > va_end(ap); > > ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn, > - IRQF_SHARED, devname, dev_id); > + irqflags, devname, dev_id); > if (ret) > kfree(devname); > return ret; > -- > 2.18.0 >
On Mon, 30 Jul 2018 23:36:57 +0100, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Mon, 30 Jul 2018, Bjorn Helgaas wrote: > > > [+cc Thomas, Christoph, LKML] > > + Marc > > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > > > If we have a threaded interrupt with the handler being NULL, then > > > request_threaded_irq() -> __setup_irq() will complain and bail out > > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > > > being NULL and set IRQF_ONESHOT in this case. > > > > > > This change is needed to migrate the mei_me driver to > > > pci_alloc_irq_vectors() and pci_request_irq(). > > > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > > usage isn't mentioned in the request_threaded_irq() function doc or > > Documentation/ > > Right. The documentation really needs some love and care. :( > > Yes, request for pure threaded interrupts are rejected if the oneshot flag > is not set. The reason is that this would be deadly especially with level > triggered interrupts because the primary default handler just wakes the > thread and then reenables interrupts, which will make the interrupt come > back immediately and the thread won't have a chance to actually shut it up > in the device. > > That made me look into that code again and I found that we added a flag for > irq chips to tell the core that the interrupt is one shot safe, i.e. that > it can be requested w/o IRQF_ONESHOT. That was initially added to optimize > MSI based interrupts which are oneshot safe by implementation. > > dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe") > > The original patch added that flag to the x86 MSI irqchip code, but that > part was not applied for reasons which slipped from memory. It might be > worthwhile to revisit that in order to avoid the mask/unmask overhead for > such cases. Yup, that would actually be beneficial to a range of interrupt controllers (only an obscure GPIO driver makes use of this flag). We could also consider extending this to support interrupt hierarchies, as __setup_irq() seems only concerned with the top of the stack (an IRQ provided by a generic MSI stack and backed by an irqchip providing IRQCHIP_ONESHOT_SAFE would go unnoticed). M.
On Tue, 31 Jul 2018, Thomas Gleixner wrote: > On Mon, 30 Jul 2018, Bjorn Helgaas wrote: > > > [+cc maintainers of possibly erroneous callers of request_threaded_irq()] > > > > On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote: > > > [+cc Thomas, Christoph, LKML] > > > > > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > > > > If we have a threaded interrupt with the handler being NULL, then > > > > request_threaded_irq() -> __setup_irq() will complain and bail out > > > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > > > > being NULL and set IRQF_ONESHOT in this case. > > > > > > > > This change is needed to migrate the mei_me driver to > > > > pci_alloc_irq_vectors() and pci_request_irq(). > > > > > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > > > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > > > usage isn't mentioned in the request_threaded_irq() function doc or > > > Documentation/ > > > > Possibly these other request_threaded_irq() callers are similarly > > broken? I can't tell for sure about tda998x_create(), but all the > > others certainly call request_threaded_irq() with "handler == NULL" > > and irqflags that do not contain IRQF_ONESHOT: > > > > max8997_muic_probe() > > request_threaded_irq(virq, NULL, ..., IRQF_NO_SUSPEND, ...) > > > > tda998x_create() > > request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...) > > (I can't tell what irqd_get_trigger_type() does) > > It reads the trigger type back from the irq chip (level/edge/polarity) but > does not return with the ONESHOT bit set. > > > ab8500_btemp_probe() > > ab8500_charger_probe() > > request_threaded_irq(irq, NULL, ..., IRQF_SHARED | IRQF_NO_SUSPEND, ...) > > SHARED is interesting .... > > > lp8788_set_irqs() > > request_threaded_irq(virq, NULL, ..., 0, ...) > > > > max77686_rtc_probe() > > request_threaded_irq(info->virq, NULL, ..., 0, ...) > > > > wm8350_register_irq() > > request_threaded_irq(irq + wm8350->irq_base, NULL, ..., flags, ...) > > (I think all callers of wm8350_register_irq() supply 0 for "flags") > > Indeed. This all looks pretty much wrong. No idea why nobody ever noticed. I guess not many people (myself included) know the intricacies well enough to have noticed. This probably coupled with insufficient checking/warning about such practices in the IRQ subsystem. There are quite a few different devices here, so my assumption would be that interrupt handling still works in these cases. If we know certain configurations are incorrect or incompatible, might I suggest we add some form of alert to bring these to the attention of unknowing contributors?
On Tue, Jul 31, 2018 at 12:50:21AM +0200, Thomas Gleixner wrote: > On Mon, 30 Jul 2018, Bjorn Helgaas wrote: > > > [+cc maintainers of possibly erroneous callers of request_threaded_irq()] > > > > On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote: > > > [+cc Thomas, Christoph, LKML] > > > > > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > > > > If we have a threaded interrupt with the handler being NULL, then > > > > request_threaded_irq() -> __setup_irq() will complain and bail out > > > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > > > > being NULL and set IRQF_ONESHOT in this case. > > > > > > > > This change is needed to migrate the mei_me driver to > > > > pci_alloc_irq_vectors() and pci_request_irq(). > > > > > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > > > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > > > usage isn't mentioned in the request_threaded_irq() function doc or > > > Documentation/ > > > > Possibly these other request_threaded_irq() callers are similarly > > broken? I can't tell for sure about tda998x_create(), but all the > > others certainly call request_threaded_irq() with "handler == NULL" > > and irqflags that do not contain IRQF_ONESHOT: > > > > tda998x_create() > > request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...) > > (I can't tell what irqd_get_trigger_type() does) > > It reads the trigger type back from the irq chip (level/edge/polarity) but > does not return with the ONESHOT bit set. What tree are you (Bjorn) using there? Looking in my git history, request_threaded_irq() was added in January 2014, as: + irqf_trigger = + irqd_get_trigger_type(irq_get_irq_data(client->irq)); + ret = request_threaded_irq(client->irq, NULL, + tda998x_irq_thread, + irqf_trigger | IRQF_ONESHOT, + "tda998x", priv); and updated more recently for the CEC support in ae81553c30ef ("drm/i2c: tda998x: allow interrupt to be shared") to be: - irqf_trigger = + irq_flags = irqd_get_trigger_type(irq_get_irq_data(client->irq)); + irq_flags |= IRQF_SHARED | IRQF_ONESHOT; ret = request_threaded_irq(client->irq, NULL, - tda998x_irq_thread, - irqf_trigger | IRQF_ONESHOT, + tda998x_irq_thread, irq_flags, "tda998x", priv); In all cases, IRQF_ONESHOT has been set for this threaded interrupt. I've also checked drm-next, and it doesn't show anything like what Bjorn quoted. Confused. Bjorn, can you double check that what you think are erroneous request_threaded_irq() calls are actually as per what you quoted please?
On 31 July 2018 at 00:42, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc maintainers of possibly erroneous callers of request_threaded_irq()] > > On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote: >> [+cc Thomas, Christoph, LKML] >> >> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: >> > If we have a threaded interrupt with the handler being NULL, then >> > request_threaded_irq() -> __setup_irq() will complain and bail out >> > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler >> > being NULL and set IRQF_ONESHOT in this case. >> > >> > This change is needed to migrate the mei_me driver to >> > pci_alloc_irq_vectors() and pci_request_irq(). >> > >> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> >> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT >> usage isn't mentioned in the request_threaded_irq() function doc or >> Documentation/ > > Possibly these other request_threaded_irq() callers are similarly > broken? I can't tell for sure about tda998x_create(), but all the > others certainly call request_threaded_irq() with "handler == NULL" > and irqflags that do not contain IRQF_ONESHOT: > > max8997_muic_probe() > request_threaded_irq(virq, NULL, ..., IRQF_NO_SUSPEND, ...) > > tda998x_create() > request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...) > (I can't tell what irqd_get_trigger_type() does) > > ab8500_btemp_probe() > ab8500_charger_probe() > request_threaded_irq(irq, NULL, ..., IRQF_SHARED | IRQF_NO_SUSPEND, ...) > > lp8788_set_irqs() > request_threaded_irq(virq, NULL, ..., 0, ...) > > max77686_rtc_probe() > request_threaded_irq(info->virq, NULL, ..., 0, ...) max77686 works fine because it it is nested IRQ. Parent has ONESHOT flag set. Best regards, Krzysztof
On Tue, Jul 31, 2018 at 09:01:12AM +0100, Russell King - ARM Linux wrote: > On Tue, Jul 31, 2018 at 12:50:21AM +0200, Thomas Gleixner wrote: > > On Mon, 30 Jul 2018, Bjorn Helgaas wrote: > > > > > [+cc maintainers of possibly erroneous callers of request_threaded_irq()] > > > > > > On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote: > > > > [+cc Thomas, Christoph, LKML] > > > > > > > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: > > > > > If we have a threaded interrupt with the handler being NULL, then > > > > > request_threaded_irq() -> __setup_irq() will complain and bail out > > > > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler > > > > > being NULL and set IRQF_ONESHOT in this case. > > > > > > > > > > This change is needed to migrate the mei_me driver to > > > > > pci_alloc_irq_vectors() and pci_request_irq(). > > > > > > > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > > > > > > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT > > > > usage isn't mentioned in the request_threaded_irq() function doc or > > > > Documentation/ > > > > > > Possibly these other request_threaded_irq() callers are similarly > > > broken? I can't tell for sure about tda998x_create(), but all the > > > others certainly call request_threaded_irq() with "handler == NULL" > > > and irqflags that do not contain IRQF_ONESHOT: > > > > > > tda998x_create() > > > request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...) > > > (I can't tell what irqd_get_trigger_type() does) > > > > It reads the trigger type back from the irq chip (level/edge/polarity) but > > does not return with the ONESHOT bit set. > > What tree are you (Bjorn) using there? Looking in my git history, > request_threaded_irq() was added in January 2014, as: > > + irqf_trigger = > + irqd_get_trigger_type(irq_get_irq_data(client->irq)); > + ret = request_threaded_irq(client->irq, NULL, > + tda998x_irq_thread, > + irqf_trigger | IRQF_ONESHOT, > + "tda998x", priv); > > and updated more recently for the CEC support in ae81553c30ef > ("drm/i2c: tda998x: allow interrupt to be shared") to be: > > - irqf_trigger = > + irq_flags = > irqd_get_trigger_type(irq_get_irq_data(client->irq)); > + irq_flags |= IRQF_SHARED | IRQF_ONESHOT; > ret = request_threaded_irq(client->irq, NULL, > - tda998x_irq_thread, > - irqf_trigger | IRQF_ONESHOT, > + tda998x_irq_thread, irq_flags, > "tda998x", priv); > > In all cases, IRQF_ONESHOT has been set for this threaded interrupt. > > I've also checked drm-next, and it doesn't show anything like what > Bjorn quoted. > > Confused. Bjorn, can you double check that what you think are > erroneous request_threaded_irq() calls are actually as per what > you quoted please? I was looking at v4.18-rc1, but in the case of tda998x_create(), I overlooked this line: irq_flags |= IRQF_SHARED | IRQF_ONESHOT; Sorry!
On 31.07.2018 08:15, Marc Zyngier wrote: > On Mon, 30 Jul 2018 23:36:57 +0100, > Thomas Gleixner <tglx@linutronix.de> wrote: >> >> On Mon, 30 Jul 2018, Bjorn Helgaas wrote: >> >>> [+cc Thomas, Christoph, LKML] >> >> + Marc >> >>> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote: >>>> If we have a threaded interrupt with the handler being NULL, then >>>> request_threaded_irq() -> __setup_irq() will complain and bail out >>>> if the IRQF_ONESHOT flag isn't set. Therefore check for the handler >>>> being NULL and set IRQF_ONESHOT in this case. >>>> >>>> This change is needed to migrate the mei_me driver to >>>> pci_alloc_irq_vectors() and pci_request_irq(). >>>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> >>> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT >>> usage isn't mentioned in the request_threaded_irq() function doc or >>> Documentation/ >> >> Right. The documentation really needs some love and care. :( >> >> Yes, request for pure threaded interrupts are rejected if the oneshot flag >> is not set. The reason is that this would be deadly especially with level >> triggered interrupts because the primary default handler just wakes the >> thread and then reenables interrupts, which will make the interrupt come >> back immediately and the thread won't have a chance to actually shut it up >> in the device. >> >> That made me look into that code again and I found that we added a flag for >> irq chips to tell the core that the interrupt is one shot safe, i.e. that >> it can be requested w/o IRQF_ONESHOT. That was initially added to optimize >> MSI based interrupts which are oneshot safe by implementation. >> >> dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe") >> >> The original patch added that flag to the x86 MSI irqchip code, but that >> part was not applied for reasons which slipped from memory. It might be >> worthwhile to revisit that in order to avoid the mask/unmask overhead for >> such cases. > > Yup, that would actually be beneficial to a range of interrupt > controllers (only an obscure GPIO driver makes use of this flag). > I'm not an expert on that area, but if MSI is oneshot-safe in general, then we could do the following in the framework instead of touching every MSI irq_chip. Opinions? I tested on X86 and at least my system is still running. diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c index 4ca2fd46..ba6da943 100644 --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode, if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) msi_domain_update_chip_ops(info); + /* MSI is oneshot-safe in general */ + info->chip->flags |= IRQCHIP_ONESHOT_SAFE; + domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0, fwnode, &msi_domain_ops, info); -- 2.18.0 > We could also consider extending this to support interrupt > hierarchies, as __setup_irq() seems only concerned with the top of the > stack (an IRQ provided by a generic MSI stack and backed by an irqchip > providing IRQCHIP_ONESHOT_SAFE would go unnoticed). > Would it be sufficient if one irq_chip in the hierarchy is oneshot-safe? Or what do we have to check for? > M. > Heiner
On Wed, 1 Aug 2018, Heiner Kallweit wrote: > On 31.07.2018 08:15, Marc Zyngier wrote: > > On Mon, 30 Jul 2018 23:36:57 +0100, > > Thomas Gleixner <tglx@linutronix.de> wrote: > >> > >> Yes, request for pure threaded interrupts are rejected if the oneshot flag > >> is not set. The reason is that this would be deadly especially with level > >> triggered interrupts because the primary default handler just wakes the > >> thread and then reenables interrupts, which will make the interrupt come > >> back immediately and the thread won't have a chance to actually shut it up > >> in the device. > >> > >> That made me look into that code again and I found that we added a flag for > >> irq chips to tell the core that the interrupt is one shot safe, i.e. that > >> it can be requested w/o IRQF_ONESHOT. That was initially added to optimize > >> MSI based interrupts which are oneshot safe by implementation. > >> > >> dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe") > >> > >> The original patch added that flag to the x86 MSI irqchip code, but that > >> part was not applied for reasons which slipped from memory. It might be > >> worthwhile to revisit that in order to avoid the mask/unmask overhead for > >> such cases. > > > > Yup, that would actually be beneficial to a range of interrupt > > controllers (only an obscure GPIO driver makes use of this flag). > > > > I'm not an expert on that area, but if MSI is oneshot-safe in general, > then we could do the following in the framework instead of touching > every MSI irq_chip. Opinions? > I tested on X86 and at least my system is still running. > > diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c > index 4ca2fd46..ba6da943 100644 > --- a/kernel/irq/msi.c > +++ b/kernel/irq/msi.c > @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode, > if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) > msi_domain_update_chip_ops(info); > > + /* MSI is oneshot-safe in general */ > + info->chip->flags |= IRQCHIP_ONESHOT_SAFE; > + > domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0, Looks about right, though there might be dragons. MSI is not always as sane as it should be... > > We could also consider extending this to support interrupt > > hierarchies, as __setup_irq() seems only concerned with the top of the > > stack (an IRQ provided by a generic MSI stack and backed by an irqchip > > providing IRQCHIP_ONESHOT_SAFE would go unnoticed). > > > > Would it be sufficient if one irq_chip in the hierarchy is oneshot-safe? > Or what do we have to check for? I think the top most chip is the key, the rest of the hierarchy is irrelevant because the top most chip is the one which is responsible for not creating an interrupt storm after the interrupt got acknowledged. Thanks, tglx
On 03.08.2018 16:09, Thomas Gleixner wrote: > On Wed, 1 Aug 2018, Heiner Kallweit wrote: >> On 31.07.2018 08:15, Marc Zyngier wrote: >>> On Mon, 30 Jul 2018 23:36:57 +0100, >>> Thomas Gleixner <tglx@linutronix.de> wrote: >>>> >>>> Yes, request for pure threaded interrupts are rejected if the oneshot flag >>>> is not set. The reason is that this would be deadly especially with level >>>> triggered interrupts because the primary default handler just wakes the >>>> thread and then reenables interrupts, which will make the interrupt come >>>> back immediately and the thread won't have a chance to actually shut it up >>>> in the device. >>>> >>>> That made me look into that code again and I found that we added a flag for >>>> irq chips to tell the core that the interrupt is one shot safe, i.e. that >>>> it can be requested w/o IRQF_ONESHOT. That was initially added to optimize >>>> MSI based interrupts which are oneshot safe by implementation. >>>> >>>> dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe") >>>> >>>> The original patch added that flag to the x86 MSI irqchip code, but that >>>> part was not applied for reasons which slipped from memory. It might be >>>> worthwhile to revisit that in order to avoid the mask/unmask overhead for >>>> such cases. >>> >>> Yup, that would actually be beneficial to a range of interrupt >>> controllers (only an obscure GPIO driver makes use of this flag). >>> >> >> I'm not an expert on that area, but if MSI is oneshot-safe in general, >> then we could do the following in the framework instead of touching >> every MSI irq_chip. Opinions? >> I tested on X86 and at least my system is still running. >> >> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c >> index 4ca2fd46..ba6da943 100644 >> --- a/kernel/irq/msi.c >> +++ b/kernel/irq/msi.c >> @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode, >> if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) >> msi_domain_update_chip_ops(info); >> >> + /* MSI is oneshot-safe in general */ >> + info->chip->flags |= IRQCHIP_ONESHOT_SAFE; >> + >> domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0, > > Looks about right, though there might be dragons. MSI is not always as sane > as it should be... > When saying "MSI isn't always sane", are you referring to the hardware or the controller driver implementation? Basically for me the question is whether we would be able to fix the issue if we meet such a dragon, or whether we would have to revert the change. Do you think the chance of a dragon is low enough? Or better add the flag only to the X86 PCI MSI irqchip for now? >>> We could also consider extending this to support interrupt >>> hierarchies, as __setup_irq() seems only concerned with the top of the >>> stack (an IRQ provided by a generic MSI stack and backed by an irqchip >>> providing IRQCHIP_ONESHOT_SAFE would go unnoticed). >>> >> >> Would it be sufficient if one irq_chip in the hierarchy is oneshot-safe? >> Or what do we have to check for? > > I think the top most chip is the key, the rest of the hierarchy is > irrelevant because the top most chip is the one which is responsible for > not creating an interrupt storm after the interrupt got acknowledged. > Thanks for the explanation, then I'll submit a patch implementing this logic. > Thanks, > > tglx > Heiner
On Fri, 3 Aug 2018, Heiner Kallweit wrote: > On 03.08.2018 16:09, Thomas Gleixner wrote: > > On Wed, 1 Aug 2018, Heiner Kallweit wrote: > >> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c > >> index 4ca2fd46..ba6da943 100644 > >> --- a/kernel/irq/msi.c > >> +++ b/kernel/irq/msi.c > >> @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode, > >> if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) > >> msi_domain_update_chip_ops(info); > >> > >> + /* MSI is oneshot-safe in general */ > >> + info->chip->flags |= IRQCHIP_ONESHOT_SAFE; > >> + > >> domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0, > > > > Looks about right, though there might be dragons. MSI is not always as sane > > as it should be... > > > When saying "MSI isn't always sane", are you referring to the hardware or > the controller driver implementation? Basically for me the question is > whether we would be able to fix the issue if we meet such a dragon, > or whether we would have to revert the change. It's hardware unfortunately, so it might be a revert. PCI-MSI should be safe, but the wild MSI variants in SoCs might be the actual dragon caves. > Do you think the chance of a dragon is low enough? Or better add the > flag only to the X86 PCI MSI irqchip for now? I think PCI-MSI in general would be not too risky. Famous last words. Thanks, tglx
On 03.08.2018 21:40, Thomas Gleixner wrote: > On Fri, 3 Aug 2018, Heiner Kallweit wrote: >> On 03.08.2018 16:09, Thomas Gleixner wrote: >>> On Wed, 1 Aug 2018, Heiner Kallweit wrote: >>>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c >>>> index 4ca2fd46..ba6da943 100644 >>>> --- a/kernel/irq/msi.c >>>> +++ b/kernel/irq/msi.c >>>> @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode, >>>> if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) >>>> msi_domain_update_chip_ops(info); >>>> >>>> + /* MSI is oneshot-safe in general */ >>>> + info->chip->flags |= IRQCHIP_ONESHOT_SAFE; >>>> + >>>> domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0, >>> >>> Looks about right, though there might be dragons. MSI is not always as sane >>> as it should be... >>> >> When saying "MSI isn't always sane", are you referring to the hardware or >> the controller driver implementation? Basically for me the question is >> whether we would be able to fix the issue if we meet such a dragon, >> or whether we would have to revert the change. > > It's hardware unfortunately, so it might be a revert. PCI-MSI should be > safe, but the wild MSI variants in SoCs might be the actual dragon caves. > pci_msi_create_irq_domain() is used by controller drivers for PCI(e) controllers in SoCs like Mediatek, Armada, .. I assume you're referring to some other (non-PCI) SoC-internal MSI mechanism when saying "wild variants", right? >> Do you think the chance of a dragon is low enough? Or better add the >> flag only to the X86 PCI MSI irqchip for now? > > I think PCI-MSI in general would be not too risky. Famous last words. > > Thanks, > > tglx >
diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c index 2a808e10..a1de501a 100644 --- a/drivers/pci/irq.c +++ b/drivers/pci/irq.c @@ -86,13 +86,17 @@ int pci_request_irq(struct pci_dev *dev, unsigned int nr, irq_handler_t handler, va_list ap; int ret; char *devname; + unsigned long irqflags = IRQF_SHARED; + + if (!handler) + irqflags |= IRQF_ONESHOT; va_start(ap, fmt); devname = kvasprintf(GFP_KERNEL, fmt, ap); va_end(ap); ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn, - IRQF_SHARED, devname, dev_id); + irqflags, devname, dev_id); if (ret) kfree(devname); return ret;
If we have a threaded interrupt with the handler being NULL, then request_threaded_irq() -> __setup_irq() will complain and bail out if the IRQF_ONESHOT flag isn't set. Therefore check for the handler being NULL and set IRQF_ONESHOT in this case. This change is needed to migrate the mei_me driver to pci_alloc_irq_vectors() and pci_request_irq(). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/pci/irq.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)