[PATCH] get USB suspend to work again on 2.6.17-mm1

[PATCH] get USB suspend to work again on 2.6.17-mm1

[Greg KH]

Mattai and Jiri, can you try the patch below to see if it fixes the USB
suspend problem you are seeing with 2.6.17-mm1?

David, we really should not be caring about what the children of a USB
device is doing here, as who knows what type of "device" might hang off
of a struct usb_device.  This patch is just a band-aid around this area,
until Alan's patches fix up everything "properly" :)

thanks,

greg k-h

-----------------------------
Subject: USB: get USB suspend to work again

Yeah, it's a hack, but it is only temporary until Alan's patches
reworking this area make it in.  We really should not care what devices
below us are doing, especially when we do not really know what type of
devices they are.  This patch relies on the fact that the endpoint
devices do not have a driver assigned to us.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/usb/core/usb.c |    2 ++
 1 file changed, 2 insertions(+)

--- gregkh-2.6.orig/drivers/usb/core/usb.c
+++ gregkh-2.6/drivers/usb/core/usb.c
@@ -991,6 +991,8 @@ void usb_buffer_unmap_sg (struct usb_dev
 
 static int verify_suspended(struct device *dev, void *unused)
 {
+	if (dev->driver == NULL)
+		return 0;
 	return (dev->power.power_state.event == PM_EVENT_ON) ? -EBUSY : 0;
 }
 

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Jiri Slaby]

Greg KH napsal(a):
> Mattai and Jiri, can you try the patch below to see if it fixes the USB
> suspend problem you are seeing with 2.6.17-mm1?
> 
> David, we really should not be caring about what the children of a USB
> device is doing here, as who knows what type of "device" might hang off
> of a struct usb_device.  This patch is just a band-aid around this area,
> until Alan's patches fix up everything "properly" :)
> 
> thanks,
> 
> greg k-h
> 
> -----------------------------
> Subject: USB: get USB suspend to work again
> 
> Yeah, it's a hack, but it is only temporary until Alan's patches
> reworking this area make it in.  We really should not care what devices
> below us are doing, especially when we do not really know what type of
> devices they are.  This patch relies on the fact that the endpoint
> devices do not have a driver assigned to us.
> 
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> ---
>  drivers/usb/core/usb.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- gregkh-2.6.orig/drivers/usb/core/usb.c
> +++ gregkh-2.6/drivers/usb/core/usb.c
> @@ -991,6 +991,8 @@ void usb_buffer_unmap_sg (struct usb_dev
>  
>  static int verify_suspended(struct device *dev, void *unused)
>  {
> +	if (dev->driver == NULL)
> +		return 0;
>  	return (dev->power.power_state.event == PM_EVENT_ON) ? -EBUSY : 0;
>  }
>  

Yeah, it works just fine.

regards,
-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Mattia Dongili]

On Thu, Jun 22, 2006 at 01:29:52PM -0700, Greg KH wrote:
> Mattai and Jiri, can you try the patch below to see if it fixes the USB
> suspend problem you are seeing with 2.6.17-mm1?

it does, s2ram now works again

thanks
-- 
mattia
:wq!

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Mattia Dongili]

On Thu, Jun 22, 2006 at 01:29:52PM -0700, Greg KH wrote:
> Mattai and Jiri, can you try the patch below to see if it fixes the USB
> suspend problem you are seeing with 2.6.17-mm1?

great! it also solves the s2disk problems I was seeing since
2.6.17-rc5-mm1.

double thanks!
-- 
mattia
:wq!

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[David Brownell]

On Thursday 22 June 2006 1:29 pm, Greg KH wrote:
> 
> David, we really should not be caring about what the children of a USB
> device is doing here, as who knows what type of "device" might hang off
> of a struct usb_device. 

Should be _only_ interfaces; everything else descends from an interface.

There was previously an invariant that the interfaces were marked
as quiescent unless the interface (a) had a driver, and (b) that
driver was not suspended.  Evidently that has been lost.  This patch
may be insufficient; ISTR other places relying on that invariant.

And yes, we _should_ care about whether or not any interface is
still active, until the pm core code starts to pay attention to
the driver model tree at all times ... even outside of system-wide
suspend transitions.  Today, the pm core code doesn't even use
that tree directly, and all runtime state changes (like selective
suspend with USB) completely bypass that pm tree.

- Dave



> --- gregkh-2.6.orig/drivers/usb/core/usb.c
> +++ gregkh-2.6/drivers/usb/core/usb.c
> @@ -991,6 +991,8 @@ void usb_buffer_unmap_sg (struct usb_dev
>  
>  static int verify_suspended(struct device *dev, void *unused)
>  {
> +	if (dev->driver == NULL)
> +		return 0;
>  	return (dev->power.power_state.event == PM_EVENT_ON) ? -EBUSY : 0;
>  }
>  
> 

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Greg KH]

On Thu, Jun 22, 2006 at 04:24:02PM -0700, David Brownell wrote:
> On Thursday 22 June 2006 1:29 pm, Greg KH wrote:
> > 
> > David, we really should not be caring about what the children of a USB
> > device is doing here, as who knows what type of "device" might hang off
> > of a struct usb_device. 
> 
> Should be _only_ interfaces; everything else descends from an interface.

Not anymore, and who knows what might hang off a USB device in the
future.  We can't necessarily control our children like this, as some
other subsystem might want to use a usb_device as a parent, and there's
nothing wrong with that.

> There was previously an invariant that the interfaces were marked
> as quiescent unless the interface (a) had a driver, and (b) that
> driver was not suspended.  Evidently that has been lost.  This patch
> may be insufficient; ISTR other places relying on that invariant.
> 
> And yes, we _should_ care about whether or not any interface is
> still active, until the pm core code starts to pay attention to
> the driver model tree at all times ... even outside of system-wide
> suspend transitions.  Today, the pm core code doesn't even use
> that tree directly, and all runtime state changes (like selective
> suspend with USB) completely bypass that pm tree.

Hm, ok, yes, we should care about interfaces, but we need some way to
only walk them, not anything else that might be attached to us...

thanks,

greg k-h

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Alan Stern]

On Thu, 22 Jun 2006, Greg KH wrote:

> > And yes, we _should_ care about whether or not any interface is
> > still active, until the pm core code starts to pay attention to
> > the driver model tree at all times ... even outside of system-wide
> > suspend transitions.  Today, the pm core code doesn't even use
> > that tree directly, and all runtime state changes (like selective
> > suspend with USB) completely bypass that pm tree.
> 
> Hm, ok, yes, we should care about interfaces, but we need some way to
> only walk them, not anything else that might be attached to us...

In my upcoming patch set this test isn't needed at all, because suspending
a device automatically suspends all of its interfaces first.  I've already
submitted the first few revised patches in that set (not the part that
removes the test, though), but you've probably been too busy to look at
them yet.

Alan Stern

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[David Brownell]

On Thursday 22 June 2006 4:51 pm, Greg KH wrote:
> > There was previously an invariant that the interfaces were marked
> > as quiescent unless the interface (a) had a driver, and (b) that
> > driver was not suspended.  Evidently that has been lost.  This patch
> > may be insufficient; ISTR other places relying on that invariant.
> > 
> > And yes, we _should_ care about whether or not any interface is
> > still active, until the pm core code starts to pay attention to
> > the driver model tree at all times ... even outside of system-wide
> > suspend transitions.  Today, the pm core code doesn't even use
> > that tree directly, and all runtime state changes (like selective
> > suspend with USB) completely bypass that pm tree.
> 
> Hm, ok, yes, we should care about interfaces, but we need some way to
> only walk them, not anything else that might be attached to us...

Under what scenario could it possibly be legitimate to suspend a
usb device -- or interface, or anything else -- with its children
remaining active?  The ability to guarantee that could _never_ happen
was one of the fundamental motivations for the driver model ...

Maybe that invariant should be replaced with something else, though
I'm not sure what.  Certainly it would make a fair amount of sense
to leave unused PCI devices in the PCI_D3 state, for example; that
would be the PCI analogue of that invariant.

- Dave

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Greg KH]

On Thu, Jun 22, 2006 at 08:34:43PM -0700, David Brownell wrote:
> On Thursday 22 June 2006 4:51 pm, Greg KH wrote:
> > > There was previously an invariant that the interfaces were marked
> > > as quiescent unless the interface (a) had a driver, and (b) that
> > > driver was not suspended.  Evidently that has been lost.  This patch
> > > may be insufficient; ISTR other places relying on that invariant.
> > > 
> > > And yes, we _should_ care about whether or not any interface is
> > > still active, until the pm core code starts to pay attention to
> > > the driver model tree at all times ... even outside of system-wide
> > > suspend transitions.  Today, the pm core code doesn't even use
> > > that tree directly, and all runtime state changes (like selective
> > > suspend with USB) completely bypass that pm tree.
> > 
> > Hm, ok, yes, we should care about interfaces, but we need some way to
> > only walk them, not anything else that might be attached to us...
> 
> Under what scenario could it possibly be legitimate to suspend a
> usb device -- or interface, or anything else -- with its children
> remaining active?  The ability to guarantee that could _never_ happen
> was one of the fundamental motivations for the driver model ...

I'm not disagreeing with that.  It's just that you are looping all
struct devices that are attached to a struct usb_device and assuming
that they are all of type struct usb_interface.  And that's not true
anymore, and should never have been assumed (which is probably my fault
way long ago when converting USB to the driver model.)

We probably need to keep our own list of interfaces if we want to
properly walk them now...

And we also need to be able to handle devices in the device tree that do
not have a suspend/resume function, or ones that are not attached to any
bus, without failing the suspend, as obviously they do not care or need
to worry about the whole issue.

thanks,

greg k-h

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Greg KH]

On Thu, Jun 22, 2006 at 10:45:15PM -0400, Alan Stern wrote:
> On Thu, 22 Jun 2006, Greg KH wrote:
> 
> > > And yes, we _should_ care about whether or not any interface is
> > > still active, until the pm core code starts to pay attention to
> > > the driver model tree at all times ... even outside of system-wide
> > > suspend transitions.  Today, the pm core code doesn't even use
> > > that tree directly, and all runtime state changes (like selective
> > > suspend with USB) completely bypass that pm tree.
> > 
> > Hm, ok, yes, we should care about interfaces, but we need some way to
> > only walk them, not anything else that might be attached to us...
> 
> In my upcoming patch set this test isn't needed at all, because suspending
> a device automatically suspends all of its interfaces first.  I've already
> submitted the first few revised patches in that set (not the part that
> removes the test, though), but you've probably been too busy to look at
> them yet.

I've glanced at them (and yes, been busy, they are still in my TO-APPLY
queue, trying to sync up with Linus first), but I don't see anything in
that set that changes the suspend logic.

Or am I just missing something obvious?  Which patch does that in your
revised series?

thanks,

greg k-h

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Alan Stern]

On Thu, 22 Jun 2006, Greg KH wrote:

> > In my upcoming patch set this test isn't needed at all, because suspending
> > a device automatically suspends all of its interfaces first.  I've already
> > submitted the first few revised patches in that set (not the part that
> > removes the test, though), but you've probably been too busy to look at
> > them yet.
> 
> I've glanced at them (and yes, been busy, they are still in my TO-APPLY
> queue, trying to sync up with Linus first), but I don't see anything in
> that set that changes the suspend logic.
> 
> Or am I just missing something obvious?  Which patch does that in your
> revised series?

It's not in any of the pieces you've gotten so far.  It's about 3 patches 
farther down the line.

Alan Stern

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Alan Stern]

On Thu, 22 Jun 2006, Greg KH wrote:

> > Under what scenario could it possibly be legitimate to suspend a
> > usb device -- or interface, or anything else -- with its children
> > remaining active?  The ability to guarantee that could _never_ happen
> > was one of the fundamental motivations for the driver model ...
> 
> I'm not disagreeing with that.  It's just that you are looping all
> struct devices that are attached to a struct usb_device and assuming
> that they are all of type struct usb_interface.  And that's not true
> anymore, and should never have been assumed (which is probably my fault
> way long ago when converting USB to the driver model.)

In fact the code doesn't make that assumption.  It only assumes that the 
dev->power.power_state.event field is set correctly (0 for not suspended, 
non-zero for suspended) and that you don't want to suspend a device if its 
children aren't all suspended.

> We probably need to keep our own list of interfaces if we want to
> properly walk them now...

We do have such a list: udev->actconfig->interface[].

> And we also need to be able to handle devices in the device tree that do
> not have a suspend/resume function, or ones that are not attached to any
> bus, without failing the suspend, as obviously they do not care or need
> to worry about the whole issue.

Ah, there's the rub.  If a driver doesn't have suspend/resume methods, is 
it because it doesn't need them, or is it because nobody has written them 
yet?  In the latter case, failing the suspend or unbinding the driver are 
the only safe courses.

And when you're dealing with a device that isn't on a bus or doesn't have 
a driver, then clearly you _can't_ suspend it.  You can abort the system 
sleep, or you can go ahead knowing that the device may not be in a 
low-power mode.

Alan Stern

Re: [linux-usb-devel] [PATCH] get USB suspend to work again on 2.6.17-mm1

[David Brownell]

On Friday 23 June 2006 7:51 am, Alan Stern wrote:

> Ah, there's the rub.  If a driver doesn't have suspend/resume methods, is 
> it because it doesn't need them, or is it because nobody has written them 
> yet?  In the latter case, failing the suspend or unbinding the driver are 
> the only safe courses.

I think the former would ba a dangerous assumption ... in the category
of "intermittent bugs triggering later on" rather than "easily reproduced
bugs triggering right at the trouble spot" (like the latter).

- Dave

Re: [linux-pm] [PATCH] get USB suspend to work again on 2.6.17-mm1

[Jim Gettys]

On Thu, 2006-06-22 at 20:34 -0700, David Brownell wrote:

> Under what scenario could it possibly be legitimate to suspend a
> usb device -- or interface, or anything else -- with its children
> remaining active?  The ability to guarantee that could _never_ happen
> was one of the fundamental motivations for the driver model ...
> 

I'm not sure this directly applies, but....

The Marvell wireless chip we're using this generation in the OLPC
machine is interfaced via USB.  Not ideal, but there's no other game in
town to let us keep the mesh network up while the main machine is STR.

We intend to leave the Marvell chip on (it can forward packets in the
mesh network, and/or wake up the CPU if there are inbound packets for
the machine that matter), and turn off the USB interface it is attached
to.
                        Regards,
                             - Jim

-- 
Jim Gettys
One Laptop Per Child

Re: [linux-pm] [PATCH] get USB suspend to work again on 2.6.17-mm1

[David Brownell]

On Saturday 24 June 2006 7:42 pm, Jim Gettys wrote:
> On Thu, 2006-06-22 at 20:34 -0700, David Brownell wrote:
> 
> > Under what scenario could it possibly be legitimate to suspend a
> > usb device -- or interface, or anything else -- with its children
> > remaining active?  The ability to guarantee that could _never_ happen
> > was one of the fundamental motivations for the driver model ...
> 
> I'm not sure this directly applies, but....

It's not a counterexample, but it may be an interesting example of
the sort of loosely coupled multiprocessor that gets more common.
The different processors use different power management schemes.
(I think an ACPI "embedded processor" has related issues.)


> The Marvell wireless chip we're using this generation in the OLPC
> machine is interfaced via USB.  Not ideal, but there's no other game in
> town to let us keep the mesh network up while the main machine is STR.

So presumably it's both "self" powered (so far as the parent hub is
concerned!) and also has some kind of cpu and firmware.  I'll assume
this chipset is used with discrete products too, not only for wiring to
motherboards.  (Despite the board layout advantages of using serial
interfaces:  fewer high speed signal lines, etc.)


> We intend to leave the Marvell chip on (it can forward packets in the
> mesh network, and/or wake up the CPU if there are inbound packets for
> the machine that matter), and turn off the USB interface it is attached
> to.

The normal way to do such things -- from the perspective of the firmware
inside a USB peripheral doing that routing etc -- recognizes that the
USB suspend state affects only the upstream link, and uses remote wakeup
signaling in the normal fashion.  (An SPI or I2C/SMBUS based coprocessor
probably needs an IRQ signal line.)

That is, from the perspective of the host CPU, it's suspended normally.
Just like any other USB device (like I sketched above).

But the peripheral's CPU can continue doing whatever it likes ... which
doesn't necessarily include stopping.  Bus powered USB peripherals would
probably try to suspend their CPUs though, since otherwise it may be hard
to meet the 500 microAmpere power budget.

- Dave

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Greg KH]

On Fri, Jun 23, 2006 at 10:51:47AM -0400, Alan Stern wrote:
> On Thu, 22 Jun 2006, Greg KH wrote:
> 
> > > Under what scenario could it possibly be legitimate to suspend a
> > > usb device -- or interface, or anything else -- with its children
> > > remaining active?  The ability to guarantee that could _never_ happen
> > > was one of the fundamental motivations for the driver model ...
> > 
> > I'm not disagreeing with that.  It's just that you are looping all
> > struct devices that are attached to a struct usb_device and assuming
> > that they are all of type struct usb_interface.  And that's not true
> > anymore, and should never have been assumed (which is probably my fault
> > way long ago when converting USB to the driver model.)
> 
> In fact the code doesn't make that assumption.  It only assumes that the 
> dev->power.power_state.event field is set correctly (0 for not suspended, 
> non-zero for suspended) and that you don't want to suspend a device if its 
> children aren't all suspended.

Yes, but it's looking at devices it should _not_ care about.  The USB
core should only care about devices it controls, not other devices in
the device chain.  Those are for the driver core to handle.

> > We probably need to keep our own list of interfaces if we want to
> > properly walk them now...
> 
> We do have such a list: udev->actconfig->interface[].

Ah, ok, I thought it was somewhere...  David, why don't we walk that
list instead?

> > And we also need to be able to handle devices in the device tree that do
> > not have a suspend/resume function, or ones that are not attached to any
> > bus, without failing the suspend, as obviously they do not care or need
> > to worry about the whole issue.
> 
> Ah, there's the rub.  If a driver doesn't have suspend/resume methods, is 
> it because it doesn't need them, or is it because nobody has written them 
> yet?  In the latter case, failing the suspend or unbinding the driver are 
> the only safe courses.

No, if it's not there, just expect that it knows what it is doing, and
don't fail the thing.  Unless you want to add those methods to _every_
driver in the kernel, and that's going to be a lot of work...

> And when you're dealing with a device that isn't on a bus or doesn't have 
> a driver, then clearly you _can't_ suspend it.  You can abort the system 
> sleep, or you can go ahead knowing that the device may not be in a 
> low-power mode.

It's a virtual device, it can go to sleep just fine with nothing needed
to have done at all, as it's just in kernel memory, no physical thing to
turn to "low power" mode at all.

thanks,

greg k-h

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[David Brownell]

On Monday 26 June 2006 4:57 pm, Greg KH wrote:
> On Fri, Jun 23, 2006 at 10:51:47AM -0400, Alan Stern wrote:
> > On Thu, 22 Jun 2006, Greg KH wrote:
> > 
> > > > Under what scenario could it possibly be legitimate to suspend a
> > > > usb device -- or interface, or anything else -- with its children
> > > > remaining active?  The ability to guarantee that could _never_ happen
> > > > was one of the fundamental motivations for the driver model ...
> > > 
> > > I'm not disagreeing with that.  It's just that you are looping all
> > > struct devices that are attached to a struct usb_device and assuming
> > > that they are all of type struct usb_interface. ...
> > 
> > In fact the code doesn't make that assumption.  It only assumes that the 
> > dev->power.power_state.event field is set correctly ...
> 
> Yes, but it's looking at devices it should _not_ care about.  The USB
> core should only care about devices it controls, not other devices in
> the device chain.  Those are for the driver core to handle.

The basic problem is that the driver core does ** NOT ** maintain such
integrity constraints.  So it's unsafe to remove those checks for cases
(like USB) where devices get suspended outside transition to "system sleep"
states like "standby", "suspend-to-ram", and "suspend-to-disk". [1]

Go back to my original question:  is there a legitimate scenario where
that test should fail?  Nobody has come up with even one ...


Even so-called "virtual" devices (talking to abstracted hardware) need to
quiesce.  And as Adam has pointed out separately, often most of the work to
quiesce drivers should be at such a "virtual" level.  Most of the time when
a driver for a "physical" device (touches real registers) does complicated
work to quiesce, it's because the next level in the driver stack didn't
create a "virtual" device to package that logic.  As with "eth0".


> > > We probably need to keep our own list of interfaces if we want to
> > > properly walk them now...
> > 
> > We do have such a list: udev->actconfig->interface[].
> 
> Ah, ok, I thought it was somewhere...  David, why don't we walk that
> list instead?

Because the type of the child doesn't matter.  If it hasn't suspended,
the parent must not suspend.  The original point of the driver model was
to be able to enforce that integrity constraint.

Plus, this way we use the driver model data structures ... in much the
way the driver model itself would, if it were maintaining such integrity
constraints.  If the driver model is ever going to fix those issues,
we'll at least know it has sufficient data at hand.

- Dave

[1] One simple example:  echo 3 > /sys/devices/.../power/state for a
    device with children that aren't suspended.  The PM core will not
    detect that error.

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Pavel Machek]

Hi!

> > > And we also need to be able to handle devices in the device tree that do
> > > not have a suspend/resume function, or ones that are not attached to any
> > > bus, without failing the suspend, as obviously they do not care or need
> > > to worry about the whole issue.
> > 
> > Ah, there's the rub.  If a driver doesn't have suspend/resume methods, is 
> > it because it doesn't need them, or is it because nobody has written them 
> > yet?  In the latter case, failing the suspend or unbinding the driver are 
> > the only safe courses.
> 
> No, if it's not there, just expect that it knows what it is doing, and
> don't fail the thing.  Unless you want to add those methods to _every_
> driver in the kernel, and that's going to be a lot of work...

I believe 90% of drivers need them, anyway... Idea is that if we
refuse the suspend, user has dmesg and did not loose his work. If we
suspend but can't resume due to driver problems, it is slightly more
interesting to debug, and user lost open applications.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Alan Stern]

On Mon, 26 Jun 2006, David Brownell wrote:

> On Monday 26 June 2006 4:57 pm, Greg KH wrote:
> > On Fri, Jun 23, 2006 at 10:51:47AM -0400, Alan Stern wrote:
> > > On Thu, 22 Jun 2006, Greg KH wrote:
> > > 
> > > > > Under what scenario could it possibly be legitimate to suspend a
> > > > > usb device -- or interface, or anything else -- with its children
> > > > > remaining active?  The ability to guarantee that could _never_ happen
> > > > > was one of the fundamental motivations for the driver model ...
> > > > 
> > > > I'm not disagreeing with that.  It's just that you are looping all
> > > > struct devices that are attached to a struct usb_device and assuming
> > > > that they are all of type struct usb_interface. ...
> > > 
> > > In fact the code doesn't make that assumption.  It only assumes that the 
> > > dev->power.power_state.event field is set correctly ...
> > 
> > Yes, but it's looking at devices it should _not_ care about.  The USB
> > core should only care about devices it controls, not other devices in
> > the device chain.  Those are for the driver core to handle.
> 
> The basic problem is that the driver core does ** NOT ** maintain such
> integrity constraints.  So it's unsafe to remove those checks for cases
> (like USB) where devices get suspended outside transition to "system sleep"
> states like "standby", "suspend-to-ram", and "suspend-to-disk". [1]
> 
> Go back to my original question:  is there a legitimate scenario where
> that test should fail?  Nobody has come up with even one ...
> 
> 
> Even so-called "virtual" devices (talking to abstracted hardware) need to
> quiesce.  And as Adam has pointed out separately, often most of the work to
> quiesce drivers should be at such a "virtual" level.  Most of the time when
> a driver for a "physical" device (touches real registers) does complicated
> work to quiesce, it's because the next level in the driver stack didn't
> create a "virtual" device to package that logic.  As with "eth0".

An easy way around the problem is to create simple suspend/resume methods 
for the endpoint devices.  They don't have to do anything other than set 
dev->power.power_state.event.  Not until these "endpoint devices" start 
implementing some real functionality.

Alan Stern

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[David Brownell]

> > > > And we also need to be able to handle devices in the device tree that do
> > > > not have a suspend/resume function ...
> > >
> > > Ah, there's the rub.  If a driver doesn't have suspend/resume methods, is 
> > > it because it doesn't need them, or is it because nobody has written them 
> > > yet?  In the latter case, failing the suspend or unbinding the driver are 
> > > the only safe courses.
> > 
> > No, if it's not there, just expect that it knows what it is doing, and
> > don't fail the thing.  Unless you want to add those methods to _every_
> > driver in the kernel, and that's going to be a lot of work...

It seems reasonable to me to require that drivers have at least
stub "it's actually OK to do nothing here" suspend/resume methods.


> I believe 90% of drivers need them, anyway... Idea is that if we
> refuse the suspend, user has dmesg and did not loose his work. If we
> suspend but can't resume due to driver problems, it is slightly more
> interesting to debug, and user lost open applications.

Or as I put it earlier, a clean failure right at the "suspend/resume
method missing" point is more robust than unpredictable failures much
later (long after that actual failure points).

- Dave

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Greg KH]

On Tue, Jun 27, 2006 at 10:38:39AM -0700, David Brownell wrote:
> > > > > And we also need to be able to handle devices in the device tree that do
> > > > > not have a suspend/resume function ...
> > > >
> > > > Ah, there's the rub.  If a driver doesn't have suspend/resume methods, is 
> > > > it because it doesn't need them, or is it because nobody has written them 
> > > > yet?  In the latter case, failing the suspend or unbinding the driver are 
> > > > the only safe courses.
> > > 
> > > No, if it's not there, just expect that it knows what it is doing, and
> > > don't fail the thing.  Unless you want to add those methods to _every_
> > > driver in the kernel, and that's going to be a lot of work...
> 
> It seems reasonable to me to require that drivers have at least
> stub "it's actually OK to do nothing here" suspend/resume methods.

No, the point is that these devices have no driver associated with them.
They are "class" devices, and as such, are virtual.

Hm, well, I guess I should go add the suspend callbacks to the class, as
Linus's core changes is going to be expecting that...

Anyway, for virtual devices, it often times makes no sense to have a
suspend function, and as such, they should not be required to provide a
null function...

thanks,

greg k-h

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Greg KH]

On Mon, Jun 26, 2006 at 07:04:05PM -0700, David Brownell wrote:
> On Monday 26 June 2006 4:57 pm, Greg KH wrote:
> > On Fri, Jun 23, 2006 at 10:51:47AM -0400, Alan Stern wrote:
> > > On Thu, 22 Jun 2006, Greg KH wrote:
> > > 
> > > > > Under what scenario could it possibly be legitimate to suspend a
> > > > > usb device -- or interface, or anything else -- with its children
> > > > > remaining active?  The ability to guarantee that could _never_ happen
> > > > > was one of the fundamental motivations for the driver model ...
> > > > 
> > > > I'm not disagreeing with that.  It's just that you are looping all
> > > > struct devices that are attached to a struct usb_device and assuming
> > > > that they are all of type struct usb_interface. ...
> > > 
> > > In fact the code doesn't make that assumption.  It only assumes that the 
> > > dev->power.power_state.event field is set correctly ...
> > 
> > Yes, but it's looking at devices it should _not_ care about.  The USB
> > core should only care about devices it controls, not other devices in
> > the device chain.  Those are for the driver core to handle.
> 
> The basic problem is that the driver core does ** NOT ** maintain such
> integrity constraints.  So it's unsafe to remove those checks for cases
> (like USB) where devices get suspended outside transition to "system sleep"
> states like "standby", "suspend-to-ram", and "suspend-to-disk". [1]
> 
> Go back to my original question:  is there a legitimate scenario where
> that test should fail?  Nobody has come up with even one ...

Again, virtual devices that are no associated with any driver.  Exactly
the situation we have today with the usb endpoints and the usb_device
structures. 

And again, the USB core should not be messing with any devices that it
does not control, that just happen to be hanging off of the USB devices.

> Even so-called "virtual" devices (talking to abstracted hardware) need to
> quiesce.

No they don't.  If they did, they would be bound to a driver and provide
a suspend method.  Look at the current code for 2 examples of struct
devices that meet this critera.

> > Ah, ok, I thought it was somewhere...  David, why don't we walk that
> > list instead?
> 
> Because the type of the child doesn't matter.  If it hasn't suspended,
> the parent must not suspend.  The original point of the driver model was
> to be able to enforce that integrity constraint.

Well, who knows what the original point of the driver model was in this
regard, as that developer is not helping with development anymore :(

> Plus, this way we use the driver model data structures ... in much the
> way the driver model itself would, if it were maintaining such integrity
> constraints.  If the driver model is ever going to fix those issues,
> we'll at least know it has sufficient data at hand.

If we want to add these kinds of constraints to the driver model, great,
let's add them to the core and have that discussion.  But we should NOT
be adding it to a random driver subsystem to try to enforce requirements
the rest of the driver tree does not obey.  That's just insane.

thanks,

greg k-h

Re: [PATCH] get USB suspend to work again on 2.6.17-mm1

[Greg KH]

On Tue, Jun 27, 2006 at 11:24:50AM -0400, Alan Stern wrote:
> On Mon, 26 Jun 2006, David Brownell wrote:
> 
> > On Monday 26 June 2006 4:57 pm, Greg KH wrote:
> > > On Fri, Jun 23, 2006 at 10:51:47AM -0400, Alan Stern wrote:
> > > > On Thu, 22 Jun 2006, Greg KH wrote:
> > > > 
> > > > > > Under what scenario could it possibly be legitimate to suspend a
> > > > > > usb device -- or interface, or anything else -- with its children
> > > > > > remaining active?  The ability to guarantee that could _never_ happen
> > > > > > was one of the fundamental motivations for the driver model ...
> > > > > 
> > > > > I'm not disagreeing with that.  It's just that you are looping all
> > > > > struct devices that are attached to a struct usb_device and assuming
> > > > > that they are all of type struct usb_interface. ...
> > > > 
> > > > In fact the code doesn't make that assumption.  It only assumes that the 
> > > > dev->power.power_state.event field is set correctly ...
> > > 
> > > Yes, but it's looking at devices it should _not_ care about.  The USB
> > > core should only care about devices it controls, not other devices in
> > > the device chain.  Those are for the driver core to handle.
> > 
> > The basic problem is that the driver core does ** NOT ** maintain such
> > integrity constraints.  So it's unsafe to remove those checks for cases
> > (like USB) where devices get suspended outside transition to "system sleep"
> > states like "standby", "suspend-to-ram", and "suspend-to-disk". [1]
> > 
> > Go back to my original question:  is there a legitimate scenario where
> > that test should fail?  Nobody has come up with even one ...
> > 
> > 
> > Even so-called "virtual" devices (talking to abstracted hardware) need to
> > quiesce.  And as Adam has pointed out separately, often most of the work to
> > quiesce drivers should be at such a "virtual" level.  Most of the time when
> > a driver for a "physical" device (touches real registers) does complicated
> > work to quiesce, it's because the next level in the driver stack didn't
> > create a "virtual" device to package that logic.  As with "eth0".
> 
> An easy way around the problem is to create simple suspend/resume methods 
> for the endpoint devices.  They don't have to do anything other than set 
> dev->power.power_state.event.  Not until these "endpoint devices" start 
> implementing some real functionality.

No.  Are we going to require that we do that for _every_ type of device
that might possibly hang off of a USB device?  Again, that's a
constraint the driver model currently does not impose on the rest of the
tree, so we should not have the USB core try to impose it on parts that
happen to hook up to it.

thanks,

greg k-h