public inbox for linux-pm@vger.kernel.org
 help / color / mirror / Atom feed
* Re: [patch] pm: fix runtime powermanagement's /sys interface
@ 2006-01-05 15:16 Scott E. Preece
  2006-01-05 16:41 ` [linux-pm] " Alan Stern
  2006-01-05 21:48 ` Patrick Mochel
  0 siblings, 2 replies; 22+ messages in thread
From: Scott E. Preece @ 2006-01-05 15:16 UTC (permalink / raw)
  To: pavel; +Cc: akpm, linux-pm, linux-kernel

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


My inclination would be to have the sysfs interface know generic terms,
with the implementation mapping them to device-specific terms. It ought
to be possible to build portable tools that don't have to know about
device-specific states and have the device interfaces (in sysfs) do the
necessary translation.

However, I also think there is value in having the sysfs interface
recognize the device-specific values as well, so that device-specific
tools can also be written (offering the option of taking advantage of
special capabilities of a particular device).

scott

| On St 04-01-06 17:06:09, Alan Stern wrote:
| > On Wed, 4 Jan 2006, Pavel Machek wrote:
| > 
| > > > As I mentioned in the thread (currently happening, BTW) on the linux-pm
| > > > list, what you want to do is accept a string that reflects an actual state
| > > > that the device supports. For PCI devices that support low-power states,
| > > > this would be "D1", "D2", "D3", etc. For USB devices, which only support
| > > > an "on" and "suspended" state, the values that this patch parses would
| > > > actually work.
| > > 
| > > We want _common_ values, anyway. So, we do not want "D0", "D1", "D2",
| > > "D3hot" in PCI cases. We probably want "on", "D1", "D2", "suspend",
| > > and I'm not sure about those "D1" and "D2" parts. Userspace should not
| > > have to know about details, it will mostly use "on"/"suspend" anyway.
| > 
| > It would be good to make the details available so that they are there when
| > needed.  For instance, we might export "D0", "on", "D1", "D2", "D3", and
| > "suspend", treating "on" as a synonym for "D0" and "suspend" as a synonym
| > for "D3".
| 
| Why to make it this complex?
| 
| I do not think there's any confusion possible. "on" always corresponds
| to "D0", and "suspend" is "D3". Anyone who knows what "D2" means,
| should know that, too...
| 							Pavel
| -- 
| Thanks, Sharp!
| 
| --===============85566774732936779==
| ----------
| _______________________________________________
| linux-pm mailing list
| linux-pm@lists.osdl.org
| https://lists.osdl.org/mailman/listinfo/linux-pm
| 
| --===============85566774732936779==--

-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com



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



^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [patch] pm: fix runtime powermanagement's /sys interface
@ 2006-01-06  1:37 Patrick Mochel
  2006-01-06 15:42 ` Alan Stern
  0 siblings, 1 reply; 22+ messages in thread
From: Patrick Mochel @ 2006-01-06  1:37 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Andrew Morton, Linux-pm mailing list, kernel list,
	Dominik Brodowski

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 6120 bytes --]



On Fri, 6 Jan 2006, Pavel Machek wrote:

> On Čt 05-01-06 16:04:07, Patrick Mochel wrote:

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

ACK, you beat me to it.

And, appended is a patch to export PM controls for PCI devices. The file
"pm_possible_states" exports the states a device supports, and "pm_state"
exports the current state (and provides the interface for entering a
state).

Eventually, some drivers will want to fix up those values so that it can
mask of states that it doesn't support, as well as offer possible device-
specific states.

What's interesting is that with this patch, I can see that two more
devices on my system support D1 and D2 -- the cardbus controllers, which
are actually bridges whose PM capabilities aren't exported via lspci.

Thanks,

	Patrick


diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 6707df9..628f3a3 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -36,6 +36,10 @@ obj-$(CONFIG_ACPI)    += pci-acpi.o
 # Cardbus & CompactPCI use setup-bus
 obj-$(CONFIG_HOTPLUG) += setup-bus.o

+
+# Power Management functionality
+obj-$(CONFIG_PM)	+= pm.o
+
 ifndef CONFIG_X86
 obj-y += syscall.o
 endif
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index eed67d9..83045d9 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -85,6 +85,8 @@ void __devinit pci_bus_add_device(struct
 	list_add_tail(&dev->global_list, &pci_devices);
 	spin_unlock(&pci_bus_lock);

+	pci_pm_init(dev);
+
 	pci_proc_attach_device(dev);
 	pci_create_sysfs_dev_files(dev);
 }
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6527b36..6d7afbc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -63,6 +63,23 @@ extern int pcie_mch_quirk;
 extern struct device_attribute pci_dev_attrs[];
 extern struct class_device_attribute class_device_attr_cpuaffinity;

+
+#ifdef CONFIG_PM
+extern int pci_pm_init(struct pci_dev *);
+extern void pci_pm_exit(struct pci_dev *);
+#else /* CONFIG_PM */
+static inline int pci_pm_init(struct pci_dev *)
+{
+	return 0;
+}
+
+static inline void pci_pm_exit(struct pci_dev *)
+{
+
+}
+
+#endif
+
 /**
  * pci_match_one_device - Tell if a PCI device structure has a matching
  *                        PCI device id structure
diff --git a/drivers/pci/pm.c b/drivers/pci/pm.c
new file mode 100644
index 0000000..ce476e4
--- /dev/null
+++ b/drivers/pci/pm.c
@@ -0,0 +1,138 @@
+/*
+ * drivers/pci/pm.c - Power management support for PCI devices
+ */
+
+#include <linux/pci.h>
+
+
+static ssize_t pm_possible_states_show(struct device * d,
+				       struct device_attribute * a,
+				       char * buffer)
+{
+	struct pci_dev * dev = to_pci_dev(d);
+	char * s = buffer;
+
+	s += sprintf(s, "d0");
+	if (dev->poss_states[PCI_D1])
+		s += sprintf(s, " d1");
+	if (dev->poss_states[PCI_D2])
+		s += sprintf(s, " d2");
+	if (dev->poss_states[PCI_D3hot])
+		s += sprintf(s, " d3");
+	s += sprintf(s, "\n");
+	return (s - buffer);
+}
+
+static DEVICE_ATTR(pm_possible_states, 0444, pm_possible_states_show, NULL);
+
+
+static ssize_t pm_state_show(struct device * d, struct device_attribute * a,
+			     char * buffer)
+{
+	struct pci_dev * dev = to_pci_dev(d);
+	const char * str;
+
+	switch (dev->current_state) {
+	case PCI_D0:
+		str = "d0";
+		break;
+	case PCI_D1:
+		str = "d1";
+		break;
+	case PCI_D2:
+		str = "d2";
+		break;
+	case PCI_D3hot:
+		str = "d3";
+		break;
+	default:
+		str = "d?";
+		break;
+	}
+
+	return sprintf(buffer, "%s\n", str);
+}
+
+
+static ssize_t pm_state_store(struct device * d, struct device_attribute * a,
+			      const char * buffer, size_t len)
+{
+	struct pci_dev * dev = to_pci_dev(d);
+	pci_power_t state;
+	int ret;
+
+	if (!strnicmp(buffer, "d0", len))
+		state = PCI_D0;
+	else if (!strnicmp(buffer, "d1", len))
+		state = PCI_D1;
+	else if (!strnicmp(buffer, "d2", len))
+		state = PCI_D2;
+	else if (!strnicmp(buffer, "d3", len))
+		state = PCI_D3hot;
+	else
+		return -EINVAL;
+
+	if (state == dev->current_state)
+		return 0;
+
+	if (dev->poss_states[state])
+		ret = pci_set_power_state(dev, state);
+	else
+		ret = -EINVAL;
+
+	return ret == 0 ? len : ret;
+}
+
+static DEVICE_ATTR(pm_state, 0644, pm_state_show, pm_state_store);
+
+
+static int find_states(struct pci_dev * dev)
+{
+	int cap;
+	u16 pmc;
+
+
+	/*
+	 * Every device supports D0
+	 */
+	dev->poss_states[PCI_D0] = 1;
+
+	/*
+	 * Check if the device has PM capabilties in the config space
+	 */
+	cap = pci_find_capability(dev, PCI_CAP_ID_PM);
+	if (!cap)
+		return -EIO;
+
+	/*
+	 * If it supports PM capabilities, it will support D3
+	 */
+	dev->poss_states[PCI_D3hot] = 1;
+
+	/*
+	 * Check D1 and D2 support
+	 */
+	pci_read_config_word(dev, cap + PCI_PM_PMC, &pmc);
+	if (pmc & PCI_PM_CAP_D1)
+		dev->poss_states[PCI_D1] = 1;
+	if (pmc & PCI_PM_CAP_D2)
+		dev->poss_states[PCI_D2] = 1;
+	return 0;
+}
+
+
+int pci_pm_init(struct pci_dev * dev)
+{
+	if (find_states(dev))
+		return 0;
+
+	device_create_file(&dev->dev, &dev_attr_pm_possible_states);
+	return device_create_file(&dev->dev, &dev_attr_pm_state);
+}
+
+void pci_pm_exit(struct pci_dev * dev)
+{
+	device_remove_file(&dev->dev, &dev_attr_pm_state);
+	device_remove_file(&dev->dev, &dev_attr_pm_possible_states);
+}
+
diff --git a/include/linux/pci.h b/include/linux/pci.h
index de690ca..2600119 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -106,6 +106,7 @@ struct pci_dev {
 					   this if your device has broken DMA
 					   or supports 64-bit transfers.  */

+	u32		poss_states[4];
 	pci_power_t     current_state;  /* Current operating state. In ACPI-speak,
 					   this is D0-D3, D0 being fully functional,
 					   and D3 being off. */


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



^ permalink raw reply related	[flat|nested] 22+ messages in thread
* RE: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface
@ 2006-01-05 22:13 Preece Scott-PREECE
  0 siblings, 0 replies; 22+ messages in thread
From: Preece Scott-PREECE @ 2006-01-05 22:13 UTC (permalink / raw)
  To: Patrick Mochel; +Cc: pavel, akpm, linux-pm, linux-kernel

User space has no particular reason to know which state of a particular
device corresponds to the logical state "on" or "off" or whatever other
states might be needed. Once you've defined the set of standard, generic
states, then the device driver writer can figure out which device state
matches the requirements for a given generic state.

While I wouldn't hate putting this in a system-level configuration file,
I really think it's device-specific stuff that should be built-in.

scott

-----Original Message-----
From: Patrick Mochel [mailto:mochel@digitalimplant.org] 
Sent: Thursday, January 05, 2006 3:48 PM
To: Preece Scott-PREECE
Cc: pavel@suse.cz; ; ; 
Subject: Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys
interface


On Thu, 5 Jan 2006, Scott E. Preece wrote:

> --===============26103097005026354==
>
>
> My inclination would be to have the sysfs interface know generic 
> terms, with the implementation mapping them to device-specific terms. 
> It ought to be possible to build portable tools that don't have to 
> know about device-specific states and have the device interfaces (in 
> sysfs) do the necessary translation.

Userspace should do the translation. You should give the user the
ability to specify simple, meaningful states, like "on" and "off". But,
it should be the tools itself that are mapping those requests to valid
input for the sysfs files.

Why force the translation into the kernel, and provide more
opportunities for error in parsing the sysfs files? Do it in userspace,
and you can afford much more flexibility and portability.

Thanks,


	Patrick

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [patch] pm: fix runtime powermanagement's /sys interface
  2006-01-04 22:06         ` Alan Stern
@ 2006-01-05 21:43 Patrick Mochel
  2006-01-05 22:06 ` [linux-pm] " Alan Stern
  0 siblings, 1 reply; 22+ messages in thread
From: Patrick Mochel @ 2006-01-05 21:43 UTC (permalink / raw)
  To: Alan Stern
  Cc: Andrew Morton, Linux-pm mailing list, kernel list, Pavel Machek

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1210 bytes --]


On Wed, 4 Jan 2006, Alan Stern wrote:

> On Wed, 4 Jan 2006, Pavel Machek wrote:
>
> > > As I mentioned in the thread (currently happening, BTW) on the linux-pm
> > > list, what you want to do is accept a string that reflects an actual state
> > > that the device supports. For PCI devices that support low-power states,
> > > this would be "D1", "D2", "D3", etc. For USB devices, which only support
> > > an "on" and "suspended" state, the values that this patch parses would
> > > actually work.
> >
> > We want _common_ values, anyway. So, we do not want "D0", "D1", "D2",
> > "D3hot" in PCI cases. We probably want "on", "D1", "D2", "suspend",
> > and I'm not sure about those "D1" and "D2" parts. Userspace should not
> > have to know about details, it will mostly use "on"/"suspend" anyway.
>
> It would be good to make the details available so that they are there when
> needed.  For instance, we might export "D0", "on", "D1", "D2", "D3", and
> "suspend", treating "on" as a synonym for "D0" and "suspend" as a synonym
> for "D3".

Do it in userspace; the kernel doesn't need to know about "on" or
"suspend". It should just validate and forward requests to enter specific
states.

Thanks,


	Patrick


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



^ permalink raw reply	[flat|nested] 22+ messages in thread
* [patch] pm: fix runtime powermanagement's /sys interface
@ 2005-12-27 21:34 Pavel Machek
  2005-12-27 21:55 ` Dmitry Torokhov
  0 siblings, 1 reply; 22+ messages in thread
From: Pavel Machek @ 2005-12-27 21:34 UTC (permalink / raw)
  To: Andrew Morton, kernel list, Linux-pm mailing list

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

/sys/devices/..../power interface is currently very broken. It takes
integer from user, and passes it to drivers as pm_message_t.event
... without even checking it. This changes the interface to pass
strings, and introduces checks.

Signed-off-by: Pavel Machek <pavel@suse.cz>

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
@@ -27,22 +27,25 @@
 
 static ssize_t state_show(struct device * dev, struct device_attribute *attr, char * buf)
 {
-	return sprintf(buf, "%u\n", dev->power.power_state.event);
+	if (dev->power.power_state.event)
+		return sprintf(buf, "suspend\n");
+	else
+		return sprintf(buf, "on\n");
 }
 
 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;
+	int error = -EINVAL;
 
-	state.event = simple_strtoul(buf, &rest, 10);
-	if (*rest)
-		return -EINVAL;
-	if (state.event)
-		error = dpm_runtime_suspend(dev, state);
-	else
+	state.event = PM_EVENT_SUSPEND;
+	if ((n == 2) && !strncmp(buf, "on", 2)) {
 		dpm_runtime_resume(dev);
+		error = 0;
+	}
+	if ((n == 7) && !strncmp(buf, "suspend", 7))
+		error = dpm_runtime_suspend(dev, state);
+
 	return error ? error : n;
 }
 

-- 
Thanks, Sharp!

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



^ permalink raw reply	[flat|nested] 22+ messages in thread
end of thread, other threads:[~2006-01-09 20:10 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-01-05 15:16 [patch] pm: fix runtime powermanagement's /sys interface Scott E. Preece
2006-01-05 16:41 ` [linux-pm] " Alan Stern
2006-01-05 21:14   ` Pavel Machek
2006-01-05 21:37     ` Alan Stern
2006-01-05 21:44       ` Pavel Machek
2006-01-05 21:48 ` Patrick Mochel
  -- strict thread matches above, loose matches on Subject: below --
2006-01-06  1:37 Patrick Mochel
2006-01-06 15:42 ` Alan Stern
2006-01-07  7:41   ` [linux-pm] " Adam Belay
2006-01-05 22:13 Preece Scott-PREECE
2006-01-05 21:43 Patrick Mochel
2006-01-05 22:06 ` [linux-pm] " Alan Stern
2005-12-27 21:34 Pavel Machek
2005-12-27 21:55 ` Dmitry Torokhov
2005-12-27 22:05   ` Pavel Machek
2005-12-28  4:22     ` Patrick Mochel
2006-01-04 21:34       ` [linux-pm] " Pavel Machek
2006-01-04 22:06         ` Alan Stern
2006-01-04 22:16           ` [linux-pm] " Pavel Machek
2006-01-05 21:42         ` Patrick Mochel
2006-01-05 21:55           ` Pavel Machek
2006-01-05 22:13             ` [linux-pm] " Dominik Brodowski
2006-01-05 22:23               ` Pavel Machek
2006-01-05 22:27                 ` Dominik Brodowski
2006-01-05 22:59                   ` Pavel Machek
2006-01-05 23:08                   ` Pavel Machek
2006-01-05 23:46                     ` [linux-pm] " Dominik Brodowski
2006-01-06  0:38                   ` Greg KH
2006-01-06 15:03                     ` Dominik Brodowski
2006-01-06 16:25                       ` Kay Sievers
2006-01-09 20:10                         ` [linux-pm] " Dominik Brodowski
2006-01-05 22:15             ` Patrick Mochel
2006-01-05 22:44               ` Pavel Machek
2006-01-05 23:54                 ` Patrick Mochel
2006-01-06  0:07                   ` [linux-pm] " Pavel Machek
2006-01-06 14:34             ` Tom Marshall
2006-01-06 16:20               ` Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox