Hi !
What about this patch to stay compatible with existing drivers
implementing everything in save_state ?
Ideally, we should kill save_state and fix all drivers, oh well...
(This version of the patch has correct space/tabs)
Ben.
--- a/drivers/pci/pci-driver.c Sat Aug 23 16:39:14 2003
+++ b/drivers/pci/pci-driver.c Sat Aug 23 16:39:14 2003
@@ -163,6 +163,9 @@
struct pci_dev * pci_dev = to_pci_dev(dev);
struct pci_driver * drv = pci_dev->driver;
+ /* Compatibility with drivers using obsolete save_state */
+ if (drv && drv->save_state)
+ return drv->save_state(pci_dev,state);
if (drv && drv->suspend)
return drv->suspend(pci_dev,state);
return 0;
On Sat, Aug 23, 2003 at 04:39:57PM +0200, Benjamin Herrenschmidt wrote:
> What about this patch to stay compatible with existing drivers
> implementing everything in save_state ?
And why are we now suspending device parents before their siblings???
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Sat, 2003-08-23 at 19:08, Russell King wrote:
> On Sat, Aug 23, 2003 at 04:39:57PM +0200, Benjamin Herrenschmidt wrote:
> > What about this patch to stay compatible with existing drivers
> > implementing everything in save_state ?
>
> And why are we now suspending device parents before their siblings???
I didn't notice that on the laptop here (because it's actually harmless
on this specific machine), but if this is the case, then we are badly
wrong indeed...
Ben.
On Sat, Aug 23, 2003 at 07:28:25PM +0200, Benjamin Herrenschmidt wrote:
> On Sat, 2003-08-23 at 19:08, Russell King wrote:
> > On Sat, Aug 23, 2003 at 04:39:57PM +0200, Benjamin Herrenschmidt wrote:
> > > What about this patch to stay compatible with existing drivers
> > > implementing everything in save_state ?
> >
> > And why are we now suspending device parents before their siblings???
>
> I didn't notice that on the laptop here (because it's actually harmless
> on this specific machine), but if this is the case, then we are badly
> wrong indeed...
I think the problem is fairly fundamental to the way that we add
devices to the power lists.
When a device "A" is registered with the driver model, we call
device_add(). This adds the device to the bus, which calls any
registered driver methods. Once the device has been added to the
bus, we add the device to the tail of the device power list.
In the case where a device driver for device "A" registers further
devices, which are siblings of device "A", two things can happen:
- If a driver is already registered for the device, we call its
probe method before we have added device "A" to the power lists.
Therefore, the siblings will be added to the tail of the device
power list _before_ the device "A" has been added.
- If the driver registers after the device, the device "A" will be
added to the power list _before_ the siblings.
So in one case, we have PARENT SIBLING SIBLING SIBLING and in the
other case we have SIBLING SIBLING SIBLING PARENT.
Now, we do have this pm_users thing which seems to imply that it
describes the relationship between two devices. However, its non-
functional. It operates on a per-device variable called "pm_users"
which is only ever _written_ !
So, it looks like the new power model is currently half a solution...
Therefore, we need to do one of two things. Either revert Pat's
changes and return to a power model which for the most part works,
or the new power management model needs a serious work-over.
I stand by my earlier comments that it is too late to be doing this
type of development while we're supposed to be stabilising the kernel.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
> When a device "A" is registered with the driver model, we call
> device_add(). This adds the device to the bus, which calls any
> registered driver methods. Once the device has been added to the
> bus, we add the device to the tail of the device power list.
>
> In the case where a device driver for device "A" registers further
> devices, which are siblings of device "A", two things can happen:
>
> - If a driver is already registered for the device, we call its
> probe method before we have added device "A" to the power lists.
> Therefore, the siblings will be added to the tail of the device
> power list _before_ the device "A" has been added.
>
> - If the driver registers after the device, the device "A" will be
> added to the power list _before_ the siblings.
>
> So in one case, we have PARENT SIBLING SIBLING SIBLING and in the
> other case we have SIBLING SIBLING SIBLING PARENT.
I haven't been ignoring you. I've been trying to figure exactly what
you're talking about. Do you have a concrete example? Is it some PCMCIA
driver, or some platform driver?
I fail to see the problem, in theory, besides the driver itself. In the
case where the driver registers first, ->probe() will not be called until
it matches a device, which implies the device will have been added before
->probe() is called.
Also, I think I initially misunderstood your vocabulary. Each SIBLING is a
sibling of one another, but are CHILDREN of the PARENT. Right?
> Now, we do have this pm_users thing which seems to imply that it
> describes the relationship between two devices. However, its non-
> functional. It operates on a per-device variable called "pm_users"
> which is only ever _written_ !
Hey, I hadn't finished that part, because I considered fixing the system
power management interface more important that day.
It works like this:
- A driver is able to set ->power.pm_parent for a device by calling
device_pm_set_parent() after its registered. By default, pm_parent is
set to the phsyical parent of the device.
- A device with other devices that are dependent on it for power has a
positive pm_users count.
- A device may only be suspended when pm_users == 0.
- When a device is suspended, the pm_users count of its pm_parent is
decremented, and incremented when the device is resumed.
- device_suspend() makes multiple passes over the device list, in case
power dependencies cause some devices to be deferred. It fails with an
error (and resumes all suspended devices) if a pass was made in which
no devicse were suspended, but there are still devices with a positive
pm_users count.
Sound good?
Pat
On Tue, Aug 26, 2003 at 08:31:55AM -0700, Patrick Mochel wrote:
> > So in one case, we have PARENT SIBLING SIBLING SIBLING and in the
> > other case we have SIBLING SIBLING SIBLING PARENT.
>
> I haven't been ignoring you. I've been trying to figure exactly what
> you're talking about. Do you have a concrete example? Is it some PCMCIA
> driver, or some platform driver?
It's a multi-function chip (again):
sa1111 bus
SA1111------+-------Interrupt controller
+-------USB host
+-------PS/2 ports
The SA1111 probe function registers devices whose parent is the SA1111
device.
In this case, we end up placing these devices on the list in the wrong
order.
At the point where the SA1111 probe function is called, the SA1111 device
is not on the dpm lists, because of the ordering in device_add().
When we add the USB host, PS/2 ports etc, their parent device has not
been added to the dpm lists. Only after the SA1111 probe function
completes will the SA1111 device appear on the dpm lists, and in this
case it will appear _after_ the USB and PS/2 ports.
The effect of this is that when we do a device suspend, we suspend
the SA1111, then the PS/2 ports, then the USB host. Unfortunately
this can't work because suspending the SA1111 prevents access to
these sub-devices.
> I fail to see the problem, in theory, besides the driver itself. In the
> case where the driver registers first, ->probe() will not be called until
> it matches a device, which implies the device will have been added before
> ->probe() is called.
This doesn't happen - take a careful look at device_add(), specifically
the order of bus_add_device() and device_pm_add(). We probe the device
_before_ we add it to the PM lists.
> > Now, we do have this pm_users thing which seems to imply that it
> > describes the relationship between two devices. However, its non-
> > functional. It operates on a per-device variable called "pm_users"
> > which is only ever _written_ !
>
> Hey, I hadn't finished that part, because I considered fixing the system
> power management interface more important that day.
>
> It works like this:
>
> - A driver is able to set ->power.pm_parent for a device by calling
> device_pm_set_parent() after its registered. By default, pm_parent is
> set to the phsyical parent of the device.
>
> - A device with other devices that are dependent on it for power has a
> positive pm_users count.
>
> - A device may only be suspended when pm_users == 0.
>
> - When a device is suspended, the pm_users count of its pm_parent is
> decremented, and incremented when the device is resumed.
>
> - device_suspend() makes multiple passes over the device list, in case
> power dependencies cause some devices to be deferred. It fails with an
> error (and resumes all suspended devices) if a pass was made in which
> no devicse were suspended, but there are still devices with a positive
> pm_users count.
>
> Sound good?
It sounds more complicated than our old system, but I'm willing to give
it a go. However, some drivers only want to be notified of resume events,
so this would involve more code to explicitly handle the suspend case in
such drivers.
Dunno, I'll wait until there's a patch available and see how well it fits.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
>
> - When a device is suspended, the pm_users count of its pm_parent is
> decremented, and incremented when the device is resumed.
>
> - device_suspend() makes multiple passes over the device list, in case
> power dependencies cause some devices to be deferred. It fails with an
> error (and resumes all suspended devices) if a pass was made in which
> no devicse were suspended, but there are still devices with a positive
> pm_users count.
How do you intend to deal with the childs of the device that has
pm_users non null ?
If you don't suspend it, you must also postpone all of it's childs.
That makes the list walking slightly more tricky, or you finally go
to a real tree structure ? (Which you may have to do to implement
the set_parent() thing too, no ?
Ben.
On Tue, 26 Aug 2003, Russell King wrote:
> On Tue, Aug 26, 2003 at 08:31:55AM -0700, Patrick Mochel wrote:
> > > So in one case, we have PARENT SIBLING SIBLING SIBLING and in the
> > > other case we have SIBLING SIBLING SIBLING PARENT.
> >
> > I haven't been ignoring you. I've been trying to figure exactly what
> > you're talking about. Do you have a concrete example? Is it some PCMCIA
> > driver, or some platform driver?
>
> It's a multi-function chip (again):
>
> sa1111 bus
> SA1111------+-------Interrupt controller
> +-------USB host
> +-------PS/2 ports
>
> The SA1111 probe function registers devices whose parent is the SA1111
> device.
>
> In this case, we end up placing these devices on the list in the wrong
> order.
>
> At the point where the SA1111 probe function is called, the SA1111 device
> is not on the dpm lists, because of the ordering in device_add().
Bah, sorry, I overlooked that. The patch below will add the device before
calling bus_add_device(). Sorry about the confusion.
Pat
===== drivers/base/core.c 1.73 vs edited =====
--- 1.73/drivers/base/core.c Fri Aug 15 10:27:01 2003
+++ edited/drivers/base/core.c Wed Aug 27 15:49:08 2003
@@ -225,28 +225,30 @@
dev->kobj.parent = &parent->kobj;
if ((error = kobject_add(&dev->kobj)))
- goto register_done;
-
- /* now take care of our own registration */
-
+ goto Error;
+ if ((error = device_pm_add(dev)))
+ goto PMError;
+ if ((error = bus_add_device(dev)))
+ goto BusError;
down_write(&devices_subsys.rwsem);
if (parent)
list_add_tail(&dev->node,&parent->children);
up_write(&devices_subsys.rwsem);
- bus_add_device(dev);
-
- device_pm_add(dev);
-
/* notify platform of device entry */
if (platform_notify)
platform_notify(dev);
-
- register_done:
- if (error && parent)
- put_device(parent);
+ Done:
put_device(dev);
return error;
+ BusError:
+ device_pm_remove(dev);
+ PMError:
+ kobject_unregister(&dev->kobj);
+ Error:
+ if (parent)
+ put_device(parent);
+ goto Done;
}
@@ -312,8 +314,6 @@
{
struct device * parent = dev->parent;
- device_pm_remove(dev);
-
down_write(&devices_subsys.rwsem);
if (parent)
list_del_init(&dev->node);
@@ -324,14 +324,11 @@
*/
if (platform_notify_remove)
platform_notify_remove(dev);
-
bus_remove_device(dev);
-
+ device_pm_remove(dev);
kobject_del(&dev->kobj);
-
if (parent)
put_device(parent);
-
}
/**
> > - When a device is suspended, the pm_users count of its pm_parent is
> > decremented, and incremented when the device is resumed.
> >
> > - device_suspend() makes multiple passes over the device list, in case
> > power dependencies cause some devices to be deferred. It fails with an
> > error (and resumes all suspended devices) if a pass was made in which
> > no devicse were suspended, but there are still devices with a positive
> > pm_users count.
>
> How do you intend to deal with the childs of the device that has
> pm_users non null ?
>
> If you don't suspend it, you must also postpone all of it's childs.
> That makes the list walking slightly more tricky, or you finally go
> to a real tree structure ? (Which you may have to do to implement
> the set_parent() thing too, no ?
I don't understand. We suspend the children before we suspend the device,
so as long as all the children go done, so will the parent device.
Pat
On Thu, 2003-08-28 at 01:29, Patrick Mochel wrote:
> I don't understand. We suspend the children before we suspend the device,
> so as long as all the children go done, so will the parent device.
Forget it... It had crap in mind. Anyway, my point was actually the
opposite than what I wrote :( I was really to make sure that if a
device is "held" up by pm_users beeing non-NULL, it's _parents_
(and not it's children, sorry about the confusion), are also held
non-suspended... I think I'll sleep a bit and then just go look at
the code ;)
Ben.
On Wed, Aug 27, 2003 at 03:57:14PM -0700, Patrick Mochel wrote:
> Bah, sorry, I overlooked that. The patch below will add the device before
> calling bus_add_device(). Sorry about the confusion.
For the record, this patch solves the problem, thanks.
> ===== drivers/base/core.c 1.73 vs edited =====
> --- 1.73/drivers/base/core.c Fri Aug 15 10:27:01 2003
> +++ edited/drivers/base/core.c Wed Aug 27 15:49:08 2003
> @@ -225,28 +225,30 @@
> dev->kobj.parent = &parent->kobj;
>
> if ((error = kobject_add(&dev->kobj)))
> - goto register_done;
> -
> - /* now take care of our own registration */
> -
> + goto Error;
> + if ((error = device_pm_add(dev)))
> + goto PMError;
> + if ((error = bus_add_device(dev)))
> + goto BusError;
> down_write(&devices_subsys.rwsem);
> if (parent)
> list_add_tail(&dev->node,&parent->children);
> up_write(&devices_subsys.rwsem);
>
> - bus_add_device(dev);
> -
> - device_pm_add(dev);
> -
> /* notify platform of device entry */
> if (platform_notify)
> platform_notify(dev);
> -
> - register_done:
> - if (error && parent)
> - put_device(parent);
> + Done:
> put_device(dev);
> return error;
> + BusError:
> + device_pm_remove(dev);
> + PMError:
> + kobject_unregister(&dev->kobj);
> + Error:
> + if (parent)
> + put_device(parent);
> + goto Done;
> }
>
>
> @@ -312,8 +314,6 @@
> {
> struct device * parent = dev->parent;
>
> - device_pm_remove(dev);
> -
> down_write(&devices_subsys.rwsem);
> if (parent)
> list_del_init(&dev->node);
> @@ -324,14 +324,11 @@
> */
> if (platform_notify_remove)
> platform_notify_remove(dev);
> -
> bus_remove_device(dev);
> -
> + device_pm_remove(dev);
> kobject_del(&dev->kobj);
> -
> if (parent)
> put_device(parent);
> -
> }
>
> /**
>
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html