PCI PM & compatibility

What about this patch to stay compatible with existing drivers implementing everything in save_state ?

Quoted 2 lines · Benjamin Herrenschmidt

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???

Quoted 5 lines · Russell King

Quoted 2 lines · Benjamin Herrenschmidt

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...

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_ !

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 6 lines · Patrick Mochel

Quoted 2 lines

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.

===== drivers/base/core.c 1.73 vs edited =====

drivers/base/core.c +15 -18

@@ -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.

Quoted 9 lines

- 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.