Re: [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]

On Čt 05-01-06 16:04:07, Patrick Mochel wrote:
> 
> On Fri, 6 Jan 2006, Pavel Machek wrote:
> 
> > On Pá 06-01-06 00:46:29, Dominik Brodowski wrote:
> > > On Fri, Jan 06, 2006 at 12:08:49AM +0100, Pavel Machek wrote:
> > > > Ok, so lets at least add value-checking to .../power file, and prevent
> > > > userspace see changes to PM_EVENT_SUSPEND value. 2 and 0 are now
> > > > "arbitrary cookies". I'd like to use "on" and "off", but pcmcia
> > > > apparently depends on "2" and "0", so...
> > > >
> > > > Any objections?
> > >
> > > Sorry, but yes -- the same as before, minus the PCMCIA breakage.
> >
> > I don't understand at this point.
> >
> > Current code takes value from user, and passes it down to driver,
> > without any checking. If user writes 666 into the file, it will
> > happily pass down 666 to the driver. Driver does not expect 666 in
> > pm_message_t.event. It may oops, BUG_ON() or anything like that.
> >
> > Shall I change
> >
> > #define PM_EVENT_SUSPEND 2
> >
> > to
> >
> > #define PM_EVENT_SUSPEND 1324
> >
> > to get my point across? This is kernel-specific value, it should not
> > be exported to userland.
> 
> A better point, and one that would actually be useful, would be to remove
> the file altogether. Let Dominik export a power file, with complete
> control over the values, for each pcmcia device. Then you never have to
> worry about breaking PCMCIA again.

Fine with me.

[except that now you we went from supporting 2 runtime device states
to supporting just 1... I don't think this is much of progress...]

diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -6,49 +6,6 @@
 #include <linux/string.h>
 #include "power.h"
 
-
-/**
- *	state - Control current power state of device
- *
- *	show() returns the current power state of the device. '0' indicates
- *	the device is on. Other values (1-3) indicate the device is in a low
- *	power state.
- *
- *	store() sets the current power state, which is an integer value
- *	between 0-3. If the device is on ('0'), and the value written is
- *	greater than 0, then the device is placed directly into the low-power
- *	state (via its driver's ->suspend() method).
- *	If the device is currently in a low-power state, and the value is 0,
- *	the device is powered back on (via the ->resume() method).
- *	If the device is in a low-power state, and a different low-power state
- *	is requested, the device is first resumed, then suspended into the new
- *	low-power state.
- */
-
-static ssize_t state_show(struct device * dev, struct device_attribute *attr, char * buf)
-{
-	return sprintf(buf, "%u\n", dev->power.power_state.event);
-}
-
-static ssize_t state_store(struct device * dev, struct device_attribute *attr, const char * buf, size_t n)
-{
-	pm_message_t state;
-	char * rest;
-	int error = 0;
-
-	state.event = simple_strtoul(buf, &rest, 10);
-	if (*rest)
-		return -EINVAL;
-	if (state.event)
-		error = dpm_runtime_suspend(dev, state);
-	else
-		dpm_runtime_resume(dev);
-	return error ? error : n;
-}
-
-static DEVICE_ATTR(state, 0644, state_show, state_store);
-
-
 /*
  *	wakeup - Report/change current wakeup option for device
  *
@@ -122,7 +79,6 @@ static DEVICE_ATTR(wakeup, 0644, wake_sh
 
 
 static struct attribute * power_attrs[] = {
-	&dev_attr_state.attr,
 	&dev_attr_wakeup.attr,
 	NULL,
 };

-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]