| Message ID | 20090616010101.GA11412@hash.localnet (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Bob Copeland wrote: > On Mon, Jun 15, 2009 at 09:58:30PM +0200, Rafael J. Wysocki wrote: > >> On Monday 15 June 2009, Alan Stern wrote: >> >>> I'm not sure whether the example is trustworthy. There was some >>> discussion quite a while ago regarding whether drivers should free >>> their IRQs during suspend, but I don't remember what the outcome was. >>> >> No, they shouldn't. >> >> That's why we do the entire suspend_device_irqs() thing etc. >> > > So, ath5k needs something like the following? > > diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c > index 55f7de0..0107cd6 100644 > --- a/drivers/net/wireless/ath/ath5k/base.c > +++ b/drivers/net/wireless/ath/ath5k/base.c > @@ -665,7 +665,6 @@ ath5k_pci_suspend(struct pci_dev *pdev, pm_message_t state) > > ath5k_led_off(sc); > > - free_irq(pdev->irq, sc); > pci_save_state(pdev); > pci_disable_device(pdev); > pci_set_power_state(pdev, PCI_D3hot); > @@ -686,18 +685,8 @@ ath5k_pci_resume(struct pci_dev *pdev) > if (err) > return err; > > - err = request_irq(pdev->irq, ath5k_intr, IRQF_SHARED, "ath", sc); > - if (err) { > - ATH5K_ERR(sc, "request_irq failed\n"); > - goto err_no_irq; > - } > - > ath5k_led_enable(sc); > return 0; > - > -err_no_irq: > - pci_disable_device(pdev); > - return err; > } > #endif /* CONFIG_PM */ > > Unfortunately this makes it worse. My eeepc-laptop hacks are now in wireless-testing. If I apply the above patch to wireless-testing and "remove" the device while suspended, I get a soft hang on resume. Suspending without removal works fine. I can see a BUG if I boot with no_console_suspend Unable to handle kernel NULL pointer dereference IP: klist_put Tainted: G W Process s2disk Call trace: ? klist_del ? device_del ? device_unregister ? pci_stop_dev ? pci_stop_bus ? pci_remove_device ? eeepc_rfkill_hotplug [eeepc_laptop] ? eeepc_hotk_resume [eeepc_laptop] ? acpi_device_resume ? device_resume ? hibernation_snapshot ? release_console_sem ? snapshot_ioctl ? snapshot_ioctl ? vfs_ioctl ? do_vfs_ioctl ? n_tty_write ? vfs_write ? sys_ioctl ? syscall_call -- 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 Thu, 18 Jun 2009, Alan Jenkins wrote: > Unfortunately this makes it worse. My eeepc-laptop hacks are now in > wireless-testing. If I apply the above patch to wireless-testing and > "remove" the device while suspended, I get a soft hang on resume. Is this different from the behavior without the patch? (I don't see how it could be.) > Suspending without removal works fine. > > I can see a BUG if I boot with no_console_suspend > > Unable to handle kernel NULL pointer dereference > IP: klist_put > Tainted: G W > Process s2disk > > Call trace: > ? klist_del > ? device_del > ? device_unregister > ? pci_stop_dev > ? pci_stop_bus > ? pci_remove_device > ? eeepc_rfkill_hotplug [eeepc_laptop] > ? eeepc_hotk_resume [eeepc_laptop] > ? acpi_device_resume > ? device_resume > ? hibernation_snapshot This should be doing more or less the same thing as if you removed the device while the system was running. Or is it not hot-unpluggable? Alan Stern -- 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
Alan Stern wrote: > On Thu, 18 Jun 2009, Alan Jenkins wrote: > > >> Unfortunately this makes it worse. My eeepc-laptop hacks are now in >> wireless-testing. If I apply the above patch to wireless-testing and >> "remove" the device while suspended, I get a soft hang on resume. >> > > Is this different from the behavior without the patch? (I don't see > how it could be.) > Ow. No, the free_irq patch doesn't cause this bug. I missed testing this case after adding a workaround for an even more obscure issue :-(. I have a good idea how eeepc-laptop could cause a double-free error. I suspect we originally got away with trying to call pci_remove_device() twice, but it's not actually legal, and my workaround caused two attempts to happen much closer together. I'll fix it tomorrow and re-test. With _both_ of my nasty scenarios this time (and the normal ones) - I should really make a checklist :-). >> Suspending without removal works fine. >> >> I can see a BUG if I boot with no_console_suspend >> >> Unable to handle kernel NULL pointer dereference >> IP: klist_put >> Tainted: G W >> Process s2disk >> >> Call trace: >> ? klist_del >> ? device_del >> ? device_unregister >> ? pci_stop_dev >> ? pci_stop_bus >> ? pci_remove_device >> ? eeepc_rfkill_hotplug [eeepc_laptop] >> ? eeepc_hotk_resume [eeepc_laptop] >> ? acpi_device_resume >> ? device_resume >> ? hibernation_snapshot >> > > This should be doing more or less the same thing as if you removed the > device while the system was running. Or is it not hot-unpluggable? > > Alan Stern > Outside of suspend, I can hot-unplug the device alright. I'm blaming my hotplug driver resume handler. Thanks Alan -- 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
Alan Jenkins wrote: > Alan Stern wrote: > >> On Thu, 18 Jun 2009, Alan Jenkins wrote: >> >> >> >>> Unfortunately this makes it worse. My eeepc-laptop hacks are now in >>> wireless-testing. If I apply the above patch to wireless-testing and >>> "remove" the device while suspended, I get a soft hang on resume. >>> >>> >> Is this different from the behavior without the patch? (I don't see >> how it could be.) >> >> > > Ow. No, the free_irq patch doesn't cause this bug. I missed testing > this case after adding a workaround for an even more obscure issue :-(. > I tried an earlier version without the problem workaround. Bob's suggested patch to remove the free_irq() on suspend works fine. It fixes the WARNING when the device is removed during suspend, and it still works if the device is not removed during suspend. Alan -- 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 Fri, Jun 19, 2009 at 09:34:20AM +0100, Alan Jenkins wrote: > I tried an earlier version without the problem workaround. Bob's > suggested patch to remove the free_irq() on suspend works fine. It > fixes the WARNING when the device is removed during suspend, and it > still works if the device is not removed during suspend. Okay thanks for finding it and testing. I'll send a version with my s-o-b later today.
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c index 55f7de0..0107cd6 100644 --- a/drivers/net/wireless/ath/ath5k/base.c +++ b/drivers/net/wireless/ath/ath5k/base.c @@ -665,7 +665,6 @@ ath5k_pci_suspend(struct pci_dev *pdev, pm_message_t state) ath5k_led_off(sc); - free_irq(pdev->irq, sc); pci_save_state(pdev); pci_disable_device(pdev); pci_set_power_state(pdev, PCI_D3hot); @@ -686,18 +685,8 @@ ath5k_pci_resume(struct pci_dev *pdev) if (err) return err; - err = request_irq(pdev->irq, ath5k_intr, IRQF_SHARED, "ath", sc); - if (err) { - ATH5K_ERR(sc, "request_irq failed\n"); - goto err_no_irq; - } - ath5k_led_enable(sc); return 0; - -err_no_irq: - pci_disable_device(pdev); - return err; } #endif /* CONFIG_PM */