Re: sysfs regression on Supermicro X7DB8+

Re: sysfs regression on Supermicro X7DB8+

[Len Brown]

Rui, Shaohua,
Thanks for fixing the boot regression on the Supermicro.

Here is the patch as applied from http://bugzilla.kernel.org/show_bug.cgi?id=7695
to the tip of the sysfs branch.

-Len

commit 2786f6e388e9dfe9e7b1c3c6bd7fcfba9cfb9831
Author: Rui Zhang <rui.zhang@intel.com>
Date:   Thu Dec 21 02:21:13 2006 -0500

    ACPI: fix Supermicro X7DB8+ Boot regression
    
    http://bugzilla.kernel.org/show_bug.cgi?id=7695
    
    Originally we converted bind/unbind to use a new pci bridge driver.
    The driver will add/remove _PRT, so we can eventually remove
    .bind/.unbind methods.
    
    But we found that some of the _ADR-Based devices don't have _PRT,
    i.e. they are not managed by the new ACPI PCI bridge driver.
    So that .bind method is not called for some _ADR-Based devices,
    which leads to a failure.
    
    Now we make ACPI PCI Root Bridge Driver scan and binds all _ADR-Based devices
    once the driver is loaded, in the .add method of ACPI PCI Root Bridge driver.
    
    Extra code path for calling .bind/.unbind when _ADR-Based devices
    are hot added/removed is also added.
    
    Signed-off-by: Zhang Rui <rui.zhang@intel.com>
    Signed-off-by: Len Brown <len.brown@intel.com>

diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
index aa05e92..1e2ae6e 100644
--- a/drivers/acpi/pci_bind.c
+++ b/drivers/acpi/pci_bind.c
@@ -223,6 +223,8 @@ int acpi_pci_bind(struct acpi_device *device)
 				  data->id.segment, data->id.bus,
 				  data->id.device, data->id.function));
 		data->bus = data->dev->subordinate;
+		device->ops.bind = acpi_pci_bind;
+		device->ops.unbind = acpi_pci_unbind;
 	}
 
 	/*
@@ -352,6 +354,8 @@ acpi_pci_bind_root(struct acpi_device *device,
 
 	data->id = *id;
 	data->bus = bus;
+	device->ops.bind = acpi_pci_bind;
+	device->ops.unbind = acpi_pci_unbind;
 
 	acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer);
 
@@ -374,39 +378,3 @@ acpi_pci_bind_root(struct acpi_device *device,
 
 	return result;
 }
-
-#define ACPI_PCI_BRIDGE_DRIVER_NAME "ACPI PCI Bridge Driver"
-
-static int acpi_pci_bridge_add(struct acpi_device *device);
-static int acpi_pci_bridge_remove(struct acpi_device *device, int type);
-
-static struct acpi_driver acpi_pci_bridge_driver = {
-       .name = ACPI_PCI_BRIDGE_DRIVER_NAME,
-	.ids = ACPI_PCI_BRIDGE_HID,
-       .ops = {
-               .add = acpi_pci_bridge_add,
-               .remove = acpi_pci_bridge_remove,
-       },
-};
-
-static int acpi_pci_bridge_add(struct acpi_device *device)
-{
-       return acpi_pci_bind(device);
-}
-
-static int acpi_pci_bridge_remove(struct acpi_device *device, int type)
-{
-       return acpi_pci_unbind(device);
-}
-
-static int __init acpi_pci_bridge_init(void)
-{
-       if (acpi_pci_disabled)
-               return 0;
-       if (acpi_bus_register_driver(&acpi_pci_bridge_driver) < 0)
-               return -ENODEV;
-       return 0;
-}
-
-/* Should be called after ACPI pci root driver */
-subsys_initcall(acpi_pci_bridge_init);
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 9cfc741..2e1a74a 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -151,6 +151,21 @@ static acpi_status try_get_root_bridge_busnr(acpi_handle handle, int *busnum)
 	return AE_OK;
 }
 
+static void acpi_pci_bridge_scan(struct acpi_device *device)
+{
+	int status;
+	struct acpi_device *child = NULL;
+
+	if (device->flags.bus_address)
+		if (device->parent && device->parent->ops.bind) {
+			status = device->parent->ops.bind(device);
+			if (!status) {
+				list_for_each_entry(child, &device->children, node)
+					acpi_pci_bridge_scan(child);
+			}
+		}
+}
+
 static int acpi_pci_root_add(struct acpi_device *device)
 {
 	int result = 0;
@@ -159,6 +174,7 @@ static int acpi_pci_root_add(struct acpi_device *device)
 	acpi_status status = AE_OK;
 	unsigned long value = 0;
 	acpi_handle handle = NULL;
+	struct acpi_device *child;
 
 
 	if (!device)
@@ -175,6 +191,8 @@ static int acpi_pci_root_add(struct acpi_device *device)
 	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
 	acpi_driver_data(device) = root;
 
+	device->ops.bind = acpi_pci_bind;
+
 	/* 
 	 * Segment
 	 * -------
@@ -294,6 +312,12 @@ static int acpi_pci_root_add(struct acpi_device *device)
 		result = acpi_pci_irq_add_prt(device->handle, root->id.segment,
 					      root->id.bus);
 
+	/*
+	 * Scan and bind all _ADR-Based Devices
+	 */
+	list_for_each_entry(child, &device->children, node)
+		acpi_pci_bridge_scan(child);
+
       end:
 	if (result) {
 		if (!list_empty(&root->node))
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 769e54b..30a39ba 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -837,20 +837,6 @@ acpi_video_bus_match(struct acpi_device *device)
 	return -ENODEV;
 }
 
-static int acpi_pci_bridge_match(struct acpi_device *device)
-{
-       acpi_status status;
-       acpi_handle handle;
-
-       /* pci bridge has _PRT but isn't PNP0A03 */
-       status = acpi_get_handle(device->handle, METHOD_NAME__PRT, &handle);
-       if (ACPI_FAILURE(status))
-               return -ENODEV;
-       if (!acpi_match_ids(device, "PNP0A03"))
-               return -ENODEV;
-       return 0;
-}
-
 static void acpi_device_set_id(struct acpi_device *device,
 			       struct acpi_device *parent, acpi_handle handle,
 			       int type)
@@ -886,10 +872,6 @@ static void acpi_device_set_id(struct acpi_device *device,
 			status = acpi_video_bus_match(device);
 			if(ACPI_SUCCESS(status))
 				hid = ACPI_VIDEO_HID;
-
-			status = acpi_pci_bridge_match(device);
-			if(ACPI_SUCCESS(status))
-				hid = ACPI_PCI_BRIDGE_HID;
 		}
 		break;
 	case ACPI_BUS_TYPE_POWER:
@@ -1021,6 +1003,13 @@ static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
 	if (!rmdevice)
 		return 0;
 
+	/*
+	 * unbind _ADR-Based Devices when hot removal
+	 */
+	if (dev->flags.bus_address) {
+		if ((dev->parent) && (dev->parent->ops.unbind))
+			dev->parent->ops.unbind(dev);
+	}
 	acpi_device_unregister(dev, ACPI_BUS_REMOVAL_EJECT);
 
 	return 0;
@@ -1137,6 +1126,14 @@ acpi_add_single_object(struct acpi_device **child,
 
 	result = acpi_device_register(device, parent);
 
+	/*
+	 * Bind _ADR-Based Devices when hot add
+	 */
+	if (device->flags.bus_address) {
+		if (device->parent && device->parent->ops.bind)
+			device->parent->ops.bind(device);
+	}
+
       end:
 	if (!result)
 		*child = device;
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index be67750..2781e66 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -44,7 +44,6 @@
 #define ACPI_BUTTON_HID_SLEEPF		"ACPI_FSB"
 
 #define ACPI_VIDEO_HID			"ACPI_VID"
-#define ACPI_PCI_BRIDGE_HID		"ACPI_PCI"
 /* --------------------------------------------------------------------------
                                        PCI
    -------------------------------------------------------------------------- */

Re: sysfs regression on Supermicro X7DB8+

[Len Brown]

lenb@supermicro:~> ls /sys/bus/acpi/drivers
ACPI AC Adapter Driver           ACPI PCI Root Bridge Driver
ACPI Battery Driver              ACPI Power Resource Driver
ACPI Button Driver               ACPI Processor Driver
ACPI container driver            ACPI Thermal Zone Driver
ACPI Embedded Controller Driver  hpet
ACPI Fan Driver                  motherboard
ACPI PCI Interrupt Link Driver
lenb@supermicro:~>

Seems with the exception of motherboard and hpet, we have the description
confused with the .name.  I'll fix this.

What is "PNPIDNON" (below) supposed to mean?

thanks,
-Len


supermicro:~ # ls /sys/bus/acpi/devices
ACPI_CPU:00  ACPI_THM:00  PNP0800:00  PNP0C0F:03   PNPIDNON:04  PNPIDNON:0e
ACPI_CPU:01  INT0800:00   PNP0A03:00  PNP0C0F:04   PNPIDNON:05  PNPIDNON:0f
ACPI_CPU:02  PNP0000:00   PNP0A05:00  PNP0C0F:05   PNPIDNON:06  PNPIDNON:10
ACPI_CPU:03  PNP0100:00   PNP0B00:00  PNP0C0F:06   PNPIDNON:07  PNPIDNON:11
ACPI_CPU:04  PNP0200:00   PNP0C02:00  PNP0C0F:07   PNPIDNON:08  PNPIDNON:12
ACPI_CPU:05  PNP0303:00   PNP0C04:00  PNP0F13:00   PNPIDNON:09  PNPIDNON:13
ACPI_CPU:06  PNP0401:00   PNP0C0C:00  PNPIDNON:00  PNPIDNON:0a  PNPIDNON:14
ACPI_CPU:07  PNP0501:00   PNP0C0F:00  PNPIDNON:01  PNPIDNON:0b  PNPIDNON:15
ACPI_FPB:00  PNP0501:01   PNP0C0F:01  PNPIDNON:02  PNPIDNON:0c
ACPI_SYS:00  PNP0700:00   PNP0C0F:02  PNPIDNON:03  PNPIDNON:0d

supermicro:~# ls /sys/bus/acpi/devices/PNPIDNON*
PNPIDNON:00:
path  PNP0A03:00  power  subsystem  uevent

PNPIDNON:01:
path  PNPIDNON:02  PNPIDNON:07  power  subsystem  uevent

PNPIDNON:02:
path  PNPIDNON:03  PNPIDNON:06  power  subsystem  uevent

PNPIDNON:03:
path  PNPIDNON:04  PNPIDNON:05  power  subsystem  uevent

PNPIDNON:04:
path  power  subsystem  uevent

PNPIDNON:05:
path  power  subsystem  uevent

PNPIDNON:06:
path  power  subsystem  uevent

PNPIDNON:07:
path  power  subsystem  uevent

PNPIDNON:08:
path  power  subsystem  uevent

PNPIDNON:09:
path  power  subsystem  uevent

PNPIDNON:0a:
path  power  subsystem  uevent

PNPIDNON:0b:
path  power  subsystem  uevent

PNPIDNON:0c:
path  power  subsystem  uevent

PNPIDNON:0d:
path  power  subsystem  uevent

PNPIDNON:0e:
path  power  subsystem  uevent

PNPIDNON:0f:
path  power  subsystem  uevent

PNPIDNON:10:
INT0800:00  PNP0200:00  PNP0C02:00  PNP0C0F:02  PNP0C0F:06  uevent
path        PNP0800:00  PNP0C04:00  PNP0C0F:03  PNP0C0F:07
PNP0000:00  PNP0A05:00  PNP0C0F:00  PNP0C0F:04  power
PNP0100:00  PNP0B00:00  PNP0C0F:01  PNP0C0F:05  subsystem

PNPIDNON:11:
path  PNPIDNON:12  power  subsystem  uevent

PNPIDNON:12:
path  PNPIDNON:13  PNPIDNON:14  power  subsystem  uevent

PNPIDNON:13:
path  power  subsystem  uevent

PNPIDNON:14:
path  power  subsystem  uevent

PNPIDNON:15:
path  power  subsystem  uevent

Re: sysfs regression on Supermicro X7DB8+

[Zhang Rui]

On Thu, 2006-12-21 at 04:28 -0500, Len Brown wrote:
> lenb@supermicro:~> ls /sys/bus/acpi/drivers
> ACPI AC Adapter Driver           ACPI PCI Root Bridge Driver
> ACPI Battery Driver              ACPI Power Resource Driver
> ACPI Button Driver               ACPI Processor Driver
> ACPI container driver            ACPI Thermal Zone Driver
> ACPI Embedded Controller Driver  hpet
> ACPI Fan Driver                  motherboard
> ACPI PCI Interrupt Link Driver
> lenb@supermicro:~>
> 
> Seems with the exception of motherboard and hpet, we have the description
> confused with the .name.  I'll fix this.
> 
Yes, Thomas mentioned it about half a month ago.
And I think it should be fixed in another patch separated from the sysfs
branch. And short names like "ac", "fan", seems to be more convenient.

> What is "PNPIDNON" (below) supposed to mean?
> 
For devices with PNPID, we use PNPID:instance_no as the bus_id.
But for others without PNPID, we set PNPIDNON:instance_no as their
bus_id instead.
> 
> 
> supermicro:~ # ls /sys/bus/acpi/devices
> ACPI_CPU:00  ACPI_THM:00  PNP0800:00  PNP0C0F:03   PNPIDNON:04  PNPIDNON:0e
> ACPI_CPU:01  INT0800:00   PNP0A03:00  PNP0C0F:04   PNPIDNON:05  PNPIDNON:0f
> ACPI_CPU:02  PNP0000:00   PNP0A05:00  PNP0C0F:05   PNPIDNON:06  PNPIDNON:10
> ACPI_CPU:03  PNP0100:00   PNP0B00:00  PNP0C0F:06   PNPIDNON:07  PNPIDNON:11
> ACPI_CPU:04  PNP0200:00   PNP0C02:00  PNP0C0F:07   PNPIDNON:08  PNPIDNON:12
> ACPI_CPU:05  PNP0303:00   PNP0C04:00  PNP0F13:00   PNPIDNON:09  PNPIDNON:13
> ACPI_CPU:06  PNP0401:00   PNP0C0C:00  PNPIDNON:00  PNPIDNON:0a  PNPIDNON:14
> ACPI_CPU:07  PNP0501:00   PNP0C0F:00  PNPIDNON:01  PNPIDNON:0b  PNPIDNON:15
> ACPI_FPB:00  PNP0501:01   PNP0C0F:01  PNPIDNON:02  PNPIDNON:0c
> ACPI_SYS:00  PNP0700:00   PNP0C0F:02  PNPIDNON:03  PNPIDNON:0d
> 
> supermicro:~# ls /sys/bus/acpi/devices/PNPIDNON*
> PNPIDNON:00:
> path  PNP0A03:00  power  subsystem  uevent
> 
> PNPIDNON:01:
> path  PNPIDNON:02  PNPIDNON:07  power  subsystem  uevent
> 
> PNPIDNON:02:
> path  PNPIDNON:03  PNPIDNON:06  power  subsystem  uevent
> 
> PNPIDNON:03:
> path  PNPIDNON:04  PNPIDNON:05  power  subsystem  uevent
> 
> PNPIDNON:04:
> path  power  subsystem  uevent
> 
> PNPIDNON:05:
> path  power  subsystem  uevent
> 
> PNPIDNON:06:
> path  power  subsystem  uevent
> 
> PNPIDNON:07:
> path  power  subsystem  uevent
> 
> PNPIDNON:08:
> path  power  subsystem  uevent
> 
> PNPIDNON:09:
> path  power  subsystem  uevent
> 
> PNPIDNON:0a:
> path  power  subsystem  uevent
> 
> PNPIDNON:0b:
> path  power  subsystem  uevent
> 
> PNPIDNON:0c:
> path  power  subsystem  uevent
> 
> PNPIDNON:0d:
> path  power  subsystem  uevent
> 
> PNPIDNON:0e:
> path  power  subsystem  uevent
> 
> PNPIDNON:0f:
> path  power  subsystem  uevent
> 
> PNPIDNON:10:
> INT0800:00  PNP0200:00  PNP0C02:00  PNP0C0F:02  PNP0C0F:06  uevent
> path        PNP0800:00  PNP0C04:00  PNP0C0F:03  PNP0C0F:07
> PNP0000:00  PNP0A05:00  PNP0C0F:00  PNP0C0F:04  power
> PNP0100:00  PNP0B00:00  PNP0C0F:01  PNP0C0F:05  subsystem
> 
> PNPIDNON:11:
> path  PNPIDNON:12  power  subsystem  uevent
> 
> PNPIDNON:12:
> path  PNPIDNON:13  PNPIDNON:14  power  subsystem  uevent
> 
> PNPIDNON:13:
> path  power  subsystem  uevent
> 
> PNPIDNON:14:
> path  power  subsystem  uevent
> 
> PNPIDNON:15:
> path  power  subsystem  uevent

Re: sysfs regression on Supermicro X7DB8+

[Len Brown]

On Thursday 21 December 2006 04:50, Zhang Rui wrote:
> On Thu, 2006-12-21 at 04:28 -0500, Len Brown wrote:
> > lenb@supermicro:~> ls /sys/bus/acpi/drivers
> > ACPI AC Adapter Driver           ACPI PCI Root Bridge Driver
> > ACPI Battery Driver              ACPI Power Resource Driver
> > ACPI Button Driver               ACPI Processor Driver
> > ACPI container driver            ACPI Thermal Zone Driver
> > ACPI Embedded Controller Driver  hpet
> > ACPI Fan Driver                  motherboard
> > ACPI PCI Interrupt Link Driver
> > lenb@supermicro:~>
> > 
> > Seems with the exception of motherboard and hpet, we have the description
> > confused with the .name.  I'll fix this.
> > 
> Yes, Thomas mentioned it about half a month ago.
> And I think it should be fixed in another patch separated from the sysfs
> branch. And short names like "ac", "fan", seems to be more convenient.
 
will do.

> > What is "PNPIDNON" (below) supposed to mean?
> > 
> For devices with PNPID, we use PNPID:instance_no as the bus_id.
> But for others without PNPID, we set PNPIDNON:instance_no as their
> bus_id instead.

There must be a better way to name a device that has no  PNPid   than PNPIDNON:#

How about "device:#"?

Also,  this for the first time exposes the internal fake ones

/* _HID definitions */

#define ACPI_POWER_HID                  "ACPI_PWR"
#define ACPI_PROCESSOR_HID              "ACPI_CPU"
#define ACPI_SYSTEM_HID                 "ACPI_SYS"
#define ACPI_THERMAL_HID                "ACPI_THM"
#define ACPI_BUTTON_HID_POWERF          "ACPI_FPB"
#define ACPI_BUTTON_HID_SLEEPF          "ACPI_FSB"

If we are going to expose these, then we should spend a moment to think about how
understandable they'll be appearing in the file system.  ACPI_FSB doesn't bring
"Fixed Function Sleep Button" to mind when I see it...

Any reason we can't re-use "ACPI0007" for ACPI_PROCESSOR_HID
It is already reserved in the spec.

BTW. anybody know what PNP0C08 is supposed to be?
Seems it is reserved OS use to describe generic ACPI resources
that are not associated with a particular device.

Note that while I don't think we are constrained here, standard PNP-id's must be of the form AAA####
where the A's are letters and the #'s are hex.  ACPI-id's must be of the form ACPI####
where #'s are again hex.  It would be nice if we could work within those constraints
and not conflict with the standard in case something is expecting those formats.
eg. IBM specific devices are IBM####, Toshiba are TOS####.
But the down-side is that the #### doesn't tell the reader much -- particularly
if it is non-standard.
But if we are not going to stay within the standard format and we are going to blow the 8-character
convention, then I vote for strings that actually make sense to human,
say something like this:

#define ACPI_POWER_HID                  "power_resource"
#define ACPI_PROCESSOR_HID              "processor"
#define ACPI_SYSTEM_HID                 "system"
#define ACPI_THERMAL_HID                "thermal"
#define ACPI_BUTTON_HID_POWERF          "button_power_fixed"
#define ACPI_BUTTON_HID_SLEEPF          "button_sleep_fixed"

thanks,
-Len
 
> > supermicro:~ # ls /sys/bus/acpi/devices
> > ACPI_CPU:00  ACPI_THM:00  PNP0800:00  PNP0C0F:03   PNPIDNON:04  PNPIDNON:0e
> > ACPI_CPU:01  INT0800:00   PNP0A03:00  PNP0C0F:04   PNPIDNON:05  PNPIDNON:0f
> > ACPI_CPU:02  PNP0000:00   PNP0A05:00  PNP0C0F:05   PNPIDNON:06  PNPIDNON:10
> > ACPI_CPU:03  PNP0100:00   PNP0B00:00  PNP0C0F:06   PNPIDNON:07  PNPIDNON:11
> > ACPI_CPU:04  PNP0200:00   PNP0C02:00  PNP0C0F:07   PNPIDNON:08  PNPIDNON:12
> > ACPI_CPU:05  PNP0303:00   PNP0C04:00  PNP0F13:00   PNPIDNON:09  PNPIDNON:13
> > ACPI_CPU:06  PNP0401:00   PNP0C0C:00  PNPIDNON:00  PNPIDNON:0a  PNPIDNON:14
> > ACPI_CPU:07  PNP0501:00   PNP0C0F:00  PNPIDNON:01  PNPIDNON:0b  PNPIDNON:15
> > ACPI_FPB:00  PNP0501:01   PNP0C0F:01  PNPIDNON:02  PNPIDNON:0c
> > ACPI_SYS:00  PNP0700:00   PNP0C0F:02  PNPIDNON:03  PNPIDNON:0d
> > 
> > supermicro:~# ls /sys/bus/acpi/devices/PNPIDNON*
> > PNPIDNON:00:
> > path  PNP0A03:00  power  subsystem  uevent
> > 
> > PNPIDNON:01:
> > path  PNPIDNON:02  PNPIDNON:07  power  subsystem  uevent
> > 
> > PNPIDNON:02:
> > path  PNPIDNON:03  PNPIDNON:06  power  subsystem  uevent
> > 
> > PNPIDNON:03:
> > path  PNPIDNON:04  PNPIDNON:05  power  subsystem  uevent
> > 
> > PNPIDNON:04:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:05:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:06:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:07:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:08:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:09:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:0a:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:0b:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:0c:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:0d:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:0e:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:0f:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:10:
> > INT0800:00  PNP0200:00  PNP0C02:00  PNP0C0F:02  PNP0C0F:06  uevent
> > path        PNP0800:00  PNP0C04:00  PNP0C0F:03  PNP0C0F:07
> > PNP0000:00  PNP0A05:00  PNP0C0F:00  PNP0C0F:04  power
> > PNP0100:00  PNP0B00:00  PNP0C0F:01  PNP0C0F:05  subsystem
> > 
> > PNPIDNON:11:
> > path  PNPIDNON:12  power  subsystem  uevent
> > 
> > PNPIDNON:12:
> > path  PNPIDNON:13  PNPIDNON:14  power  subsystem  uevent
> > 
> > PNPIDNON:13:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:14:
> > path  power  subsystem  uevent
> > 
> > PNPIDNON:15:
> > path  power  subsystem  uevent
> 

Re: sysfs regression on Supermicro X7DB8+

[Zhang Rui]

> > > What is "PNPIDNON" (below) supposed to mean?
> > > 
> > For devices with PNPID, we use PNPID:instance_no as the bus_id.
> > But for others without PNPID, we set PNPIDNON:instance_no as their
> > bus_id instead.
> 
> There must be a better way to name a device that has no  PNPid   than PNPIDNON:#
> 
> How about "device:#"?
Sounds good. Will do.
> Also,  this for the first time exposes the internal fake ones
> 
> /* _HID definitions */
> 
> #define ACPI_POWER_HID                  "ACPI_PWR"
> #define ACPI_PROCESSOR_HID              "ACPI_CPU"
> #define ACPI_SYSTEM_HID                 "ACPI_SYS"
> #define ACPI_THERMAL_HID                "ACPI_THM"
> #define ACPI_BUTTON_HID_POWERF          "ACPI_FPB"
> #define ACPI_BUTTON_HID_SLEEPF          "ACPI_FSB"
> 
> If we are going to expose these, then we should spend a moment to think about how
> understandable they'll be appearing in the file system.  ACPI_FSB doesn't bring
> "Fixed Function Sleep Button" to mind when I see it...
> 
> Any reason we can't re-use "ACPI0007" for ACPI_PROCESSOR_HID
> It is already reserved in the spec.
> 
> BTW. anybody know what PNP0C08 is supposed to be?
> Seems it is reserved OS use to describe generic ACPI resources
> that are not associated with a particular device.
> 
> Note that while I don't think we are constrained here, standard PNP-id's must be of the form AAA####
> where the A's are letters and the #'s are hex.  ACPI-id's must be of the form ACPI####
> where #'s are again hex.  It would be nice if we could work within those constraints
> and not conflict with the standard in case something is expecting those formats.
> eg. IBM specific devices are IBM####, Toshiba are TOS####.
> But the down-side is that the #### doesn't tell the reader much -- particularly
> if it is non-standard.
All these PNPids are from BIOS, and we expose them to userspace
directly.
Now all ACPI devices are registered before any ACPI driver is found. So
how can we recognize the platform specific devices?
> But if we are not going to stay within the standard format and we are going to blow the 8-character
> convention, then I vote for strings that actually make sense to human,
> say something like this:
> 
> #define ACPI_POWER_HID                  "power_resource"
> #define ACPI_PROCESSOR_HID              "processor"
> #define ACPI_SYSTEM_HID                 "system"
> #define ACPI_THERMAL_HID                "thermal"
> #define ACPI_BUTTON_HID_POWERF          "button_power_fixed"
> #define ACPI_BUTTON_HID_SLEEPF          "button_sleep_fixed"
That's true. I'll convert these fake PNPids to more interesting strings.

Then I have an idea about the other ones. We can also convert the PNPids
reserved in the spec to such kind of strings.
E.g.	"PNP0C0D,PNP0C0C,PNP0C0E"	"button"
	"ACPI0003"			"ac_adapter"
	"PNP0C0A"			"battery"
....
We can use an array to maintain the name mapping between PNPid and
bus_id.
Like this:
struct acpi_name_mapping {
 char PNPid[9];	/*both fake PNPids and PNPids reserved by the spec*/
 char bus_id[20]; /* names expose to userspace */
 int instance_no;
}
static struct acpi_name_mapping acpi_name_mapping_table[] = {
	{"PNP0C0A", "battery",0},
	{"ACPI_PWER_HID", "powe_resource",0},
	....
	NULL,
};
Then we can expose such kind of strings to userspace instead of PNPids.
Though it's more friendly, the load is heavy.
The problem is whether it's worth doing.

Re: sysfs regression on Supermicro X7DB8+

[Bjorn Helgaas]

On Thursday 21 December 2006 20:12, Zhang Rui wrote:
> Then I have an idea about the other ones. We can also convert the PNPids
> reserved in the spec to such kind of strings.
> E.g.	"PNP0C0D,PNP0C0C,PNP0C0E"	"button"
> 	"ACPI0003"			"ac_adapter"
> 	"PNP0C0A"			"battery"

I hesitate to hide the PNP IDs altogether.  They seem analogous
to PCI vendor/device IDs.  We expose the PCI IDs directly and
let user-space map them into human-readable strings.  In fact,
the mostly-forgotten lspnp package already has a pnp.ids file
with these mappings.  So I vote for keeping the mapping there.

Re: sysfs regression on Supermicro X7DB8+

[Len Brown]

On Thursday 21 December 2006 23:10, Bjorn Helgaas wrote:
> On Thursday 21 December 2006 20:12, Zhang Rui wrote:
> > Then I have an idea about the other ones. We can also convert the PNPids
> > reserved in the spec to such kind of strings.
> > E.g.	"PNP0C0D,PNP0C0C,PNP0C0E"	"button"
> > 	"ACPI0003"			"ac_adapter"
> > 	"PNP0C0A"			"battery"
> 
> I hesitate to hide the PNP IDs altogether.  They seem analogous
> to PCI vendor/device IDs.  We expose the PCI IDs directly and
> let user-space map them into human-readable strings.  In fact,
> the mostly-forgotten lspnp package already has a pnp.ids file
> with these mappings.  So I vote for keeping the mapping there.

I agree with Bjorn that it is a slippery slope for the kernel to try to be human friendly,
and that the kernel should just give the raw names and let an application translate them.

I think my original point was somewhat mis-interpreted.
My point is that when the kernel has _no_ PNPid to use to describe the device node
and we have to manufacture a string anyway, that we might as well manufacture
a string that a human can read.

thanks,
-Len

Re: sysfs regression on Supermicro X7DB8+

[Len Brown]

On Thursday 21 December 2006 22:12, Zhang Rui wrote:
> > > > What is "PNPIDNON" (below) supposed to mean?
> > > > 
> > > For devices with PNPID, we use PNPID:instance_no as the bus_id.
> > > But for others without PNPID, we set PNPIDNON:instance_no as their
> > > bus_id instead.
> > 
> > There must be a better way to name a device that has no  PNPid   than PNPIDNON:#
> > 
> > How about "device:#"?
> Sounds good. Will do.
> > Also,  this for the first time exposes the internal fake ones
> > 
> > /* _HID definitions */
> > 
> > #define ACPI_POWER_HID                  "ACPI_PWR"
> > #define ACPI_PROCESSOR_HID              "ACPI_CPU"
> > #define ACPI_SYSTEM_HID                 "ACPI_SYS"
> > #define ACPI_THERMAL_HID                "ACPI_THM"
> > #define ACPI_BUTTON_HID_POWERF          "ACPI_FPB"
> > #define ACPI_BUTTON_HID_SLEEPF          "ACPI_FSB"
> > 
> > If we are going to expose these, then we should spend a moment to think about how
> > understandable they'll be appearing in the file system.  ACPI_FSB doesn't bring
> > "Fixed Function Sleep Button" to mind when I see it...
> > 
> > Any reason we can't re-use "ACPI0007" for ACPI_PROCESSOR_HID
> > It is already reserved in the spec.
> > 
> > BTW. anybody know what PNP0C08 is supposed to be?
> > Seems it is reserved OS use to describe generic ACPI resources
> > that are not associated with a particular device.
> > 
> > Note that while I don't think we are constrained here, standard PNP-id's must be of the form AAA####
> > where the A's are letters and the #'s are hex.  ACPI-id's must be of the form ACPI####
> > where #'s are again hex.  It would be nice if we could work within those constraints
> > and not conflict with the standard in case something is expecting those formats.
> > eg. IBM specific devices are IBM####, Toshiba are TOS####.
> > But the down-side is that the #### doesn't tell the reader much -- particularly
> > if it is non-standard.

> All these PNPids are from BIOS, and we expose them to userspace
> directly.

right, as we should.  I described the vendor specific HIDs here just as an
example of the format.  Eg. we could do LNX#### and fit within that
naming scheme if we wanted to -- but I like the English word scheme better.

> Now all ACPI devices are registered before any ACPI driver is found. So
> how can we recognize the platform specific devices?

We don't.
These are just like PNP#### -- the are only recognized if a driver binds
to that ID.


> > But if we are not going to stay within the standard format and we are going to blow the 8-character
> > convention, then I vote for strings that actually make sense to human,
> > say something like this:
> > 
> > #define ACPI_POWER_HID                  "power_resource"
> > #define ACPI_PROCESSOR_HID              "processor"
> > #define ACPI_SYSTEM_HID                 "system"
> > #define ACPI_THERMAL_HID                "thermal"
> > #define ACPI_BUTTON_HID_POWERF          "button_power_fixed"
> > #define ACPI_BUTTON_HID_SLEEPF          "button_sleep_fixed"
> That's true. I'll convert these fake PNPids to more interesting strings.

Okay, thanks.
 
> Then I have an idea about the other ones. We can also convert the PNPids
> reserved in the spec to such kind of strings.
> E.g.	"PNP0C0D,PNP0C0C,PNP0C0E"	"button"
> 	"ACPI0003"			"ac_adapter"
> 	"PNP0C0A"			"battery"
> ....
> We can use an array to maintain the name mapping between PNPid and
> bus_id.
> Like this:
> struct acpi_name_mapping {
>  char PNPid[9];	/*both fake PNPids and PNPids reserved by the spec*/
>  char bus_id[20]; /* names expose to userspace */
>  int instance_no;
> }
> static struct acpi_name_mapping acpi_name_mapping_table[] = {
> 	{"PNP0C0A", "battery",0},
> 	{"ACPI_PWER_HID", "powe_resource",0},
> 	....
> 	NULL,
> };
> Then we can expose such kind of strings to userspace instead of PNPids.
> Though it's more friendly, the load is heavy.
> The problem is whether it's worth doing.

While yes, it is sort of tempting to make all the buttons look like "button"
there is only a small list of button types and I think a user/application
can handle the real names.

The real deal killer to putting translation in the kernel is that even if we
could translate the official PNPid list in the kernel (itself a bad idea),
we could never grok the vendor specific Ids -- since even when we
know about them we usually don't even know what they are.

So lets stick to the plan of exposing the platform provided IDs
and manufacturing additional ones when we need them and
the platform doesn't supply them.

thanks,
-Len