runtime PM ops and PCMCIA bus

[Dominik Brodowski]

Hey!

On the PCMCIA bus, implementing runtime PM "should" be simple.
However, there's a major caveat: the bus must be able to
force devices to suspend, or force them to resume during
runtime. Let's discuss this in detail:

(1) Converting the PCMCIA bus to dev_pm_ops seems easy:

    pcmcia_dev_suspend() needs to get rid of its second argument,
    then it's just a 

    SET_SYSTEM_SLEEP_PM_OPS(pcmcia_dev_suspend, pcmcia_dev_resume)


(2) Whenever two functions local to drivers/pcmcia/ds.c --
    runtime_suspend() or runtime_resume() -- are called,
    we need to force a synchronous call to pcmcia_dev_suspend(),
    or to pcmcia_dev_resume(). Currently, we do this by taking
    device_lock() and calling the function manually.


(3) However, it'd be much nicer to be able to use the new runtime
    PM interface. As the PCMCIA bus can always suspend,
    pcmcia_dev_idle() might only contain a call to

	pm_schedule_suspend(dev, 0) - or pm_runtime_suspend() ?

    To enable it, we should add

       pm_runtime_set_active(&p_dev->dev);
       pm_runtime_enable(&p_dev->dev);

    in pcmcia_device_add().


(4) What is then to be done to modify runtime_suspend() / runtime_resume() ?
    Is it:

+ static int runtime_suspend(struct device *dev)
+ {
+ 	pm_runtime_put_noidle(dev);
+ 	return pm_runtime_suspend(dev);
+ }
+ 
+ static int runtime_resume(struct device *dev)
+ {
+ 	return pm_runtime_get_sync(dev);
+ }


    or more something like


+  static int runtime_suspend(struct device *dev)
+ {
+ 	return pm_runtime_put_sync(dev);
+ }
+ 
+ static int runtime_resume(struct device *dev)
+ {
+ 	return pm_runtime_get_sync(dev);
+ }

    and modifying pcmcia_dev_idle() to call pm_runtime_suspend() instead of
    pm_schedule_suspend()?


(5) Another problem I'm seeing with the put/get approach: there are three
    different callpaths where runtime_suspend() / runtime_resume() might
    be called.[*] However, each one decreases the counter, which might get < 0.
    What to do? Using pm_runtime_suspended seems racy...


(6) As pcmcia_dev_resume() is used -- as per UNIVERSAL_DEV_PM_OPS -- what is
    needed there to set the runtime PM state correctly if we do not wake up
    the device during system sleep suspend?


Pseudo patch -- untested and probably broken [see at least (5) above]
attached. Any feedback is most welcome.

Best,
	Dominik

[*] 1) by echoing the state into a device sysfs file.
    2) by echoing the state into a socket sysfs file.
    3) during "resets" of the socket -- we need to suspend devices
           first, then physically reset the socket, then resume the devices
	   on the card.

diff --git a/drivers/pcmcia/Kconfig b/drivers/pcmcia/Kconfig
index caca50e..551fcdb 100644
--- a/drivers/pcmcia/Kconfig
+++ b/drivers/pcmcia/Kconfig
@@ -20,6 +20,7 @@ if PCCARD
 config PCMCIA
 	tristate "16-bit PCMCIA support"
 	select CRC32
+	select PM_RUNTIME
 	default y
 	---help---
 	   This option enables support for 16-bit PCMCIA cards. Most older
diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index f4df4d0..ec9f56e 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -24,6 +24,7 @@
 #include <linux/firmware.h>
 #include <linux/kref.h>
 #include <linux/dma-mapping.h>
+#include <linux/pm_runtime.h>
 
 #include <pcmcia/cs_types.h>
 #include <pcmcia/cs.h>
@@ -560,6 +561,9 @@ struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s, unsigned int fu
 	if (device_register(&p_dev->dev))
 		goto err_unreg;
 
+	pm_runtime_set_active(&p_dev->dev);
+	pm_runtime_enable(&p_dev->dev);
+
 	return p_dev;
 
  err_unreg:
@@ -952,28 +956,24 @@ static int pcmcia_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 #endif
 
 /************************ runtime PM support ***************************/
-
-static int pcmcia_dev_suspend(struct device *dev, pm_message_t state);
-static int pcmcia_dev_resume(struct device *dev);
-
 static int runtime_suspend(struct device *dev)
 {
-	int rc;
+	int ret;
 
-	device_lock(dev);
-	rc = pcmcia_dev_suspend(dev, PMSG_SUSPEND);
-	device_unlock(dev);
-	return rc;
+	dev_dbg(dev, "runtime_suspend\n");
+	ret = pm_runtime_put_sync(dev);
+	dev_dbg(dev, "runtime_suspend %d\n", ret);
+	return (ret >=0 ? 0 : ret);
 }
 
 static int runtime_resume(struct device *dev)
 {
-	int rc;
+	int ret;
 
-	device_lock(dev);
-	rc = pcmcia_dev_resume(dev);
-	device_unlock(dev);
-	return rc;
+	dev_dbg(dev, "runtime_resume\n");
+	ret = pm_runtime_get_sync(dev);
+	dev_dbg(dev, "runtime_resume %d\n", ret);
+	return (ret >=0 ? 0 : ret);
 }
 
 /************************ per-device sysfs output ***************************/
@@ -1085,7 +1085,7 @@ static struct device_attribute pcmcia_dev_attrs[] = {
 
 /* PM support, also needed for reset */
 
-static int pcmcia_dev_suspend(struct device *dev, pm_message_t state)
+static int pcmcia_dev_suspend(struct device *dev)
 {
 	struct pcmcia_device *p_dev = to_pcmcia_dev(dev);
 	struct pcmcia_driver *p_drv = NULL;
@@ -1406,6 +1406,19 @@ static struct class_interface pcmcia_bus_interface __refdata = {
 };
 
 
+static int pcmcia_dev_idle(struct device *dev)
+{
+	int ret = pm_runtime_suspend(dev);
+
+	/* we can always suspend */
+	dev_dbg(dev, "idle callback -- pm_runtime_suspend returned %d\n", ret);
+
+	return ret;
+}
+
+UNIVERSAL_DEV_PM_OPS(pcmcia_bus_pm_ops, 
+		pcmcia_dev_suspend, pcmcia_dev_resume, pcmcia_dev_idle);
+
 struct bus_type pcmcia_bus_type = {
 	.name = "pcmcia",
 	.uevent = pcmcia_bus_uevent,
@@ -1413,8 +1426,7 @@ struct bus_type pcmcia_bus_type = {
 	.dev_attrs = pcmcia_dev_attrs,
 	.probe = pcmcia_device_probe,
 	.remove = pcmcia_device_remove,
-	.suspend = pcmcia_dev_suspend,
-	.resume = pcmcia_dev_resume,
+	.pm = &pcmcia_bus_pm_ops,
 };