[patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

This updates /proc/acpi/wakeup to be more informative, primarily by showing
the sysfs node associated with each wakeup-enabled device.  Example:

	Device	S-state	  Status   Sysfs node
	PCI0	  S4	 disabled  no-bus:pci0000:00
	PS2M	  S4	 disabled  pnp:00:05
	PS2K	  S4	 disabled  pnp:00:06
	UAR1	  S4	 disabled  pnp:00:08
	USB1	  S3	 disabled  pci:0000:00:03.0
	USB2	  S3	 disabled  pci:0000:00:03.1
	USB3	  S3	 disabled  
	USB4	  S3	 disabled  pci:0000:00:03.3
	S139	  S4	 disabled  
	LAN	  S4	 disabled  pci:0000:00:04.0
	MDM	  S4	 disabled  
	AUD	  S4	 disabled  pci:0000:00:02.7
	SLPB	  S4	*enabled   

Eventually this file should be removed, but until then it's almost the only
way we have to tell how the relevant ACPI tables are broken (and cope).  In
that example, two devices don't actually exist (USB3, S139), one can't issue
wakeup events (PCI0), and two seem harmlessly (?) confused (MDM and AUD are
the same PCI device, but it's the _modem_ that does wake-on-ring).  

In particular, we need to be sure driver model nodes are properly hooked
up before we can get rid of this ACPI-only interface for wakeup events.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

--- g26.orig/drivers/acpi/sleep/proc.c	2007-04-03 16:27:22.000000000 -0700
+++ g26/drivers/acpi/sleep/proc.c	2007-04-03 16:43:26.000000000 -0700
@@ -360,21 +360,31 @@ acpi_system_wakeup_device_seq_show(struc
 {
 	struct list_head *node, *next;
 
-	seq_printf(seq, "Device	Sleep state	Status\n");
+	seq_printf(seq, "Device\tS-state\t  Status   Sysfs node\n");
 
 	spin_lock(&acpi_device_lock);
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev =
 		    container_of(node, struct acpi_device, wakeup_list);
+		struct device *ldev;
 
 		if (!dev->wakeup.flags.valid)
 			continue;
 		spin_unlock(&acpi_device_lock);
-		seq_printf(seq, "%4s	%4d		%s%8s\n",
+
+		ldev = acpi_get_physical_device(dev->handle);
+		seq_printf(seq, "%s\t  S%d\t%c%-8s  ",
 			   dev->pnp.bus_id,
 			   (u32) dev->wakeup.sleep_state,
-			   dev->wakeup.flags.run_wake ? "*" : "",
+			   dev->wakeup.flags.run_wake ? '*' : ' ',
 			   dev->wakeup.state.enabled ? "enabled" : "disabled");
+		if (ldev)
+			seq_printf(seq, "%s:%s",
+				ldev->bus ? ldev->bus->name : "no-bus",
+				ldev->bus_id);
+		seq_printf(seq, "\n");
+		put_device(ldev);
+
 		spin_lock(&acpi_device_lock);
 	}
 	spin_unlock(&acpi_device_lock);

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[Zhang Rui]

On Wed, 2007-04-04 at 08:41 +0800, David Brownell wrote:
> This updates /proc/acpi/wakeup to be more informative, primarily by
> showing
> the sysfs node associated with each wakeup-enabled device.  Example:
> 
>         Device  S-state   Status   Sysfs node
>         PCI0      S4     disabled  no-bus:pci0000:00
>         PS2M      S4     disabled  pnp:00:05
>         PS2K      S4     disabled  pnp:00:06
>         UAR1      S4     disabled  pnp:00:08
>         USB1      S3     disabled  pci:0000:00:03.0
>         USB2      S3     disabled  pci:0000:00:03.1
>         USB3      S3     disabled 
>         USB4      S3     disabled  pci:0000:00:03.3
>         S139      S4     disabled 
>         LAN       S4     disabled  pci:0000:00:04.0
>         MDM       S4     disabled 
>         AUD       S4     disabled  pci:0000:00:02.7
>         SLPB      S4    *enabled  
> 
> Eventually this file should be removed, but until then it's almost the
> only
> way we have to tell how the relevant ACPI tables are broken (and
> cope).  In
> that example, two devices don't actually exist (USB3, S139), one can't
> issue
> wakeup events (PCI0), and two seem harmlessly (?) confused (MDM and
> AUD are
> the same PCI device, but it's the _modem_ that does wake-on-ring). 
> 
Well, ACPI can't find the sysfs node for all the wakeup-enabled devices.
In fact, only pci and pnp devices can be found now.
ACPI needs the ability to distinguish all the physical devices, i.e. map
ACPI device to physical device nodes in sysfs, which I mentioned before.

Thanks,
Rui

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Thursday 05 April 2007 12:59 am, Zhang Rui wrote:
> On Wed, 2007-04-04 at 08:41 +0800, David Brownell wrote:
> > In that example, two devices don't actually exist (USB3, S139), one can't
> > issue wakeup events (PCI0), and two seem harmlessly (?) confused (MDM and
> > AUD are the same PCI device, but it's the _modem_ that does wake-on-ring). 
> > 
> Well, ACPI can't find the sysfs node for all the wakeup-enabled devices.

The only example of that type in the example I provided is SLPB, where the
sysfs node is ACPI-internal; I'm not sure how one can detect such cases.


> In fact, only pci and pnp devices can be found now.

Well, PNP courtesy of a previous patch from me, but remember that I was
using that as an example of a ** BOGUS ACPI TABLE ** as summarized above.
In my observation, such bogus tables are common.  (This particular one
was, I believe, copied from a system using a higher end version of the
same southbridge ... but which may well have had its own errors.)


> ACPI needs the ability to distinguish all the physical devices, i.e. map
> ACPI device to physical device nodes in sysfs, which I mentioned before.

For wakeup devices, the main issue I've seen is with button devices.
In my limited set of test sytems, everything else is either PCI, PNP,
or a bug (listing a non-existent device).

If this patch starts to get deployed, I expect other people will find
a few other curiousities ... and likely some things to be fixed.

The /sys/devices/acpi_system:00/ tree is kind of new.  I suspect one
way it could be more informative is to set up cross-links in sysfs
between the ACPI devices and the "real" device nodes ... e.g. on the
system I'm using right now .../device:00/PNP0A03:00/device:15/PNP0B00:00
could have a link pointing to /sys/devices/pnp0/00:06 ... and that PNP
node in turn could have an "acpi" link pointing back to the ACPI thing.

Such cross-links would let people see those relationships, and observe
which links are missing or otherwise strange.  Fixing the bugs would
seem unlikely until those things become visible.

- Dave

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[Zhang Rui]

On Thu, 2007-04-05 at 03:58 -0700, David Brownell wrote: 
> On Thursday 05 April 2007 12:59 am, Zhang Rui wrote:
> > On Wed, 2007-04-04 at 08:41 +0800, David Brownell wrote:
> > > In that example, two devices don't actually exist (USB3, S139), one can't
> > > issue wakeup events (PCI0), and two seem harmlessly (?) confused (MDM and
> > > AUD are the same PCI device, but it's the _modem_ that does wake-on-ring). 
> > > 
> > Well, ACPI can't find the sysfs node for all the wakeup-enabled devices.
> 
> The only example of that type in the example I provided is SLPB, where the
> sysfs node is ACPI-internal; I'm not sure how one can detect such cases.
> 
> 
> > In fact, only pci and pnp devices can be found now.
> 
> Well, PNP courtesy of a previous patch from me, but remember that I was
> using that as an example of a ** BOGUS ACPI TABLE ** as summarized above.
Then it's OK. :)
> 
> > ACPI needs the ability to distinguish all the physical devices, i.e. map
> > ACPI device to physical device nodes in sysfs, which I mentioned before.
> 
> For wakeup devices, the main issue I've seen is with button devices.
> In my limited set of test sytems, everything else is either PCI, PNP,
> or a bug (listing a non-existent device).
There may be other wakeup-enabled devices like legacy serial,
PS2 devices described in ACPI name space. 
> If this patch starts to get deployed, I expect other people will find
> a few other curiousities ... and likely some things to be fixed.

> The /sys/devices/acpi_system:00/ tree is kind of new.  I suspect one
> way it could be more informative is to set up cross-links in sysfs
> between the ACPI devices and the "real" device nodes ... e.g. on the
> system I'm using right now .../device:00/PNP0A03:00/device:15/PNP0B00:00
> could have a link pointing to /sys/devices/pnp0/00:06 ... and that PNP
> node in turn could have an "acpi" link pointing back to the ACPI thing.
> 
> Such cross-links would let people see those relationships, and observe
> which links are missing or otherwise strange.  Fixing the bugs would
> seem unlikely until those things become visible.
Sounds nice.
The patch below should make sense.

Add cross-links between ACPI device and "real" devices in sysfs.

Signed-off-by: Zhang Rui <rui.zhang@intel.com> 
---
 drivers/acpi/glue.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+)

Index: linux-2.6.21-rc5/drivers/acpi/glue.c
===================================================================
--- linux-2.6.21-rc5.orig/drivers/acpi/glue.c	2007-04-06 16:06:04.000000000 +0800
+++ linux-2.6.21-rc5/drivers/acpi/glue.c	2007-04-06 17:32:40.000000000 +0800
@@ -147,6 +147,7 @@ EXPORT_SYMBOL(acpi_get_physical_device);
 static int acpi_bind_one(struct device *dev, acpi_handle handle)
 {
 	acpi_status status;
+	struct acpi_device *acpi_dev;
 
 	if (dev->archdata.acpi_handle) {
 		printk(KERN_WARNING PREFIX
@@ -161,16 +162,29 @@ static int acpi_bind_one(struct device *
 	}
 	dev->archdata.acpi_handle = handle;
 
+	if (!acpi_bus_get_device(handle, &acpi_dev)) {
+		sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj, "ACPI_node");
+		sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj, "physical_node");
+	}
+
 	return 0;
 }
 
 static int acpi_unbind_one(struct device *dev)
 {
+	struct acpi_device *acpi_dev;
+
 	if (!dev->archdata.acpi_handle)
 		return 0;
 	if (dev == acpi_get_physical_device(dev->archdata.acpi_handle)) {
 		/* acpi_get_physical_device increase refcnt by one */
 		put_device(dev);
+
+		if (!acpi_bus_get_device(dev->archdata.acpi_handle, &acpi_dev)) {
+			sysfs_remove_link(&dev->kobj, "ACPI_node");
+			sysfs_remove_link(&acpi_dev->dev.kobj, "physical_node");
+		}
+
 		acpi_detach_data(dev->archdata.acpi_handle,
 				 acpi_glue_data_handler);
 		dev->archdata.acpi_handle = NULL;

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Friday 06 April 2007 2:36 am, Zhang Rui wrote:
> On Thu, 2007-04-05 at 03:58 -0700, David Brownell wrote: 
> > 
> > For wakeup devices, the main issue I've seen is with button devices.
> > In my limited set of test sytems, everything else is either PCI, PNP,
> > or a bug (listing a non-existent device).
>
> There may be other wakeup-enabled devices like legacy serial,
> PS2 devices described in ACPI name space.

Do you mean they wouldn't be listed in /proc/acpi/wakeup though?

Or just that they wouldn't be listed as PNP devices (like the button
devices)?  That seems buglike.  Not that BIOS bugs are so rare; and
I sure wouldn't know which types are common...

Because that example file certainly included UART and PS2 devices,
which were listed both as PNP devices and as ACPI devices.


> > If this patch starts to get deployed, I expect other people will find
> > a few other curiousities ... and likely some things to be fixed.
> 
> > The /sys/devices/acpi_system:00/ tree is kind of new.  I suspect one
> > way it could be more informative is to set up cross-links in sysfs
> > between the ACPI devices and the "real" device nodes ... e.g. on the
> > system I'm using right now .../device:00/PNP0A03:00/device:15/PNP0B00:00
> > could have a link pointing to /sys/devices/pnp0/00:06 ... and that PNP
> > node in turn could have an "acpi" link pointing back to the ACPI thing.
> > 
> > Such cross-links would let people see those relationships, and observe
> > which links are missing or otherwise strange.  Fixing the bugs would
> > seem unlikely until those things become visible.
>
> Sounds nice.
> The patch below should make sense.

Yeah, that's the idea.  Let's see if it's any good.  :)

I'd just call it "acpi_node" not "ACPI_node" (NO POINT IN SHOUTING),
and might not even use the "_node" suffix.

I cc'd Greg, who's our resident (or is that itinerant?) sysfs guru,
in case he has comments on this issue.

This applied over the "ACPI driver model flags and platform_enable_wake()"
patch with a bit of fuzz, on RC6.

- Dave


> Add cross-links between ACPI device and "real" devices in sysfs.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com> 
> ---
>  drivers/acpi/glue.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+)
> 
> Index: linux-2.6.21-rc5/drivers/acpi/glue.c
> ===================================================================
> --- linux-2.6.21-rc5.orig/drivers/acpi/glue.c	2007-04-06 16:06:04.000000000 +0800
> +++ linux-2.6.21-rc5/drivers/acpi/glue.c	2007-04-06 17:32:40.000000000 +0800
> @@ -147,6 +147,7 @@ EXPORT_SYMBOL(acpi_get_physical_device);
>  static int acpi_bind_one(struct device *dev, acpi_handle handle)
>  {
>  	acpi_status status;
> +	struct acpi_device *acpi_dev;
>  
>  	if (dev->archdata.acpi_handle) {
>  		printk(KERN_WARNING PREFIX
> @@ -161,16 +162,29 @@ static int acpi_bind_one(struct device *
>  	}
>  	dev->archdata.acpi_handle = handle;
>  
> +	if (!acpi_bus_get_device(handle, &acpi_dev)) {
> +		sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj, "ACPI_node");
> +		sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj, "physical_node");
> +	}
> +
>  	return 0;
>  }
>  
>  static int acpi_unbind_one(struct device *dev)
>  {
> +	struct acpi_device *acpi_dev;
> +
>  	if (!dev->archdata.acpi_handle)
>  		return 0;
>  	if (dev == acpi_get_physical_device(dev->archdata.acpi_handle)) {
>  		/* acpi_get_physical_device increase refcnt by one */
>  		put_device(dev);
> +
> +		if (!acpi_bus_get_device(dev->archdata.acpi_handle, &acpi_dev)) {
> +			sysfs_remove_link(&dev->kobj, "ACPI_node");
> +			sysfs_remove_link(&acpi_dev->dev.kobj, "physical_node");
> +		}
> +
>  		acpi_detach_data(dev->archdata.acpi_handle,
>  				 acpi_glue_data_handler);
>  		dev->archdata.acpi_handle = NULL;
> 

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[Greg KH]

On Fri, Apr 06, 2007 at 08:43:30AM -0700, David Brownell wrote:
> > > If this patch starts to get deployed, I expect other people will find
> > > a few other curiousities ... and likely some things to be fixed.
> > 
> > > The /sys/devices/acpi_system:00/ tree is kind of new.  I suspect one
> > > way it could be more informative is to set up cross-links in sysfs
> > > between the ACPI devices and the "real" device nodes ... e.g. on the
> > > system I'm using right now .../device:00/PNP0A03:00/device:15/PNP0B00:00
> > > could have a link pointing to /sys/devices/pnp0/00:06 ... and that PNP
> > > node in turn could have an "acpi" link pointing back to the ACPI thing.
> > > 
> > > Such cross-links would let people see those relationships, and observe
> > > which links are missing or otherwise strange.  Fixing the bugs would
> > > seem unlikely until those things become visible.
> >
> > Sounds nice.
> > The patch below should make sense.
> 
> Yeah, that's the idea.  Let's see if it's any good.  :)
> 
> I'd just call it "acpi_node" not "ACPI_node" (NO POINT IN SHOUTING),
> and might not even use the "_node" suffix.

Yes, please don't shout :)

> I cc'd Greg, who's our resident (or is that itinerant?) sysfs guru,
> in case he has comments on this issue.

Are you _sure_ you have a 1-to-1 relationship here?  No multiple devices
pointing to the same acpi node?  Or the other way around?  If so, you
are going to have to change the name to be something more unique.

Or how about "firmware" instead of "acpi" to be able to have the
userspace tools work on any type of firmware that provides this, like
openfirmware?

thanks,

greg k-h

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Friday 06 April 2007 10:01 pm, Greg KH wrote:

> Are you _sure_ you have a 1-to-1 relationship here?  No multiple devices
> pointing to the same acpi node?  Or the other way around?  If so, you
> are going to have to change the name to be something more unique.

I've wondered that too.  The short answer:  APCI only supports 1-1
here.  It will emit warnings if it tries to bind more than one ACPI
device to a given "real" device ... but errors the other way are
silently ignored.

By adding a warning over this create-links patch, I found that the
system in the $SUBJECT patch (and likely every ACPI system) has
two different nodes that correspond to one ACPI node:

	/sys/devices/pci0000:00 ... pci root node
	/sys/devices/pnp0/00:00 ... id PNP0a03
	/sys/devices/acpi_system:00/device:00/PNP0A03:00 ... ditto

Arguably that's too many sysfs nodes for one device...

Plus, there's the issue of flakey ACPI tables; in the $SUBJECT patch
both MDM and AUD nodes exist in the ACPI namespace, but they could
only refer to one PCI device (with MDM as the wakeup source, not AUD
as listed in the table).  Or maybe that's another case where the ACPI
code isn't handling the tables as sensibly as it might...


> Or how about "firmware" instead of "acpi" to be able to have the
> userspace tools work on any type of firmware that provides this, like
> openfirmware?

Assuming they all adopt that same "parallel tree" model, that seems
like a good idea.  The tools will likely need to understand how ACPI
and OF differ, but there's no point in reserving more names than we
really need.  Though it may be that "parallel trees" should go away.

A small glitch in the patch:  lines bigger than 80 characters.  :)

- Dave

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[Zhang Rui]

On Sat, 2007-04-07 at 13:08 -0700, David Brownell wrote:
> On Friday 06 April 2007 10:01 pm, Greg KH wrote:
> 
> > Are you _sure_ you have a 1-to-1 relationship here?  No multiple devices
> > pointing to the same acpi node?  Or the other way around?  If so, you
> > are going to have to change the name to be something more unique.
> 
> I've wondered that too.  The short answer:  APCI only supports 1-1
> here.
Right.
>   It will emit warnings if it tries to bind more than one ACPI
> device to a given "real" device ... but errors the other way are
> silently ignored.
> 
My understanding is different.
First, one "real" device can only have one device.archdata.acpi_handle,
which means it can only be bound to one ACPI device.
Second, AE_ALREADY_EXISTS will be returned when ACPI tries to bind more
than one "real" devices to the same ACPI device.
> By adding a warning over this create-links patch, I found that the
> system in the $SUBJECT patch (and likely every ACPI system) has
> two different nodes that correspond to one ACPI node:
> 
> 	/sys/devices/pci0000:00 ... pci root node
> 	/sys/devices/pnp0/00:00 ... id PNP0a03
> 	/sys/devices/acpi_system:00/device:00/PNP0A03:00 ... ditto
> 
> Arguably that's too many sysfs nodes for one device...
> 
> Plus, there's the issue of flakey ACPI tables; in the $SUBJECT patch
> both MDM and AUD nodes exist in the ACPI namespace, but they could
> only refer to one PCI device (with MDM as the wakeup source, not AUD
> as listed in the table).  Or maybe that's another case where the ACPI
> code isn't handling the tables as sensibly as it might...
> 
Could you attach this acpidump please? :)

Thanks,
Rui

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

> On Sat, 2007-04-07 at 13:08 -0700, David Brownell wrote:
> > On Friday 06 April 2007 10:01 pm, Greg KH wrote:
> > 
> > > Are you _sure_ you have a 1-to-1 relationship here?  No multiple devices
> > > pointing to the same acpi node?  Or the other way around?  If so, you
> > > are going to have to change the name to be something more unique.
> > 
> > I've wondered that too.  The short answer:  APCI only supports 1-1
> > here.
>
> Right.
>
> >   It will emit warnings if it tries to bind more than one ACPI
> > device to a given "real" device ... but errors the other way are
> > silently ignored.
>
> My understanding is different.
> First, one "real" device can only have one device.archdata.acpi_handle,
> which means it can only be bound to one ACPI device.
> Second, AE_ALREADY_EXISTS will be returned when ACPI tries to bind more
> than one "real" devices to the same ACPI device.

Exactly.  The "first" case emits a warning, the "second" case doesn't;
no matter what it is (though I only saw ALREADY_EXISTS).


When I added a warning to that case:

> > By adding a warning over this create-links patch, I found that the
> > system in the $SUBJECT patch (and likely every ACPI system) has
> > two different nodes that correspond to one ACPI node:
> > 
> > 	/sys/devices/pci0000:00 ... pci root node
> > 	/sys/devices/pnp0/00:00 ... id PNP0a03
> > 	/sys/devices/acpi_system:00/device:00/PNP0A03:00 ... ditto
> > 
> > Arguably that's too many sysfs nodes for one device...

Presumably you've noticed this same thing (not necessarily pnp0/00:00)
on other systems ...


> > Plus, there's the issue of flakey ACPI tables; in the $SUBJECT patch
> > both MDM and AUD nodes exist in the ACPI namespace, but they could
> > only refer to one PCI device (with MDM as the wakeup source, not AUD
> > as listed in the table).  Or maybe that's another case where the ACPI
> > code isn't handling the tables as sensibly as it might...
>
> Could you attach this acpidump please? :)

Off-list; yes.

- Dave

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Saturday 07 April 2007 1:08 pm, David Brownell wrote:

> By adding a warning over this create-links patch, I found that the
> system in the $SUBJECT patch (and likely every ACPI system) has
> two different nodes that correspond to one ACPI node:
> 
> 	/sys/devices/pci0000:00 ... pci root node
> 	/sys/devices/pnp0/00:00 ... id PNP0a03
> 	/sys/devices/acpi_system:00/device:00/PNP0A03:00 ... ditto
> 
> Arguably that's too many sysfs nodes for one device...

On a different system, I found a more interesting issue.

Specifically, pnp0/00:01 and pnp0:00/03 both have ids ... the appended
patch goes on top of the previous pnpacpi patch, and should (nyet tested!)
fix another place I saw that warning.

- Dave

=========	CUT HERE
Handle PNP devices that have the same IDs a bit more sanely ... I suspect
that pnp_dev.number should be compared with some acpi_device field to make
it hook up to a particular ACPI device, but I'll leave that to someone who
knows how that's supposed to work.  At least this will connect e.g. multiple
PNP nodes to ACPI nodes sharing the same PNP0c02 id.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

--- g26.orig/drivers/pnp/pnpacpi/core.c	2007-04-10 01:38:53.000000000 -0700
+++ g26/drivers/pnp/pnpacpi/core.c	2007-04-10 16:16:07.000000000 -0700
@@ -243,6 +243,7 @@ static int __init acpi_pnp_match(struct 
 
 	/* true means it matched */
 	return acpi->flags.hardware_id
+		&& !dev->archdata.acpi_handle
 		&& compare_pnp_id(pnp->id, acpi->pnp.hardware_id);
 }
 

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Tuesday 10 April 2007 4:29 pm, David Brownell wrote:
> 	... the appended
> patch goes on top of the previous pnpacpi patch, and should (nyet tested!)
> fix another place I saw that warning.

And here's a tested version.  Curiouser and curiouser.  I think the mapping
of ACPI tables to sysfs nodes still needs a bunch of work...

- Dave

==========	CUT HERE
Handle PNP devices that have the same IDs a bit more sanely ... this at least
arranges multiple one-to-one mappings between PNP nodes and ACPI nodes that
all share the same PNP0c02 id.  (Maybe mapped to the wrong node though...)

This remains a hack, since it masks failures where PNP0a03 (a PCI root bridge)
is handled by ACPI special case code (/sys/devices/pci0000:00 on my systems
instead of /sys/devices/pnp0/pnp00:0/pci0000:00!) not pure PNPACPI logic.

I suspect that pnp_dev.number and some acpi_device field need to be compared,
(so the right PNP0c02 nodes hook up to each other, PNP0a03 likewise) but I'll
leave that to someone who knows how those ACPI table lookups "should" work.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

--- g26.orig/drivers/pnp/pnpacpi/core.c	2007-04-10 01:38:53.000000000 -0700
+++ g26/drivers/pnp/pnpacpi/core.c	2007-04-10 16:35:13.000000000 -0700
@@ -243,6 +243,7 @@ static int __init acpi_pnp_match(struct 
 
 	/* true means it matched */
 	return acpi->flags.hardware_id
+		&& !acpi_get_physical_device(acpi->handle)
 		&& compare_pnp_id(pnp->id, acpi->pnp.hardware_id);
 }
 

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[Pavel Machek]

Hi!

> > Are you _sure_ you have a 1-to-1 relationship here?  No multiple devices
> > pointing to the same acpi node?  Or the other way around?  If so, you
> > are going to have to change the name to be something more unique.
> 
> I've wondered that too.  The short answer:  APCI only supports 1-1
> here.  It will emit warnings if it tries to bind more than one ACPI
...
> Assuming they all adopt that same "parallel tree" model, that seems
> like a good idea.  The tools will likely need to understand how ACPI
> and OF differ, but there's no point in reserving more names than we
> really need.  Though it may be that "parallel trees" should go away.

If mapping is indeed 1-to-1 in acpi... it would be nice to just merge
the trees.
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Friday 13 April 2007 8:59 am, Pavel Machek wrote:
> Hi!
> 
> > > Are you _sure_ you have a 1-to-1 relationship here?  No multiple devices
> > > pointing to the same acpi node?  Or the other way around?  If so, you
> > > are going to have to change the name to be something more unique.
> > 
> > I've wondered that too.  The short answer:  APCI only supports 1-1
> > here.  It will emit warnings if it tries to bind more than one ACPI

(Clarifying:  the ACPI *code* works that way.  I have no idea whether
ACPI itself makes such restrictions.  And I suspect that ACPI practice
will in some cases couple multiple ACPI devices to one PCI device...)


> ...
> > Assuming they all adopt that same "parallel tree" model, that seems
> > like a good idea.  The tools will likely need to understand how ACPI
> > and OF differ, but there's no point in reserving more names than we
> > really need.  Though it may be that "parallel trees" should go away.
> 
> If mapping is indeed 1-to-1 in acpi... it would be nice to just merge
> the trees.

Could you elaborate a bit ... what do you mean by "merge"?

One way to merge the trees would be to relocate

	/sys/devices/pci* --> ... this *HAS* a PNP node
	/sys/devices/pnp*/X --> /sys/devices/acpi_system*/.../X

Not having the PCI root be its PNPACPI node seems more buglike
to me than anything else.  And for other nodes, it seems like
maybe there should be a "firmware" subdirectory holding the
ACPI state (hid, path, etc) ... creating a minor chicken/egg
problem that ACPI would need to sort out.  Likely one can just
assume PCI and PNP busses exist, so creating nodes on those
buses based on ACPI tables would not be a problem.  And for
ACPI-internal devices, like SLPB and power buttons, the ACPI
bus would suffice.  But are there other busses?

- Dave

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Tuesday 17 April 2007 12:53 pm, David Brownell wrote:
> On Friday 13 April 2007 8:59 am, Pavel Machek wrote:
> 
> > ...
> > > Assuming they all adopt that same "parallel tree" model, that seems
> > > like a good idea.  The tools will likely need to understand how ACPI
> > > and OF differ, but there's no point in reserving more names than we
> > > really need.  Though it may be that "parallel trees" should go away.
> > 
> > If mapping is indeed 1-to-1 in acpi... it would be nice to just merge
> > the trees.
> 
> Could you elaborate a bit ... what do you mean by "merge"?
> 
> One way to merge the trees would be to relocate
> 
> 	/sys/devices/pci* --> ... this *HAS* a PNP node
> 	/sys/devices/pnp*/X --> /sys/devices/acpi_system*/.../X
> 
> Not having the PCI root be its PNPACPI node seems more buglike
> to me than anything else.  And for other nodes...

Looks like the i8042 serial nodes will be bizarre too:

	/sys/devices/pnp0/00:09
		... touchpad's PNP node
	/sys/devices/acpi_system:00/device:00/PNP0A03:00/device:15/PNP0F13:00
		... its ACPI node
	/sys/devices/platform/i8042/serio4
		... its serio node

That seems like two nodes too many, but without me trying to twist
my mind around i8042 issues, I can't quite speculate why struct
serio "is-a" device rather than "has-a" device (the PNP node) as
would be the case with a more normal driver structure.

But the existence of that device_add() in serio.c sure explains why
the PNP node doesn't get associated with the input class device one
would expect from knowing that 00:09 is the touchpad.

And hmm, just this morning I saw email from Greg re-affirming that
drivers should not device_add().  Converting such legacy drivers is
simple though, right?  :)

- Dave

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[Greg KH]

On Tue, Apr 17, 2007 at 02:57:49PM -0700, David Brownell wrote:
> On Tuesday 17 April 2007 12:53 pm, David Brownell wrote:
> > On Friday 13 April 2007 8:59 am, Pavel Machek wrote:
> > 
> > > ...
> > > > Assuming they all adopt that same "parallel tree" model, that seems
> > > > like a good idea.  The tools will likely need to understand how ACPI
> > > > and OF differ, but there's no point in reserving more names than we
> > > > really need.  Though it may be that "parallel trees" should go away.
> > > 
> > > If mapping is indeed 1-to-1 in acpi... it would be nice to just merge
> > > the trees.
> > 
> > Could you elaborate a bit ... what do you mean by "merge"?
> > 
> > One way to merge the trees would be to relocate
> > 
> > 	/sys/devices/pci* --> ... this *HAS* a PNP node
> > 	/sys/devices/pnp*/X --> /sys/devices/acpi_system*/.../X
> > 
> > Not having the PCI root be its PNPACPI node seems more buglike
> > to me than anything else.  And for other nodes...
> 
> Looks like the i8042 serial nodes will be bizarre too:
> 
> 	/sys/devices/pnp0/00:09
> 		... touchpad's PNP node
> 	/sys/devices/acpi_system:00/device:00/PNP0A03:00/device:15/PNP0F13:00
> 		... its ACPI node
> 	/sys/devices/platform/i8042/serio4
> 		... its serio node
> 
> That seems like two nodes too many, but without me trying to twist
> my mind around i8042 issues, I can't quite speculate why struct
> serio "is-a" device rather than "has-a" device (the PNP node) as
> would be the case with a more normal driver structure.
> 
> But the existence of that device_add() in serio.c sure explains why
> the PNP node doesn't get associated with the input class device one
> would expect from knowing that 00:09 is the touchpad.

Ick, how can we fix this up?

> And hmm, just this morning I saw email from Greg re-affirming that
> drivers should not device_add().  Converting such legacy drivers is
> simple though, right?  :)

Heh, yeah right, they can remain platform drivers :)

thanks,

greg k-h

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Tuesday 17 April 2007 8:03 pm, Greg KH wrote:
> On Tue, Apr 17, 2007 at 02:57:49PM -0700, David Brownell wrote:

> > Looks like the i8042 serial nodes will be bizarre too:
> > 
> > 	/sys/devices/pnp0/00:09
> > 		... touchpad's PNP node
> > 	/sys/devices/acpi_system:00/device:00/PNP0A03:00/device:15/PNP0F13:00
> > 		... its ACPI node
> > 	/sys/devices/platform/i8042/serio4
> > 		... its serio node
> > 
> > That seems like two nodes too many, but without me trying to twist
> > my mind around i8042 issues, I can't quite speculate why struct
> > serio "is-a" device rather than "has-a" device (the PNP node) as
> > would be the case with a more normal driver structure.
> > 
> > But the existence of that device_add() in serio.c sure explains why
> > the PNP node doesn't get associated with the input class device one
> > would expect from knowing that 00:09 is the touchpad.
> 
> Ick, how can we fix this up?

You're asking the wrong penguin for that answer ... :)

But I'd suggest that someone who knows ACPI -- and PNPACPI --- work
on merging those ACPI and PNP nodes.  (Same thing would need doing
with ACPI and PCI ...)

That ACPI subtree is quite new, at least in terms of being public.
So I'd think its details must be very flexible right now.


> > And hmm, just this morning I saw email from Greg re-affirming that
> > drivers should not device_add().  Converting such legacy drivers is
> > simple though, right?  :)
> 
> Heh, yeah right, they can remain platform drivers :)

Most platform drivers aren't legacy drivers.  Just the x86 ones!
And a lot of those wouldn't be headaches if PNPACPI were used better...

In this case, making the i8042 stuff use PNP more naturally would
seem like the most natural way to resolve the problem for all but
pre-ACPI legacy x86 hardware.  Now that ACPI implies PNPACPI by
default, various corner cases have vanished.

That said, I know less about i8042 wierdness -- and I know there's
a lot of it! -- than about ACPI, so I'll stay out of that.  Beyond
pointing out these curious little corner cases I've uncovered while
trying to make sure wakeup annotations get assigned to the right
sysfs device nodes.  :)

- Dave


> 
> thanks,
> 
> greg k-h
> 

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[Matthew Garrett]

On Tue, Apr 03, 2007 at 05:41:42PM -0700, David Brownell wrote:
> This updates /proc/acpi/wakeup to be more informative, primarily by showing
> the sysfs node associated with each wakeup-enabled device.  Example:

This looks good. 

> 	S139	  S4	 disabled  

Any idea what this one is? There's the potential for all sorts of weird 
platform devices to expose wakeup capabilities.

> Eventually this file should be removed, but until then it's almost the only
> way we have to tell how the relevant ACPI tables are broken (and cope).  In
> that example, two devices don't actually exist (USB3, S139), one can't issue
> wakeup events (PCI0), and two seem harmlessly (?) confused (MDM and AUD are
> the same PCI device, but it's the _modem_ that does wake-on-ring).  

Could the MDM entry be referring to the modem codec on the ac97 or 
hda bus? 

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[David Brownell]

On Thursday 05 April 2007 2:26 am, Matthew Garrett wrote:
> On Tue, Apr 03, 2007 at 05:41:42PM -0700, David Brownell wrote:
> > This updates /proc/acpi/wakeup to be more informative, primarily by showing
> > the sysfs node associated with each wakeup-enabled device.  Example:
> 
> This looks good. 

Also "good at exposing some puzzling info" ... ;)


> > 	S139	  S4	 disabled  
> 
> Any idea what this one is? There's the potential for all sorts of weird 
> platform devices to expose wakeup capabilities.

My guess is Firewire (IEEE 1394); "F193" on a different system (different
chip vendor) *is* Firewire.  This BIOS seems to have wrongly copied ACPI
tables from a similar system with a fancier southbridge from the same
chip vendor.  (ISTR the vendor's reference design used the fancier chip.)


> > Eventually this file should be removed, but until then it's almost the only
> > way we have to tell how the relevant ACPI tables are broken (and cope).  In
> > that example, two devices don't actually exist (USB3, S139), one can't issue
> > wakeup events (PCI0), and two seem harmlessly (?) confused (MDM and AUD are
> > the same PCI device, but it's the _modem_ that does wake-on-ring).  
> 
> Could the MDM entry be referring to the modem codec on the ac97 or 
> hda bus? 

That's my assumption; yes.   Another system lists "AUD0" and "MODM", but
shows neither as a wakeup device.  As I said, "confused".

- Dave

Re: [patch 2.6.21-rc5-git] make /proc/acpi/wakeup more useful

[Len Brown]

Applied,
and thanks for turning over the rocks in sysfs to see what wiggled out:-)

thanks,
-Len

On Tuesday 03 April 2007 20:41, David Brownell wrote:
> This updates /proc/acpi/wakeup to be more informative, primarily by showing
> the sysfs node associated with each wakeup-enabled device.  Example:
> 
> 	Device	S-state	  Status   Sysfs node
> 	PCI0	  S4	 disabled  no-bus:pci0000:00
> 	PS2M	  S4	 disabled  pnp:00:05
> 	PS2K	  S4	 disabled  pnp:00:06
> 	UAR1	  S4	 disabled  pnp:00:08
> 	USB1	  S3	 disabled  pci:0000:00:03.0
> 	USB2	  S3	 disabled  pci:0000:00:03.1
> 	USB3	  S3	 disabled  
> 	USB4	  S3	 disabled  pci:0000:00:03.3
> 	S139	  S4	 disabled  
> 	LAN	  S4	 disabled  pci:0000:00:04.0
> 	MDM	  S4	 disabled  
> 	AUD	  S4	 disabled  pci:0000:00:02.7
> 	SLPB	  S4	*enabled   
> 
> Eventually this file should be removed, but until then it's almost the only
> way we have to tell how the relevant ACPI tables are broken (and cope).  In
> that example, two devices don't actually exist (USB3, S139), one can't issue
> wakeup events (PCI0), and two seem harmlessly (?) confused (MDM and AUD are
> the same PCI device, but it's the _modem_ that does wake-on-ring).  
> 
> In particular, we need to be sure driver model nodes are properly hooked
> up before we can get rid of this ACPI-only interface for wakeup events.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> 
> --- g26.orig/drivers/acpi/sleep/proc.c	2007-04-03 16:27:22.000000000 -0700
> +++ g26/drivers/acpi/sleep/proc.c	2007-04-03 16:43:26.000000000 -0700
> @@ -360,21 +360,31 @@ acpi_system_wakeup_device_seq_show(struc
>  {
>  	struct list_head *node, *next;
>  
> -	seq_printf(seq, "Device	Sleep state	Status\n");
> +	seq_printf(seq, "Device\tS-state\t  Status   Sysfs node\n");
>  
>  	spin_lock(&acpi_device_lock);
>  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
>  		struct acpi_device *dev =
>  		    container_of(node, struct acpi_device, wakeup_list);
> +		struct device *ldev;
>  
>  		if (!dev->wakeup.flags.valid)
>  			continue;
>  		spin_unlock(&acpi_device_lock);
> -		seq_printf(seq, "%4s	%4d		%s%8s\n",
> +
> +		ldev = acpi_get_physical_device(dev->handle);
> +		seq_printf(seq, "%s\t  S%d\t%c%-8s  ",
>  			   dev->pnp.bus_id,
>  			   (u32) dev->wakeup.sleep_state,
> -			   dev->wakeup.flags.run_wake ? "*" : "",
> +			   dev->wakeup.flags.run_wake ? '*' : ' ',
>  			   dev->wakeup.state.enabled ? "enabled" : "disabled");
> +		if (ldev)
> +			seq_printf(seq, "%s:%s",
> +				ldev->bus ? ldev->bus->name : "no-bus",
> +				ldev->bus_id);
> +		seq_printf(seq, "\n");
> +		put_device(ldev);
> +
>  		spin_lock(&acpi_device_lock);
>  	}
>  	spin_unlock(&acpi_device_lock);
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>