search for: pci_device_shutdown

...ci, the device may still try to send interupts, > > which will be on IRQ lines seeing MSI-X disabled. Those interrupts will > > be unhandled, and may cause flood. Here is the problem I want to solve - file system driver hang: If a fs code happen to hit __wait_on_buffer right after pci pci_device_shutdown disabled msix, it will never make progress because the requests it waits for will never be completed. So the system hangs. In other words we will want to reset virtio device before pci_device_shutdown AND wake up all waiters. Unfortunately, neither your patch nor mine does that, because virtio bu...

...ci, the device may still try to send interupts, > > which will be on IRQ lines seeing MSI-X disabled. Those interrupts will > > be unhandled, and may cause flood. Here is the problem I want to solve - file system driver hang: If a fs code happen to hit __wait_on_buffer right after pci pci_device_shutdown disabled msix, it will never make progress because the requests it waits for will never be completed. So the system hangs. In other words we will want to reset virtio device before pci_device_shutdown AND wake up all waiters. Unfortunately, neither your patch nor mine does that, because virtio bu...

...<c0290238>] (__free_irq+0x2c4/0x2c8) [ 31.139355] [<c0290238>] (__free_irq) from [<c02902d0>] (free_irq+0x44/0x78) [ 31.142909] [<c02902d0>] (free_irq) from [<c059d3a8>] (vp_del_vqs+0x68/0x1c0) [ 31.146299] [<c059d3a8>] (vp_del_vqs) from [<c056ca4c>] (pci_device_shutdown+0x3c/0x78) The obvious fix is to drop the affinity hint before freeing the interrupt. Signed-off-by: Marc Zyngier <marc.zyngier at arm.com> --- drivers/virtio/virtio_pci_common.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/virtio/virtio_pci_common.c...

...<c0290238>] (__free_irq+0x2c4/0x2c8) [ 31.139355] [<c0290238>] (__free_irq) from [<c02902d0>] (free_irq+0x44/0x78) [ 31.142909] [<c02902d0>] (free_irq) from [<c059d3a8>] (vp_del_vqs+0x68/0x1c0) [ 31.146299] [<c059d3a8>] (vp_del_vqs) from [<c056ca4c>] (pci_device_shutdown+0x3c/0x78) The obvious fix is to drop the affinity hint before freeing the interrupt. Signed-off-by: Marc Zyngier <marc.zyngier at arm.com> --- drivers/virtio/virtio_pci_common.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/virtio/virtio_pci_common.c...

...t issue there though. Sorry Pierre, I missed this question and am seeing it only now on saving the message away: On shutdown the driver isn't unloaded, rather the driver core walks the device hierarchy bottom-up in device_shutdown() and, for PCI- enumerated GPUs, invokes the bus shutdown hook pci_device_shutdown(). This in turn would normally invoke ->shutdown in nouveau_drm_pci_driver, but we haven't defined it. So the only thing that happens is that pci_device_shutdown() runtime resumes the device and optionally turns off busmastering if a kexec kernel is going to be loaded. So that's mostly...

...o send interupts, > > > which will be on IRQ lines seeing MSI-X disabled. Those interrupts will > > > be unhandled, and may cause flood. > > Here is the problem I want to solve - file system driver hang: > > If a fs code happen to hit __wait_on_buffer right after pci pci_device_shutdown > disabled msix, it will never make progress because the requests it waits for > will never be completed. So the system hangs. Paolo says that pci reset of virtio scsi device guarantees that all outstanding requests complete. If true and implemented correctly, I don't see what else need...

...o send interupts, > > > which will be on IRQ lines seeing MSI-X disabled. Those interrupts will > > > be unhandled, and may cause flood. > > Here is the problem I want to solve - file system driver hang: > > If a fs code happen to hit __wait_on_buffer right after pci pci_device_shutdown > disabled msix, it will never make progress because the requests it waits for > will never be completed. So the system hangs. Paolo says that pci reset of virtio scsi device guarantees that all outstanding requests complete. If true and implemented correctly, I don't see what else need...

...gt; > > > and the INTx lines are shared with other devices. > > > > > > Do we care? Is this a performance path? I thought we were in a kexec > > > shutdown path. > > > > Yes but the handler would always have to be registered, right? > > The pci_device_shutdown() path you're modifying calls drv->shutdown() > immediately before disabling MSI, so I suppose you could register a > handler in a virtio shutdown method. I guess we could. Not sure what we'd do e.g. if that fails. > > > > Seems silly to slow them down just so we can...

...gt; > > > and the INTx lines are shared with other devices. > > > > > > Do we care? Is this a performance path? I thought we were in a kexec > > > shutdown path. > > > > Yes but the handler would always have to be registered, right? > > The pci_device_shutdown() path you're modifying calls drv->shutdown() > immediately before disabling MSI, so I suppose you could register a > handler in a virtio shutdown method. I guess we could. Not sure what we'd do e.g. if that fails. > > > > Seems silly to slow them down just so we can...

...ci-driver.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 3cb2210..fb29c96 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev) > > pm_runtime_resume(dev); > > - if (drv && drv->shutdown) > + if (drv && drv->shutdown) { > drv->shutdown(pci_dev); > - pci_msi_shutdown(pci_dev); > - pci_msix_shutdown(pci_dev); > + pci_msi_shutdown(pci_dev); >...

...ci-driver.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 3cb2210..fb29c96 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev) > > pm_runtime_resume(dev); > > - if (drv && drv->shutdown) > + if (drv && drv->shutdown) { > drv->shutdown(pci_dev); > - pci_msi_shutdown(pci_dev); > - pci_msix_shutdown(pci_dev); > + pci_msi_shutdown(pci_dev); >...

...t; > which will be on IRQ lines seeing MSI-X disabled. Those interrupts will > > > > be unhandled, and may cause flood. > > > > Here is the problem I want to solve - file system driver hang: > > > > If a fs code happen to hit __wait_on_buffer right after pci pci_device_shutdown > > disabled msix, it will never make progress because the requests it waits for > > will never be completed. So the system hangs. > > Paolo says that pci reset of virtio scsi device guarantees > that all outstanding requests complete. > > If true and implemented correc...

...t; > which will be on IRQ lines seeing MSI-X disabled. Those interrupts will > > > > be unhandled, and may cause flood. > > > > Here is the problem I want to solve - file system driver hang: > > > > If a fs code happen to hit __wait_on_buffer right after pci pci_device_shutdown > > disabled msix, it will never make progress because the requests it waits for > > will never be completed. So the system hangs. > > Paolo says that pci reset of virtio scsi device guarantees > that all outstanding requests complete. > > If true and implemented correc...

Currently shutdown is nop for virtio devices, but the core code could remove things behind us such as MSI-X handler etc. For example in the case of virtio-scsi-pci, the device may still try to send interupts, which will be on IRQ lines seeing MSI-X disabled. Those interrupts will be unhandled, and may cause flood. Remove the device in "shutdown" callback to allow device drivers clean up

Currently shutdown is nop for virtio devices, but the core code could remove things behind us such as MSI-X handler etc. For example in the case of virtio-scsi-pci, the device may still try to send interupts, which will be on IRQ lines seeing MSI-X disabled. Those interrupts will be unhandled, and may cause flood. Remove the device in "shutdown" callback to allow device drivers clean up

Unbinding nouveau on a dual GPU MacBook Pro oopses because we iterate over the bl_connectors list in nouveau_backlight_exit() but skipped initializing it in nouveau_backlight_init(). Stacktrace for posterity: BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 IP: nouveau_backlight_exit+0x2b/0x70 [nouveau] nouveau_display_destroy+0x29/0x80 [nouveau]

...I_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8), }; enum pci_irq_reroute_variant { diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 3cb2210..59d9e40 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev) if (drv && drv->shutdown) drv->shutdown(pci_dev); - pci_msi_shutdown(pci_dev); - pci_msix_shutdown(pci_dev); + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) { + pci_msi_shutdown(pci_dev); + pci_msix_shutdown(pci_dev); + } #ifdef CONF...

...I_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8), }; enum pci_irq_reroute_variant { diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 3cb2210..59d9e40 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev) if (drv && drv->shutdown) drv->shutdown(pci_dev); - pci_msi_shutdown(pci_dev); - pci_msix_shutdown(pci_dev); + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) { + pci_msi_shutdown(pci_dev); + pci_msix_shutdown(pci_dev); + } #ifdef CONF...

.... If we do that, maybe there's > some kind of race between that resume and the reboot? I think we do in fact resume PCI devices before shutdown. Here's the path I'm looking at: device_shutdown pm_runtime_get_noresume pm_runtime_barrier dev->bus->shutdown pci_device_shutdown pm_runtime_resume __pm_runtime_resume(dev, 0) rpm_resume(dev, 0) __update_runtime_status(dev, RPM_RESUMING) callback = RPM_GET_CALLBACK(dev, runtime_resume) rpm_callback(callback, dev) __rpm_callback...

...>> }; >> >> enum pci_irq_reroute_variant { >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c >> index 3cb2210..59d9e40 100644 >> --- a/drivers/pci/pci-driver.c >> +++ b/drivers/pci/pci-driver.c >> @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev) >> >> if (drv && drv->shutdown) >> drv->shutdown(pci_dev); >> - pci_msi_shutdown(pci_dev); >> - pci_msix_shutdown(pci_dev); >> + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) { >> + pci_msi_shutd...