| Message ID | 20190319011214.7847-1-mr.nuke.me@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PCI/LINK: bw_notification: Do not leave interrupt handler NULL | expand |
On Mon, Mar 18, 2019 at 08:12:04PM -0500, Alexandru Gagniuc wrote: > A threaded IRQ with a NULL handler does not work with level-triggered > interrupts. request_threaded_irq() will return an error: > > genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 16 > pcie_bw_notification: probe of 0000:00:1b.0:pcie010 failed with error -22 > > For level interrupts we need to silence the interrupt before exiting > the IRQ handler, so just clear the PCI_EXP_LNKSTA_LBMS bit there. > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> I've tested this on my Ivy Bridge MacBook Pro which uses INTx on root and downstream ports and I'm not seeing probe errors, so: Tested-by: Lukas Wunner <lukas@wunner.de> > @@ -67,8 +77,8 @@ static irqreturn_t pcie_bw_notification_handler(int irq, void *context) > __pcie_print_link_status(dev, false); > up_read(&pci_bus_sem); > > + pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status); > pcie_update_link_speed(port->subordinate, link_status); > - pcie_capability_write_word(port, PCI_EXP_LNKSTA, events); > return IRQ_HANDLED; > } I'd suggest leaving the call to pcie_update_link_speed() in pcie_bw_notification_irq() to avoid the duplicate read of the Link Status register. With that addressed, Reviewed-by: Lukas Wunner <lukas@wunner.de> Thanks, Lukas
On Mon, Mar 18, 2019 at 08:12:04PM -0500, Alexandru Gagniuc wrote: > I was able to test this on edge-triggered interrupts. None of my > machines have PCIe ports that use level-triggered interrupts. This > might not be too straightforward to test without a hardware yanker, > but if there's a way to force a specific interrupt to be level > triggered, I could do the testing on my end. Spec says INTx emulation is required, so I think it ought to be possible to force them to level-triggered if you disable MSI. The kernel parameter 'pci=nomsi' can force everything to INTx.
Hi Alexandru, On Mon, Mar 18, 2019 at 08:12:04PM -0500, Alexandru Gagniuc wrote: > A threaded IRQ with a NULL handler does not work with level-triggered > interrupts. request_threaded_irq() will return an error: > > genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 16 > pcie_bw_notification: probe of 0000:00:1b.0:pcie010 failed with error -22 > > For level interrupts we need to silence the interrupt before exiting > the IRQ handler, so just clear the PCI_EXP_LNKSTA_LBMS bit there. > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> What's your thought regarding Lukas' comment? If you do repost this, please add a Fixes: tag to help connect this with the initial commit. If not, I can add the tag myself. > --- > > OOPS! I'm sorry for the noise. Here's the fix. > > I was able to test this on edge-triggered interrupts. None of my > machines have PCIe ports that use level-triggered interrupts. This > might not be too straightforward to test without a hardware yanker, > but if there's a way to force a specific interrupt to be level > triggered, I could do the testing on my end. > > drivers/pci/pcie/bw_notification.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pcie/bw_notification.c b/drivers/pci/pcie/bw_notification.c > index d2eae3b7cc0f..001d6253ad48 100644 > --- a/drivers/pci/pcie/bw_notification.c > +++ b/drivers/pci/pcie/bw_notification.c > @@ -44,11 +44,10 @@ static void pcie_disable_link_bandwidth_notification(struct pci_dev *dev) > pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnk_ctl); > } > > -static irqreturn_t pcie_bw_notification_handler(int irq, void *context) > +static irqreturn_t pcie_bw_notification_irq(int irq, void *context) > { > struct pcie_device *srv = context; > struct pci_dev *port = srv->port; > - struct pci_dev *dev; > u16 link_status, events; > int ret; > > @@ -58,6 +57,17 @@ static irqreturn_t pcie_bw_notification_handler(int irq, void *context) > if (ret != PCIBIOS_SUCCESSFUL || !events) > return IRQ_NONE; > > + pcie_capability_write_word(port, PCI_EXP_LNKSTA, events); > + return IRQ_WAKE_THREAD; > +} > + > +static irqreturn_t pcie_bw_notification_handler(int irq, void *context) > +{ > + struct pcie_device *srv = context; > + struct pci_dev *port = srv->port; > + struct pci_dev *dev; > + u16 link_status; > + > /* > * Print status from downstream devices, not this root port or > * downstream switch port. > @@ -67,8 +77,8 @@ static irqreturn_t pcie_bw_notification_handler(int irq, void *context) > __pcie_print_link_status(dev, false); > up_read(&pci_bus_sem); > > + pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status); > pcie_update_link_speed(port->subordinate, link_status); > - pcie_capability_write_word(port, PCI_EXP_LNKSTA, events); > return IRQ_HANDLED; > } > > @@ -80,7 +90,8 @@ static int pcie_bandwidth_notification_probe(struct pcie_device *srv) > if (!pcie_link_bandwidth_notification_supported(srv->port)) > return -ENODEV; > > - ret = request_threaded_irq(srv->irq, NULL, pcie_bw_notification_handler, > + ret = request_threaded_irq(srv->irq, pcie_bw_notification_irq, > + pcie_bw_notification_handler, > IRQF_SHARED, "PCIe BW notif", srv); > if (ret) > return ret; > -- > 2.19.2 >
On 3/20/19 8:46 AM, Bjorn Helgaas wrote: > Hi Alexandru, > > On Mon, Mar 18, 2019 at 08:12:04PM -0500, Alexandru Gagniuc wrote: >> A threaded IRQ with a NULL handler does not work with level-triggered >> interrupts. request_threaded_irq() will return an error: >> >> genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 16 >> pcie_bw_notification: probe of 0000:00:1b.0:pcie010 failed with error -22 >> >> For level interrupts we need to silence the interrupt before exiting >> the IRQ handler, so just clear the PCI_EXP_LNKSTA_LBMS bit there. >> >> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > > What's your thought regarding Lukas' comment? If you do repost this, > please add a Fixes: tag to help connect this with the initial commit. I like Lukas's idea. I should have this re-posted by end of week, unless there's an urgency to get it out earlier. Alex
On Wed, Mar 20, 2019 at 08:48:33AM -0500, Alex G. wrote: > On 3/20/19 8:46 AM, Bjorn Helgaas wrote: > > On Mon, Mar 18, 2019 at 08:12:04PM -0500, Alexandru Gagniuc wrote: > > > A threaded IRQ with a NULL handler does not work with level-triggered > > > interrupts. request_threaded_irq() will return an error: > > > > > > genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 16 > > > pcie_bw_notification: probe of 0000:00:1b.0:pcie010 failed with error -22 > > > > > > For level interrupts we need to silence the interrupt before exiting > > > the IRQ handler, so just clear the PCI_EXP_LNKSTA_LBMS bit there. > > > > > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > > > > What's your thought regarding Lukas' comment? If you do repost this, > > please add a Fixes: tag to help connect this with the initial commit. > > I like Lukas's idea. I should have this re-posted by end of week, unless > there's an urgency to get it out earlier. It would have been ideal to get the fix in -rc2, but I guess the end of the week is OK because it's probably already too late for me to apply it, run it through 0-day, get it in -next, and ask Linus to pull it for -rc2. Bjorn
diff --git a/drivers/pci/pcie/bw_notification.c b/drivers/pci/pcie/bw_notification.c index d2eae3b7cc0f..001d6253ad48 100644 --- a/drivers/pci/pcie/bw_notification.c +++ b/drivers/pci/pcie/bw_notification.c @@ -44,11 +44,10 @@ static void pcie_disable_link_bandwidth_notification(struct pci_dev *dev) pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnk_ctl); } -static irqreturn_t pcie_bw_notification_handler(int irq, void *context) +static irqreturn_t pcie_bw_notification_irq(int irq, void *context) { struct pcie_device *srv = context; struct pci_dev *port = srv->port; - struct pci_dev *dev; u16 link_status, events; int ret; @@ -58,6 +57,17 @@ static irqreturn_t pcie_bw_notification_handler(int irq, void *context) if (ret != PCIBIOS_SUCCESSFUL || !events) return IRQ_NONE; + pcie_capability_write_word(port, PCI_EXP_LNKSTA, events); + return IRQ_WAKE_THREAD; +} + +static irqreturn_t pcie_bw_notification_handler(int irq, void *context) +{ + struct pcie_device *srv = context; + struct pci_dev *port = srv->port; + struct pci_dev *dev; + u16 link_status; + /* * Print status from downstream devices, not this root port or * downstream switch port. @@ -67,8 +77,8 @@ static irqreturn_t pcie_bw_notification_handler(int irq, void *context) __pcie_print_link_status(dev, false); up_read(&pci_bus_sem); + pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status); pcie_update_link_speed(port->subordinate, link_status); - pcie_capability_write_word(port, PCI_EXP_LNKSTA, events); return IRQ_HANDLED; } @@ -80,7 +90,8 @@ static int pcie_bandwidth_notification_probe(struct pcie_device *srv) if (!pcie_link_bandwidth_notification_supported(srv->port)) return -ENODEV; - ret = request_threaded_irq(srv->irq, NULL, pcie_bw_notification_handler, + ret = request_threaded_irq(srv->irq, pcie_bw_notification_irq, + pcie_bw_notification_handler, IRQF_SHARED, "PCIe BW notif", srv); if (ret) return ret;
A threaded IRQ with a NULL handler does not work with level-triggered interrupts. request_threaded_irq() will return an error: genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 16 pcie_bw_notification: probe of 0000:00:1b.0:pcie010 failed with error -22 For level interrupts we need to silence the interrupt before exiting the IRQ handler, so just clear the PCI_EXP_LNKSTA_LBMS bit there. Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> --- OOPS! I'm sorry for the noise. Here's the fix. I was able to test this on edge-triggered interrupts. None of my machines have PCIe ports that use level-triggered interrupts. This might not be too straightforward to test without a hardware yanker, but if there's a way to force a specific interrupt to be level triggered, I could do the testing on my end. drivers/pci/pcie/bw_notification.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)