Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> ChangeSet 1.2215, 2004/12/21 16:25:03-08:00, greg@kroah.com
> 
> 	[PATCH] USB: fix Scheduling while atomic warning when resuming.
> 	
> 	This fixes a warning when resuming the USB EHCI host controller driver.

> diff -Nru a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> --- a/drivers/usb/host/ehci-hub.c	2004-12-21 17:04:09 -08:00
> +++ b/drivers/usb/host/ehci-hub.c	2004-12-21 17:04:09 -08:00
> @@ -122,7 +122,7 @@
>  		writel (temp, &ehci->regs->port_status [i]);
>  	}
>  	i = HCS_N_PORTS (ehci->hcs_params);
> -	msleep (20);
> +	mdelay (20);
>  	while (i--) {
>  		temp = readl (&ehci->regs->port_status [i]);


This is more than a little bit silly.

The entire resume function holds spin_lock_irq() for far longer than a 
timer tick.

If we are going for a minimalist -rc patch, why not drop the lock, 
sleep, then reacquire the lock?

This strikes me as a bad change, make in haste for -rc, that will get 
quickly forgotten (and left as-is) once 2.6.10 is released.

	Jeff

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[David Brownell]

On Tuesday 21 December 2004 7:48 pm, Jeff Garzik wrote:
> If we are going for a minimalist -rc patch, why not drop the lock, 
> sleep, then reacquire the lock?

If that lock were dropped, what would prevent other tasks from
touching the hardware while it's sending RESUME signaling down
the bus, and thereby mucking up the resume sequence?

That 20+ msec is a protocol timer.  Normally we'd want khubd
to handle such things, and then resume all the child devices,
but that's not so readily done for root hubs or during calls
from the pm core code.

- Dave

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[Jeff Garzik]

David Brownell wrote:
> On Tuesday 21 December 2004 7:48 pm, Jeff Garzik wrote:
> 
>>If we are going for a minimalist -rc patch, why not drop the lock, 
>>sleep, then reacquire the lock?
> 
> 
> If that lock were dropped, what would prevent other tasks from
> touching the hardware while it's sending RESUME signaling down
> the bus, and thereby mucking up the resume sequence?

Precisely what other tasks are active for this hardware, during resume?

	Jeff

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[David Brownell]

On Tuesday 21 December 2004 8:46 pm, Jeff Garzik wrote:
> > If that lock were dropped, what would prevent other tasks from
> > touching the hardware while it's sending RESUME signaling down
> > the bus, and thereby mucking up the resume sequence?
> 
> Precisely what other tasks are active for this hardware, during resume?

There's no guarantee that suspend() and resume() methods
are only called during system-wide suspend and resume.

And likewise, if there were no other tasks, why should
you even be concerned about spinning vs sleeping?

- Dave

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[Jeff Garzik]

David Brownell wrote:
> On Tuesday 21 December 2004 8:46 pm, Jeff Garzik wrote:
> 
>>>If that lock were dropped, what would prevent other tasks from
>>>touching the hardware while it's sending RESUME signaling down
>>>the bus, and thereby mucking up the resume sequence?
>>
>>Precisely what other tasks are active for this hardware, during resume?
> 
> 
> There's no guarantee that suspend() and resume() methods
> are only called during system-wide suspend and resume.

That is precisely the reason why I am concerned.  If it was only during 
system-wide resume, the impact of the very-long mdelay() would be more 
difficult to notice.

You also ignored my question :)

If the PCI layer is calling the resume method for a PCI device while 
simultaneously calling the suspend method, that's a PCI layer problem. 
Similarly, If the USB layer is calling into your driver while you are 
resuming, something is broken and it ain't your locking.

	Jeff

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[Benjamin Herrenschmidt]

> If the PCI layer is calling the resume method for a PCI device while 
> simultaneously calling the suspend method, that's a PCI layer problem. 
> Similarly, If the USB layer is calling into your driver while you are 
> resuming, something is broken and it ain't your locking.

Actually, the later isn't broken, something may well call into the
higher level USB drivers while resuming, but indeed, those shouldn't
send any URB down the stack when the bus is suspended, and the EHCI
driver should drop incoming URBs as well until fully resumed.

I think the lock here is only needed to protect the HCD state
transitions David, no ? All we need is make sure that we don't let
things get queued (or call into EHCI code path that will end up
touch the HW) while suspended.

Ben.

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[David Brownell]

On Tuesday 21 December 2004 9:27 pm, Jeff Garzik wrote:
> > There's no guarantee that suspend() and resume() methods
> > are only called during system-wide suspend and resume.
> 
> That is precisely the reason why I am concerned.  If it was only during 
> system-wide resume, the impact of the very-long mdelay() would be more 
> difficult to notice.
> 
> You also ignored my question :)

I didn't ignore it; I answered it with a question!  If you had
answered mine, you'd have had the answer to yours ... :)

One way another task can be active during resume is with sysfs:
"echo -n 0 > /sys/devices/.../power/state", after similar selective
suspend of the device.  That's uncommon for now, primarily useful
for unit-testing driver suspend/resume.  Plus, its design is
currently borked; the pm core code doesn't bother to suspend
children of the device first.  But I do expect that selective
suspend/resume should work in Linux; it's not reasonable to design
the pm framework otherwise.

But in any case, while it'd be difficult to notice that mdelay()
in current systems (since selective suspend/resume is still rare)
it'd clearly be wrong to assume that resume() methods don't need
to have mutual exclusion during their critical sections.


> If the PCI layer is calling the resume method for a PCI device while 
> simultaneously calling the suspend method, that's a PCI layer problem.

I never suggested such a scenario.  Though that would be another
case where such critical sections matter, like the remove() method.


> Similarly, If the USB layer is calling into your driver while you are 
> resuming, something is broken and it ain't your locking.

Which gets back to the question I asked you:  if not the lock in
question, what's ensuring that everything behaves right?

As I said originally, I don't much like long udelays, but
at least it's clearly correct in terms of mutual exclusion.
You've not shown any solution that's equivalently correct.

- Dave

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[David Brownell]

On Wednesday 22 December 2004 3:59 am, Benjamin Herrenschmidt wrote:
> 
> > Similarly, If the USB layer is calling into your driver while you are 
> > resuming, something is broken and it ain't your locking.
> 
> Actually, the later isn't broken, something may well call into the
> higher level USB drivers while resuming, but indeed, those shouldn't
> send any URB down the stack when the bus is suspended, and the EHCI
> driver should drop incoming URBs as well until fully resumed.

Well, not "drop" like the network layer might, but report an error;
if the bus is suspended, so are all its devices and drivers.  When
a USB device is suspended, all its endpoint queues should be empty.
(And devices can be suspended without the bus being suspended...)

Such checks should be part of usbcore, not just one HCD, but we're
still working towards better interfaces there.  It's excessively
painful to write HCDs.


> I think the lock here is only needed to protect the HCD state
> transitions David, no ?

State transitions and access to hardware.  There are a few other
code paths that can trigger state transitions then, like rmmod.
And Alan's pointed out that we'll be wanting autoresume mechanisms,
so that parent hubs implicitly wake up as needed.

One thing we don't have right now is a per-HCD spinlock that
usbcore has access to.  Periodically I wonder whether such a
lock might help sort some of these issues out.


> All we need is make sure that we don't let 
> things get queued (or call into EHCI code path that will end up
> touch the HW) while suspended.

That lock protects all HCD code paths that touch HW,
As well as the state transitions.  I don't recall that
any other state transitions require such a long delay,
except maybe initialization (which happens before the
upper levels of the USB stack see the controller).

- Dave

Re: [PATCH] USB: fix Scheduling while atomic warning when resuming.

[Benjamin Herrenschmidt]

On Wed, 2004-12-22 at 08:16 -0800, David Brownell wrote:
> On Wednesday 22 December 2004 3:59 am, Benjamin Herrenschmidt wrote:
> > 
> > > Similarly, If the USB layer is calling into your driver while you are 
> > > resuming, something is broken and it ain't your locking.
> > 
> > Actually, the later isn't broken, something may well call into the
> > higher level USB drivers while resuming, but indeed, those shouldn't
> > send any URB down the stack when the bus is suspended, and the EHCI
> > driver should drop incoming URBs as well until fully resumed.
> 
> Well, not "drop" like the network layer might, but report an error;
> if the bus is suspended, so are all its devices and drivers.  When
> a USB device is suspended, all its endpoint queues should be empty.
> (And devices can be suspended without the bus being suspended...)

Oh sure, yes, "drop" wasn't the best choice of word, but error is what I
meant. Sorry.

> Such checks should be part of usbcore, not just one HCD, but we're
> still working towards better interfaces there.  It's excessively
> painful to write HCDs.

Yes :)
> 
> > I think the lock here is only needed to protect the HCD state
> > transitions David, no ?
> 
> State transitions and access to hardware.  There are a few other
> code paths that can trigger state transitions then, like rmmod.
> And Alan's pointed out that we'll be wanting autoresume mechanisms,
> so that parent hubs implicitly wake up as needed.
> 
> One thing we don't have right now is a per-HCD spinlock that
> usbcore has access to.  Periodically I wonder whether such a
> lock might help sort some of these issues out.
> 
> 
> > All we need is make sure that we don't let 
> > things get queued (or call into EHCI code path that will end up
> > touch the HW) while suspended.
> 
> That lock protects all HCD code paths that touch HW,
> As well as the state transitions.  I don't recall that
> any other state transitions require such a long delay,
> except maybe initialization (which happens before the
> upper levels of the USB stack see the controller).
> 
> - Dave
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>