| Message ID | 4d34a0a70902031601r5c8c3424l4cd399193142e612@mail.gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | Felipe Balbi |
| Headers | show |
On Tuesday 03 February 2009, Kim Kyuwon wrote: > USB should be suspended with interrupt disabled[1]. That text means that the sysdev.suspend() method is called with IRQs disabled ... just like suspend_late() methods do (now) for other busses. (There is no longer any point to using a sysdev; platform_device can now do everything sysdev can do.) > If USB is suspended with > interrupt enabled and connected to host PC, a kernel panic would occur When > it wakes up. Because, after the arch_suspend_enable_irqs() function is called > in the suspend_enter() function, USB Interrupt handler is called, even though > USB controller is still not resumed! All devices are resumed after the > device_resume() is called. This change seems pretty wrong. The first thing I noticed is that it could prevent remote wakeup from working. Assuming you actulaly observed this oops, the bug is more likely to be that it didn't enter the right kind of suspend mode. There are at least two types of suspend that a USB host should be prepared to handle: - OFF ... the lowest power mode, everything connected to the host gets logically disconnected. Wakeup involves complete re-enumeration. - SUSPEND ... with two variants, where remote wakeup is (a) enabled or (b) disabled, but the USB link enters the suspend state: VBUS supplied, with low current draw, and no SOFs get sent. In the wakeup-enabled case, an IRQ is often used as the wakeup trigger. I'm not entirely sure this driver handles suspend right.. Now, the actual details of how the USB controller, its transceiver, and other related hardware is configured... can vary a lot. Are you sure you haven't broken remote wakeup by doing this? - Dave > [1] /Documentation/power/devices.txt: 412 line > > Signed-off-by: Kim Kyuwon <chammoru@gmail.com> > --- > drivers/usb/musb/musb_core.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > index 2cc34fa..0dfe15e 100644 > --- a/drivers/usb/musb/musb_core.c > +++ b/drivers/usb/musb/musb_core.c > @@ -2151,6 +2151,8 @@ static int musb_suspend(struct platform_device > *pdev, pm_message_t message) > > spin_lock_irqsave(&musb->lock, flags); > > + disable_irq(musb->nIrq); > + > if (is_peripheral_active(musb)) { > /* FIXME force disconnect unless we know USB will wake > * the system up quickly enough to respond ... > @@ -2184,6 +2186,8 @@ static int musb_resume(struct platform_device *pdev) > else > clk_enable(musb->clock); > > + enable_irq(musb->nIrq); > + > /* for static cmos like DaVinci, register values were preserved > * unless for some reason the whole soc powered down and we're > * not treating that as a whole-system restart (e.g. swsusp) > -- > Kim Kyuwon > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Sat, Feb 21, 2009 at 11:30 AM, David Brownell <david-b@pacbell.net> wrote: > On Tuesday 03 February 2009, Kim Kyuwon wrote: >> USB should be suspended with interrupt disabled[1]. > > That text means that the sysdev.suspend() method is > called with IRQs disabled ... just like suspend_late() > methods do (now) for other busses. (There is no longer > any point to using a sysdev; platform_device can now do > everything sysdev can do.) > I thought why the '/Documentation/power/devices.txt' mentioned 'disabling IRQs'. Let's look at the call-stack which is shown when the processor resumes from 'Sleep' state. suspend_devices_and_enter() -- suspend_enter() -- -- arch_suspend_disable_irqs() -- -- suspend_ops->enter() // Waking up from here -- -- arch_suspend_enable_irqs() // <A> -- -- <Do someting..> -- device_resume() // <B> As I talked in the commit message, after invoking the arch_suspend_disable_irqs() function, some interrupt handlers would start to be invoked again. And most interrupt handlers access each peripheral controller register. However, each iclk, fclk and controller are disabled until the resume() functions are called in device_resume() function. Therefore, Not disabling IRQs can cause kernel panics. For instance, if an suspend function disable iclk, an IRQ handler that accesses controller register and invoked from <A> to <B> will make the data abort exception. Maybe all device driver doesn't need to disable IRQs while entering CPU sleep state but as you can see most devices that handle the peripheral controller should disable IRQs. >> If USB is suspended with >> interrupt enabled and connected to host PC, a kernel panic would occur When >> it wakes up. Because, after the arch_suspend_enable_irqs() function is called >> in the suspend_enter() function, USB Interrupt handler is called, even though >> USB controller is still not resumed! All devices are resumed after the >> device_resume() is called. > > This change seems pretty wrong. The first thing I noticed > is that it could prevent remote wakeup from working. > > Assuming you actually observed this oops, the bug is more > likely to be that it didn't enter the right kind of suspend > mode. There are at least two types of suspend that a USB > host should be prepared to handle: > > - OFF ... the lowest power mode, everything connected to > the host gets logically disconnected. Wakeup involves > complete re-enumeration. > > - SUSPEND ... with two variants, where remote wakeup is > (a) enabled or (b) disabled, but the USB link enters > the suspend state: VBUS supplied, with low current > draw, and no SOFs get sent. > > In the wakeup-enabled case, an IRQ is often used as the > wakeup trigger. > > I'm not entirely sure this driver handles suspend right.. > > Now, the actual details of how the USB controller, its > transceiver, and other related hardware is configured... > can vary a lot. > > Are you sure you haven't broken remote wakeup by doing > this? Of course, this patch prevents remote wakeup, because I applied this patch to the 'SUSPEND' function. Suspend function is invoked to disable the USB from working not to enable the remote wakeup. The musb_suspend() function is already disabling the USB clock. That means that if generic_interrupt() is invoked from <A> to <B>, it also causes oops, so USB IRQ should be disabled. I think the 'remote wakeup' function is another issue and another feature. the current suspend function of musb driver doesn't consider the 'remote wakeup' enough. It can be added after my patch is applied. But there is one thing absolutely true, "If the USB clock is disabled, IRQ should be disabled". This patch is just for the stable musb. Somewhat related: USB insertion and deletion can be configured as wake-source too. As you know it is different from USB interrupt. so I don't think waking up from the USB interrupt is still not critical :) > - Dave >
On Sunday 22 February 2009, Kim Kyuwon wrote: > Hi, > > On Sat, Feb 21, 2009 at 11:30 AM, David Brownell <david-b@pacbell.net> wrote: > > On Tuesday 03 February 2009, Kim Kyuwon wrote: > >> USB should be suspended with interrupt disabled[1]. > > > > That text means that the sysdev.suspend() method is > > called with IRQs disabled ... just like suspend_late() > > methods do (now) for other busses. (There is no longer > > any point to using a sysdev; platform_device can now do > > everything sysdev can do.) > > > > I thought why the '/Documentation/power/devices.txt' mentioned > 'disabling IRQs'. Let's look at the call-stack That can't matter, because the USB controller isn't implemented using the "sysdev" infrastructure. > which is shown when the > processor resumes from 'Sleep' state. > > suspend_devices_and_enter() > -- suspend_enter() > -- -- arch_suspend_disable_irqs() > -- -- suspend_ops->enter() // Waking up from here > -- -- arch_suspend_enable_irqs() // <A> > -- -- <Do someting..> > -- device_resume() // <B> It doesn't enable IRQs until after it's resumed sysdev nodes and done resume_early() for all other devices. That's before your <A>. > As I talked in the commit message, after invoking the > arch_suspend_disable_irqs() function, some interrupt handlers would > start to be invoked again. And most interrupt handlers access each > peripheral controller register. However, each iclk, fclk and > controller are disabled until the resume() functions are called in > device_resume() function. That blanket rule is wrong ... and creates confusion. First, remember that system-wide suspend isn't really the best model for most OMAP systems. I won't revisit those discussions here. Just remember that the goals include having: - normal system run states put everything that's not in active use in a low power state; - system idle states do the same, leveraging some extra mechanisms available when the CPU, and maybe DMA, are inactive; - wake events only reactivate a bare minimum of hardware. That is, the C1/C2/.../C6/... idle states, combined with device low power states, supplant global states like "suspend to RAM", which are exited by wasting power issuing resume() to *every* device. There's a whole spectrum of low power states. The poor starved trio of states that can be entered by writing to the /sys/power/state file is only a small subset, with a big drawback of needing a user choice/action. Second, suspend() doesn't always put everything into an OFF state. Even a system-wide "suspend to RAM" state is expected to support various wake events. That requires various hardware resources being left partially active in suspend(). Consider for example "echo standby > /sys/power/state". It's quite normal for a suspend() method running in that context leave resources active that wouldn't stay that way with "echo mem > /sys/power/state"; ditto with "echo disk > ...". > Therefore, Not disabling IRQs can cause kernel panics. For instance, > if an suspend function disable iclk, an IRQ handler that accesses > controller register and invoked from <A> to <B> will make the data > abort exception. If a suspend() leaves IRQs enabled and disables iclk, the IRQ handler obviously needs to be know to re-enable iclk. Better might be to disable the iclk in suspend_late() and enable it in resume_early(). When suspend() is told that its device should be able to wake the system, that may well mean leaving the fclk active and IRQ enabled. Details are device-specific ... I've not looked at that issue with respect to MUSB. > Maybe all device driver doesn't need to disable IRQs while entering > CPU sleep state but as you can see most devices that handle the > peripheral controller should disable IRQs. CPU sleep state != device sleep state; as I've already commented, various device low power states need to be able to wake the system. Plus of course, "system sleep state" != "CPU sleep state". And when a driver needs to keep some infrastructure active to serve as a wake event source, that tends to cascade. It may not be possible to enter the deepest sleep states with certain devices being wake-active, since they may require certain clocks to stay enabled. > > Are you sure you haven't broken remote wakeup by doing > > this? > > Of course, this patch prevents remote wakeup, So this patch is clearly broken. NAK. > because I applied this > patch to the 'SUSPEND' function. Suspend function is invoked to > disable the USB from working not to enable the remote wakeup. Wrong. To repeat myself: there are two variants of suspend, with remote wakeup (a) enabled or else (b) disbled. Plus there's a pseudo-suspend, with VBUS power disabled so that all USB device sessions are broken ... that psuedo-suspend will normally be mapped to an OFF state. Compare to PCs. USB remote wakeup routinely works from the "standby" state; mostly works from "suspend-to-RAM", except on laptops which happen to prioritize battery power over functionality; and sometimes works from "hibernation". You are asserting it should never work in any of those cases ... which is obviously wrong, which means there's something wrong with your logic. > The > musb_suspend() function is already disabling the USB clock. That means > that if generic_interrupt() is invoked from <A> to <B>, it also causes > oops, so USB IRQ should be disabled. See above. In other systems, the answer is trivial: the early_resume() method can enable the interface clock. Wouldn't that work here? > I think the 'remote wakeup' function is another issue and another > feature. the current suspend function of musb driver doesn't consider > the 'remote wakeup' enough. That's probably true. Power management in general for this driver has mostly been ignored. But that's no excuse for doing something that's clearly wrong. > It can be added after my patch is applied. > But there is one thing absolutely true, "If the USB clock is disabled, > IRQ should be disabled". This patch is just for the stable musb. Try coming up with a better patch though. The simple case may be just disabling the interface clock, and having completely functional remote wakeup. Trickier cases will involve disabling more clocks, or even entering OFF mode (and then completely re-initializing the hardware). > Somewhat related: USB insertion and deletion can be configured as > wake-source too. As you know it is different from USB interrupt. so I > don't think waking up from the USB interrupt is still not critical :) Acatually, those events *DO* map to interrupts from the MUSB state machine. - Dave > > - Dave > > > > -- > Kyuwon > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c index 2cc34fa..0dfe15e 100644 --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -2151,6 +2151,8 @@ static int musb_suspend(struct platform_device *pdev, pm_message_t message) spin_lock_irqsave(&musb->lock, flags); + disable_irq(musb->nIrq); + if (is_peripheral_active(musb)) { /* FIXME force disconnect unless we know USB will wake
USB should be suspended with interrupt disabled[1]. If USB is suspended with interrupt enabled and connected to host PC, a kernel panic would occur When it wakes up. Because, after the arch_suspend_enable_irqs() function is called in the suspend_enter() function, USB Interrupt handler is called, even though USB controller is still not resumed! All devices are resumed after the device_resume() is called. [1] /Documentation/power/devices.txt: 412 line Signed-off-by: Kim Kyuwon <chammoru@gmail.com> --- drivers/usb/musb/musb_core.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) * the system up quickly enough to respond ... @@ -2184,6 +2186,8 @@ static int musb_resume(struct platform_device *pdev) else clk_enable(musb->clock); + enable_irq(musb->nIrq); + /* for static cmos like DaVinci, register values were preserved * unless for some reason the whole soc powered down and we're * not treating that as a whole-system restart (e.g. swsusp) -- Kim Kyuwon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html