Re: [linux-usb-devel] [PATCH]switching off autosuspend through sysfs

Re: [linux-usb-devel] [PATCH]switching off autosuspend through sysfs

[Alan Stern]

On Tue, 23 Jan 2007, Greg KH wrote:

> On Tue, Jan 23, 2007 at 04:22:43PM -0800, David Brownell wrote:
> > > > >  static struct attribute *dev_attrs[] = {
> > > > > +	/* power management attributes */
> > > > > +	&dev_attr_autosuspend.attr,
> > > > 
> > > > Belongs in /sys/devices/.../power/... then, right?
> > >
> > > No, I thought we want to drop that power/ directory.
> > 
> > Dropping that directory hasn't AFAIK ever been discussed.
> > If it were to be dropped, where would the per-device wakeup
> > flags live?
> 
> I don't know, it just really annoys me to see that power directory there
> with no use for it for a lot of devices :)

Would it help to add a flag somewhere in struct device (or struct
dev_pm_info) for indicating that the device is not cognizant of PM?  For
instance, all those USB endpoint pseudo-devices we create -- it's a waste
of time to try doing power management on them and it generates a bunch of
useless and distracting warning messages in the system log.

> > >	I have no
> > > objection to it going into the USB device directory, as that is the
> > > proper place for it.
> > 
> > It'd be OK for a USB-specific mechanism, but there was at
> > one point some discussion about this mechanism being more
> > generally appropriate.
> 
> Hm, I missed that.  This seems to be a very usb-specific thing at the
> moment.

It is USB-specific because I wanted to start out in a well-defined 
environment where the mechanisms could get tested and shaken out.  When 
the problems are gone, it will make sense to think about moving the whole 
autosuspend infrastructure into the PM core.

>  And an annoying one too for some of my devices...powering off
> the bus for USB devices with no drivers might start causing devices that
> use usbfs to do wierd things.  Still trying to work it all out though...

If it does then we need to know and find a way to deal with it.

Alan Stern

Re: [PATCH]switching off autosuspend through sysfs

[Greg KH]

On Wed, Jan 24, 2007 at 11:51:17AM -0500, Alan Stern wrote:
> On Tue, 23 Jan 2007, Greg KH wrote:
> 
> > On Tue, Jan 23, 2007 at 04:22:43PM -0800, David Brownell wrote:
> > > > > >  static struct attribute *dev_attrs[] = {
> > > > > > +	/* power management attributes */
> > > > > > +	&dev_attr_autosuspend.attr,
> > > > > 
> > > > > Belongs in /sys/devices/.../power/... then, right?
> > > >
> > > > No, I thought we want to drop that power/ directory.
> > > 
> > > Dropping that directory hasn't AFAIK ever been discussed.
> > > If it were to be dropped, where would the per-device wakeup
> > > flags live?
> > 
> > I don't know, it just really annoys me to see that power directory there
> > with no use for it for a lot of devices :)
> 
> Would it help to add a flag somewhere in struct device (or struct
> dev_pm_info) for indicating that the device is not cognizant of PM?  For
> instance, all those USB endpoint pseudo-devices we create -- it's a waste
> of time to try doing power management on them and it generates a bunch of
> useless and distracting warning messages in the system log.

Yes.  In fact we should just make it a "has pm" type flag, as the
majority of devices do not.

So, what kind of devices do support these files?  I can think of:
	PCI
	USB
and that's it right now.  Do platform devices really use those files?

thanks,

greg k-h

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [PATCH]switching off autosuspend through sysfs

[Greg KH]

On Wed, Jan 24, 2007 at 05:15:24PM -0800, Greg KH wrote:
> On Wed, Jan 24, 2007 at 11:51:17AM -0500, Alan Stern wrote:
> > On Tue, 23 Jan 2007, Greg KH wrote:
> > 
> > > On Tue, Jan 23, 2007 at 04:22:43PM -0800, David Brownell wrote:
> > > > > > >  static struct attribute *dev_attrs[] = {
> > > > > > > +	/* power management attributes */
> > > > > > > +	&dev_attr_autosuspend.attr,
> > > > > > 
> > > > > > Belongs in /sys/devices/.../power/... then, right?
> > > > >
> > > > > No, I thought we want to drop that power/ directory.
> > > > 
> > > > Dropping that directory hasn't AFAIK ever been discussed.
> > > > If it were to be dropped, where would the per-device wakeup
> > > > flags live?
> > > 
> > > I don't know, it just really annoys me to see that power directory there
> > > with no use for it for a lot of devices :)
> > 
> > Would it help to add a flag somewhere in struct device (or struct
> > dev_pm_info) for indicating that the device is not cognizant of PM?  For
> > instance, all those USB endpoint pseudo-devices we create -- it's a waste
> > of time to try doing power management on them and it generates a bunch of
> > useless and distracting warning messages in the system log.
> 
> Yes.  In fact we should just make it a "has pm" type flag, as the
> majority of devices do not.
> 
> So, what kind of devices do support these files?  I can think of:
> 	PCI
> 	USB
> and that's it right now.  Do platform devices really use those files?

Something as simple as this patch perhaps?

thanks,

greg k-h

---
 drivers/base/power/sysfs.c |    7 +++++--
 drivers/pci/probe.c        |    1 +
 include/linux/device.h     |    1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

--- gregkh-2.6.orig/drivers/base/power/sysfs.c
+++ gregkh-2.6/drivers/base/power/sysfs.c
@@ -156,10 +156,13 @@ static struct attribute_group pm_attr_gr
 
 int dpm_sysfs_add(struct device * dev)
 {
-	return sysfs_create_group(&dev->kobj, &pm_attr_group);
+	if (dev->supports_power)
+		return sysfs_create_group(&dev->kobj, &pm_attr_group);
+	return 0;
 }
 
 void dpm_sysfs_remove(struct device * dev)
 {
-	sysfs_remove_group(&dev->kobj, &pm_attr_group);
+	if (dev->supports_power)
+		sysfs_remove_group(&dev->kobj, &pm_attr_group);
 }
--- gregkh-2.6.orig/drivers/pci/probe.c
+++ gregkh-2.6/drivers/pci/probe.c
@@ -893,6 +893,7 @@ pci_scan_device(struct pci_bus *bus, int
 	dev->sysdata = bus->sysdata;
 	dev->dev.parent = bus->bridge;
 	dev->dev.bus = &pci_bus_type;
+	dev->dev.supports_power = 1;
 	dev->devfn = devfn;
 	dev->hdr_type = hdr_type & 0x7f;
 	dev->multifunction = !!(hdr_type & 0x80);
--- gregkh-2.6.orig/include/linux/device.h
+++ gregkh-2.6/include/linux/device.h
@@ -365,6 +365,7 @@ struct device {
 	char	bus_id[BUS_ID_SIZE];	/* position on parent bus */
 	struct device_type	*type;
 	unsigned		is_registered:1;
+	unsigned		supports_power:1;
 	struct device_attribute uevent_attr;
 	struct device_attribute *devt_attr;
 

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [PATCH]switching off autosuspend through sysfs

[Alan Stern]

On Wed, 24 Jan 2007, Greg KH wrote:

> > > Would it help to add a flag somewhere in struct device (or struct
> > > dev_pm_info) for indicating that the device is not cognizant of PM?  For
> > > instance, all those USB endpoint pseudo-devices we create -- it's a waste
> > > of time to try doing power management on them and it generates a bunch of
> > > useless and distracting warning messages in the system log.
> > 
> > Yes.  In fact we should just make it a "has pm" type flag, as the
> > majority of devices do not.
> > 
> > So, what kind of devices do support these files?  I can think of:
> > 	PCI
> > 	USB
> > and that's it right now.  Do platform devices really use those files?
> 
> Something as simple as this patch perhaps?

I had something bigger in mind.  More like the patch below (completely 
untested).  Plus of course code to turn off the new flag for the 
appropriate devices.

Alan Stern


Index: usb-2.6/drivers/base/power/sysfs.c
===================================================================
--- usb-2.6.orig/drivers/base/power/sysfs.c
+++ usb-2.6/drivers/base/power/sysfs.c
@@ -152,10 +152,13 @@ static struct attribute_group pm_attr_gr
 
 int dpm_sysfs_add(struct device * dev)
 {
-	return sysfs_create_group(&dev->kobj, &pm_attr_group);
+	if (dev->supports_pm)
+		return sysfs_create_group(&dev->kobj, &pm_attr_group);
+	return 0;
 }
 
 void dpm_sysfs_remove(struct device * dev)
 {
-	sysfs_remove_group(&dev->kobj, &pm_attr_group);
+	if (dev->supports_pm)
+		sysfs_remove_group(&dev->kobj, &pm_attr_group);
 }
Index: usb-2.6/drivers/pci/probe.c
===================================================================
--- usb-2.6.orig/drivers/pci/probe.c
+++ usb-2.6/drivers/pci/probe.c
@@ -893,6 +893,7 @@ pci_scan_device(struct pci_bus *bus, int
 	dev->sysdata = bus->sysdata;
 	dev->dev.parent = bus->bridge;
 	dev->dev.bus = &pci_bus_type;
+	dev->dev.supports_pm = 1;
 	dev->devfn = devfn;
 	dev->hdr_type = hdr_type & 0x7f;
 	dev->multifunction = !!(hdr_type & 0x80);
Index: usb-2.6/include/linux/device.h
===================================================================
--- usb-2.6.orig/include/linux/device.h
+++ usb-2.6/include/linux/device.h
@@ -367,6 +367,7 @@ struct device {
 	char	bus_id[BUS_ID_SIZE];	/* position on parent bus */
 	struct device_type	*type;
 	unsigned		is_registered:1;
+	unsigned		supports_pm:1;
 	struct device_attribute uevent_attr;
 	struct device_attribute *devt_attr;
 
Index: usb-2.6/drivers/base/power/resume.c
===================================================================
--- usb-2.6.orig/drivers/base/power/resume.c
+++ usb-2.6/drivers/base/power/resume.c
@@ -24,6 +24,9 @@ int resume_device(struct device * dev)
 {
 	int error = 0;
 
+	if (!dev->supports_pm)
+		return error;
+
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 	down(&dev->sem);
@@ -52,6 +55,9 @@ static int resume_device_early(struct de
 {
 	int error = 0;
 
+	if (!dev->supports_pm)
+		return error;
+
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 	if (dev->bus && dev->bus->resume_early) {
Index: usb-2.6/drivers/base/power/suspend.c
===================================================================
--- usb-2.6.orig/drivers/base/power/suspend.c
+++ usb-2.6/drivers/base/power/suspend.c
@@ -50,6 +50,9 @@ int suspend_device(struct device * dev, 
 {
 	int error = 0;
 
+	if (!dev->supports_pm)
+		return error;
+
 	down(&dev->sem);
 	if (dev->power.power_state.event) {
 		dev_dbg(dev, "PM: suspend %d-->%d\n",
@@ -103,7 +106,8 @@ static int suspend_device_late(struct de
 {
 	int error = 0;
 
-	if (dev->bus && dev->bus->suspend_late && !dev->power.power_state.event) {
+	if (dev->supports_pm && dev->bus && dev->bus->suspend_late &&
+			!dev->power.power_state.event) {
 		dev_dbg(dev, "LATE %s%s\n",
 			suspend_verb(state.event),
 			((state.event == PM_EVENT_SUSPEND)


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [PATCH]switching off autosuspend through sysfs

[David Brownell]

> > > I don't know, it just really annoys me to see that power directory there
> > > with no use for it for a lot of devices :)
> > 
> > Would it help to add a flag somewhere in struct device (or struct
> > dev_pm_info) for indicating that the device is not cognizant of PM? 

If it would, the semantics would need to be clarified.  Is the problem
that they can't suspend/resume at all?  Or is the problem instead that
they (or their drivers) don't know how to do that without any loss of
functionality ... without a complete reset or whatever?

Most devices can -- if poked and prodded appropriately, and if no
BIOS or uploaded firmware interferes badly enough -- make it through
some kind of system suspend state.  Whether that's the one the user
has chosen ... different issue!  And whether the driver knows how to
do that ... also a different issue!

But the specific problems we're seeing right now with the USB autosuspend
stuff relate to _runtime_ suspend.  I believe some of them are just bugs
in the stack; where better coddling of the devices would let them make it
through unscathed.  But some of them, likewise, probably _do_ have real
problems with real runtime suspend states (that don't map to "off").


> > For 
> > instance, all those USB endpoint pseudo-devices we create -- it's a waste
> > of time to try doing power management on them and it generates a bunch of
> > useless and distracting warning messages in the system log.
> 
> Yes.  In fact we should just make it a "has pm" type flag, as the
> majority of devices do not.
> 
> So, what kind of devices do support these files?  I can think of:
> 	PCI

Actually, PCI still doesn't because of strangeness in the init
sequencing on at least PPC ... I can forward the patch that makes
X86 init the wakeup flag from the PM attributes.

There is however also the issue that on x86, ACPI (in my own
experience) just basically doesn't work as an intermediary for
most wakeup events.  And after all these years, /proc/acpi/wakeup
STILL doesn't hook up to the relevant devices in any useful way.
Drivers have no idea if their devices are expected to support
wakeup events at all.  (That is, assuming ACPI supported it ...)


> 	USB
> and that's it right now.  Do platform devices really use those files?

Depends on the platform.  AT91 is pretty good about it; most of the
platform devices there support wakeup from at least one of the two
power states.   Some of the OMAP devices do, and PXA isn't far from
having at least the IRQ wake mechanisms working.  I've been working
on getting a lot of RTCs to act as wakeup event sources, since that's
one of the few device types that issues wakeup events on most systems.

There's a chicken/egg problem here too ... how to get to a critical
mass of drivers (and platforms) that handle wakeup events properly.

... When most platforms won't even go to _sleep_ properly.

It's actually been a lot *easier* in my experience to get that to
work on an embedded Linux platform than on an x86 PC.  That's because
despite the usual glitches in chips and specs, the vendors are mostly
accustomed to giving that sort of information out to folk implementing
a variety of operating systems.   Whereas on PCs, they basically only
give it out to BIOS vendors, who then generate buggy firmware and
buggy ACPI bytecode to deliver to otherwise happy penguins; naturally
that sort of third or fourth hand stuff works poorly.

- Dave




-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-pm] [PATCH]switching off autosuspend through sysfs

[David Brownell]

On Wednesday 24 January 2007 6:15 pm, David Brownell wrote:

> > So, what kind of devices do support these files?  I can think of:
> > 	PCI
> 
> Actually, PCI still doesn't because of strangeness in the init
> sequencing on at least PPC ... I can forward the patch that makes
> X86 init the wakeup flag from the PM attributes.

And here it is.  Last refreshed against 2.6.20-rc5; it still works
like a charm on x86.  (As it did since before that PPC patch...)
I have no idea whether this still makes trouble on PPC.

- Dave


=================	CUT HERE
This patch teaches "pci_dev" about the driver model wakeup support:

 - It marks devices as supporting wakeup when the PME# capability is
   listed in its PCI PM capability.

 - pci_enable_wake() refuses to enable wake if that's been disabled
   (e.g. through sysfs).

NOTE that PowerPC does PCI probing a bit differently ... which evidently
causes this to fail on that platform (and maybe others).

One issue is that the driver model "init + add == register" pattern isn't
used inside PCI ...  and the PCI probe change that made PowerPC happier
worsened the problem by making "add" do some "init" too.  Maybe PCI should
match the driver model a lot more closely and grow a "pci_dev_init()".


Index: g26/drivers/pci/probe.c
===================================================================
--- g26.orig/drivers/pci/probe.c	2007-01-20 05:09:21.000000000 -0800
+++ g26/drivers/pci/probe.c	2007-01-20 05:18:42.000000000 -0800
@@ -654,6 +654,7 @@ static void pci_read_irq(struct pci_dev 
 static int pci_setup_device(struct pci_dev * dev)
 {
 	u32 class;
+	u16 pm;
 
 	sprintf(pci_name(dev), "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
 		dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
@@ -682,6 +683,19 @@ static int pci_setup_device(struct pci_d
 		pci_read_config_word(dev, PCI_SUBSYSTEM_VENDOR_ID, &dev->subsystem_vendor);
 		pci_read_config_word(dev, PCI_SUBSYSTEM_ID, &dev->subsystem_device);
 
+		/* PCI PM capable devices may be able to issue PME# (wakeup) */
+		pm = pci_find_capability(dev, PCI_CAP_ID_PM);
+		if (pm) {
+			pci_read_config_word(dev, pm + PCI_PM_PMC, &pm);
+			if (pm & PCI_PM_CAP_PME_MASK)
+				device_init_wakeup(&dev->dev, 1);
+
+			/* REVISIT: if (pm & PCI_PM_CAP_PME_D3cold) then
+			 * pci pm spec 1.2, section 3.2.4 says we should
+			 * init PCI_PM_CTRL_PME_{STATUS,ENABLE} ...
+			 */
+		}
+
 		/*
 		 *	Do the ugly legacy mode stuff here rather than broken chip
 		 *	quirk code. Legacy mode ATA controllers have fixed
@@ -848,6 +862,7 @@ pci_scan_device(struct pci_bus *bus, int
 
 	dev->bus = bus;
 	dev->sysdata = bus->sysdata;
+	device_initialize(&dev->dev);
 	dev->dev.parent = bus->bridge;
 	dev->dev.bus = &pci_bus_type;
 	dev->devfn = devfn;
@@ -871,7 +886,6 @@ pci_scan_device(struct pci_bus *bus, int
 
 void __devinit pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
-	device_initialize(&dev->dev);
 	dev->dev.release = pci_release_dev;
 	pci_dev_get(dev);
 
Index: g26/drivers/pci/pci.c
===================================================================
--- g26.orig/drivers/pci/pci.c	2007-01-20 05:18:31.000000000 -0800
+++ g26/drivers/pci/pci.c	2007-01-20 05:18:42.000000000 -0800
@@ -817,6 +817,10 @@ int pci_enable_wake(struct pci_dev *dev,
 	if (!pm) 
 		return enable ? -EIO : 0; 
 
+	/* don't enable unless policy set through driver core allows it */
+	if (!device_may_wakeup(&dev->dev) && enable)
+		return -EROFS;
+
 	/* Check device's ability to generate PME# */
 	pci_read_config_word(dev,pm+PCI_PM_PMC,&value);
 



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH]switching off autosuspend through sysfs

[Alan Stern]

On Wed, 24 Jan 2007, David Brownell wrote:

> 
> > > > I don't know, it just really annoys me to see that power directory there
> > > > with no use for it for a lot of devices :)
> > > 
> > > Would it help to add a flag somewhere in struct device (or struct
> > > dev_pm_info) for indicating that the device is not cognizant of PM? 
> 
> If it would, the semantics would need to be clarified.  Is the problem
> that they can't suspend/resume at all?  Or is the problem instead that
> they (or their drivers) don't know how to do that without any loss of
> functionality ... without a complete reset or whatever?

The semantics I intended was that the flag should indicate the driver was 
completely ignorant of PM.  It has no suspend or resume methods and no way 
of enabling or disabling remote wakeup.  As is true, for example, of those 
USB endpoint pseudo-devices.

Maybe this means the flag belongs in the driver's structure instead of the
device's.  But the PM core tries to avoid looking at drivers...

> Most devices can -- if poked and prodded appropriately, and if no
> BIOS or uploaded firmware interferes badly enough -- make it through
> some kind of system suspend state.  Whether that's the one the user
> has chosen ... different issue!  And whether the driver knows how to
> do that ... also a different issue!
> 
> But the specific problems we're seeing right now with the USB autosuspend
> stuff relate to _runtime_ suspend.  I believe some of them are just bugs
> in the stack; where better coddling of the devices would let them make it
> through unscathed.  But some of them, likewise, probably _do_ have real
> problems with real runtime suspend states (that don't map to "off").

That would be a separate issue.  For now there's a (large?) set of devices
whose drivers don't know Power Management from a hole in the wall.

Alan Stern

Re: [PATCH]switching off autosuspend through sysfs

[Oliver Neukum]

Am Mittwoch, 24. Januar 2007 17:51 schrieb Alan Stern:
> > Hm, I missed that.  This seems to be a very usb-specific thing at the
> > moment.
> 
> It is USB-specific because I wanted to start out in a well-defined 
> environment where the mechanisms could get tested and shaken out.  When 
> the problems are gone, it will make sense to think about moving the whole 
> autosuspend infrastructure into the PM core.

USB has some advantages. There's only one level of suspend,
which is supposed to be mandatory. In addition, it's important for PM
because USB always means DMA.

	Regards
		Oliver

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel