Skip to contentSkip to conversation

convert ACPI driver model to Linux driver model - takes 2

RFC In review Shaohua Li 2 participants 5 messages Oct 13, 2005 – Oct 20, 2005 3 messages not in the archive

Subsystems (3)
lore
Share & tools
lore

Shaohua Li

author
#0 lore raw

RFC 1/1 convert ACPI driver model to Linux driver model - takes 2

Hi,
This is the updated patch for the topic. In this version, I completely convert devices in ACPI bus into Linux driver model. Now the device driver for ACPI bus device should be a 'struct acpi_phys_driver'. Old style driver (struct acpi_driver) now can't work for ACPI bus devices. Here I still regard devices listed in DSDT but not belonging to any physical bus as ACPI bus devices.
For back compatibility, old style driver model still applies to non-ACPI bus devices. In the implementation, we register an old style driver (acpi_dummy) for each acpi bus device, whose goal is to enumerate acpi bus devices and prevent new old style driver binding with the devices. I tried to convert one ACPI driver (link device driver) to the new model. It's quite easy.
I'm looking forward to your comments/suggestions.

Thanks,
Shaohua
---

 linux-2.6.14-rc3-root/drivers/acpi/scan.c     |  231 +++++++++++++++++++++++++-
 linux-2.6.14-rc3-root/include/acpi/acpi_bus.h |   34 +++
 2 files changed, 263 insertions(+), 2 deletions(-)
drivers/acpi/scan.c renamed from drivers/acpi/scan.c~acpi_bus +230 -1
diff -puN drivers/acpi/scan.c~acpi_bus drivers/acpi/scan.c
@@ -112,6 +112,234 @@ static struct kset acpi_namespace_kset =
 	.hotplug_ops = &namespace_hotplug_ops,
 };
 
+/* ACPI phys bus */
+static int acpi_phys_bus_match(struct device *dev, struct device_driver *drv)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+	struct acpi_phys_driver *phys_drv = to_acpi_phys_drv(drv);
+	return !acpi_match_ids(phys_dev->acpi_dev, phys_drv->ids);
+}
+
+static int acpi_phys_bus_hotplug(struct device *dev, char **envp,
+	int num_envp, char *buffer, int buffer_size)
+{
+	int i = 0;
+	struct acpi_device *acpi_dev = to_acpi_phys_dev(dev)->acpi_dev;
+	char *scratch;
+	int length = 0;
+	struct acpi_compatible_id_list *cid_list;
+
+	scratch = buffer;
+	if (acpi_dev->flags.hardware_id) {
+		envp[i++] = scratch;
+		length += scnprintf(scratch, buffer_size - length, "HWID=%s",
+			acpi_dev->pnp.hardware_id);
+		if ((buffer_size - length <=0) || (i > num_envp))
+			return -ENOMEM;
+		++length;
+		scratch += length;
+	}
+	if (acpi_dev->flags.compatible_ids) {
+		int j;
+		cid_list = acpi_dev->pnp.cid_list;
+
+		/* multiple _CID entries */
+		for (j = 0; j < cid_list->count; j++)
+		{
+			envp[i++] = scratch;
+			length += scnprintf(scratch, buffer_size - length,
+				"COMPTID=%s", cid_list->id[j].value);
+			if ((buffer_size - length <=0) || (i > num_envp))
+				return -ENOMEM;
+			++length;
+			scratch += length;
+		}
+	}
+	envp[i] = NULL;
+
+	return 0;
+}
+
+static int acpi_phys_bus_suspend(struct device * dev, pm_message_t state)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+
+	if (!phys_dev->driver || !phys_dev->driver->suspend)
+		return 0;
+	return phys_dev->driver->suspend(phys_dev, state);
+}
+
+static int acpi_phys_bus_resume(struct device * dev)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+
+	if (!phys_dev->driver || !phys_dev->driver->resume)
+		return 0;
+	return phys_dev->driver->resume(phys_dev);
+}
+
+static struct bus_type acpi_phys_bus_type = {
+	.name = "acpi",
+	.match = acpi_phys_bus_match,
+	.hotplug = acpi_phys_bus_hotplug,
+	.suspend = acpi_phys_bus_suspend,
+	.resume = acpi_phys_bus_resume,
+};
+
+static LIST_HEAD(acpi_phys_devices);
+static DEFINE_SPINLOCK(acpi_phys_bus_lock);
+static struct acpi_phys_device *phys_root;
+
+static void acpi_phys_device_release(struct device *dev)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+	kfree(phys_dev);
+}
+
+static int acpi_phys_bus_add(struct acpi_device *acpi_dev)
+{
+	struct acpi_phys_device *phys_dev;
+
+	phys_dev = kzalloc(sizeof(struct acpi_phys_device), GFP_KERNEL);
+	if (!phys_dev)
+		return -ENOMEM;
+
+	spin_lock(&acpi_phys_bus_lock);
+	list_add_tail(&phys_dev->node, &acpi_phys_devices);
+	spin_unlock(&acpi_phys_bus_lock);
+
+	phys_dev->acpi_dev = acpi_dev;
+	phys_dev->phys_dev.parent = &phys_root->phys_dev;
+	phys_dev->phys_dev.bus = &acpi_phys_bus_type;
+	device_initialize(&phys_dev->phys_dev);
+	sprintf(phys_dev->phys_dev.bus_id, "%s", acpi_dev->pnp.bus_id);
+	phys_dev->phys_dev.release = acpi_phys_device_release;
+	device_add(&phys_dev->phys_dev);
+	return 0;
+}
+
+static int acpi_phys_bus_remove(struct acpi_device *acpi_dev, int type)
+{
+	struct acpi_phys_device *phys_dev, *tmp, *find = NULL;
+
+	spin_lock(&acpi_phys_bus_lock);
+	list_for_each_entry_safe(phys_dev, tmp, &acpi_phys_devices, node) {
+		if (phys_dev->acpi_dev == acpi_dev) {
+			find = phys_dev;
+			list_del(&find->node);
+			break;
+		}
+	}
+	spin_unlock(&acpi_phys_bus_lock);
+	if (find)
+		device_unregister(&find->phys_dev);
+	return 0;
+}
+
+/* this driver is linking acpi device with acpi phys device */
+static struct acpi_driver dummy_driver = {
+	.name = "acpi_dummy",
+	/* List all ids of ACPI phys devices */
+	.ids =
+//		"PNP0C09," /* EC */
+//		"ACPI0003," /* AC */
+//		"PNP0C0F," /* Link */
+//		"PNP0C0A," /* battery */
+//		"PNP0C0C,PNP0C0E,PNP0C0D,ACPI_FPB,ACPI_FSB," /* button */
+//		"PNP0C0B," /* fan */
+//		"PNP0A03," /* PCI root */
+//		ACPI_POWER_HID"," /* power */
+//		ACPI_PROCESSOR_HID"," /* CPU */
+//		ACPI_THERMAL_HID"," /* thermal */
+		"",
+	.ops = {
+		.add = acpi_phys_bus_add,
+		.remove = acpi_phys_bus_remove,
+	},
+};
+
+static int __init acpi_phys_bus_init(void)
+{
+	bus_register(&acpi_phys_bus_type);
+
+	phys_root = kzalloc(sizeof(struct acpi_phys_device), GFP_KERNEL);
+	if (!phys_root)
+		return -ENOMEM;
+
+	phys_root->acpi_dev = acpi_root;
+	phys_root->phys_dev.release = acpi_phys_device_release;
+	sprintf(phys_root->phys_dev.bus_id, "%s", acpi_root->pnp.bus_id);
+	device_register(&phys_root->phys_dev);
+
+	/* enumerate devices */
+	acpi_bus_register_driver(&dummy_driver);
+	return 0;
+}
+
+
+static int acpi_phys_device_probe(struct device *dev)
+{
+	int error = -ENODEV;
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+	struct acpi_phys_driver *phys_drv = to_acpi_phys_drv(dev->driver);
+
+	get_device(dev);
+	if (!phys_dev->driver && phys_drv->probe) {
+		error = phys_drv->probe(phys_dev);
+		if (error >= 0)
+			phys_dev->driver = phys_drv;
+	}
+	if (error < 0)
+		put_device(dev);
+	return error;
+}
+
+static int acpi_phys_device_remove(struct device * dev)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+	struct acpi_phys_driver *phys_drv = phys_dev->driver;
+
+	if (phys_drv) {
+		if (phys_drv->remove)
+			phys_drv->remove(phys_dev);
+		phys_dev->driver = NULL;
+	}
+	put_device(dev);
+	return 0;
+}
+
+static void acpi_phys_device_shutdown(struct device *dev)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+
+	if (!phys_dev->driver || !phys_dev->driver->shutdown)
+		return;
+
+	phys_dev->driver->shutdown(phys_dev);
+}
+
+int acpi_phys_driver_register(struct acpi_phys_driver *drv)
+{
+	int error;
+	/*FIXME: add to a list */
+	drv->phys_drv.name = drv->name;
+	drv->phys_drv.bus = &acpi_phys_bus_type;
+	drv->phys_drv.probe = acpi_phys_device_probe;
+	drv->phys_drv.remove = acpi_phys_device_remove;
+	drv->phys_drv.shutdown = acpi_phys_device_shutdown;
+	drv->phys_drv.owner = drv->owner;
+	error = driver_register(&drv->phys_drv);
+	return error;
+}
+EXPORT_SYMBOL(acpi_phys_driver_register);
+
+void acpi_phys_driver_unregister(struct acpi_phys_driver *drv)
+{
+	driver_unregister(&drv->phys_drv);
+}
+EXPORT_SYMBOL(acpi_phys_driver_unregister);
+/*-----------------------------------*/
+
 static void acpi_device_register(struct acpi_device *device,
 				 struct acpi_device *parent)
 {
@@ -1400,8 +1628,9 @@ static int __init acpi_scan_init(void)
 
 	if (result)
 		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);
+	acpi_phys_bus_init();
 
-      Done:
+Done:
 	return_VALUE(result);
 }
 
include/acpi/acpi_bus.h renamed from include/acpi/acpi_bus.h~acpi_bus +33 -1
diff -puN include/acpi/acpi_bus.h~acpi_bus include/acpi/acpi_bus.h
@@ -28,6 +28,7 @@
 
 #include <linux/kobject.h>
 
+#include <linux/device.h>
 #include <acpi/acpi.h>
 
 #define PREFIX			"ACPI: "
@@ -300,6 +301,38 @@ struct acpi_device {
 
 #define acpi_driver_data(d)	((d)->driver_data)
 
+/* acpi bus type for real ACPI device */
+struct acpi_phys_device {
+	struct list_head node;
+	struct acpi_device *acpi_dev;
+	struct device phys_dev;
+	struct acpi_phys_driver *driver;
+	void *driver_data;
+};
+
+#define acpi_phys_driver_data(d) ((d)->driver_data)
+
+struct acpi_phys_driver {
+//	struct list_head node;
+	char name[80];
+	char class[80];
+	struct module *owner;
+	char *ids;
+	int (*probe)(struct acpi_phys_device *dev);
+	void (*remove) (struct acpi_phys_device *dev);
+	int  (*suspend) (struct acpi_phys_device *dev, pm_message_t state);
+	int  (*resume) (struct acpi_phys_device *dev);
+	void (*shutdown) (struct acpi_phys_device *dev);
+
+	struct device_driver    phys_drv;
+};
+
+#define to_acpi_phys_dev(n) container_of(n, struct acpi_phys_device, phys_dev)
+#define to_acpi_phys_drv(n) container_of(n, struct acpi_phys_driver, phys_drv)
+int acpi_phys_driver_register(struct acpi_phys_driver *drv);
+void acpi_phys_driver_unregister(struct acpi_phys_driver *drv);
+
+
 /*
  * Events
  * ------
@@ -339,7 +372,6 @@ void acpi_remove_dir(struct acpi_device 
 /*
  * Bind physical devices with ACPI devices
  */
-#include <linux/device.h>
 struct acpi_bus_type {
 	struct list_head list;
 	struct bus_type *bus;

_

-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions, and more. http://solutions.newsforge.com/ibmarch.tmpl

A message that is not in the archive (sent off-list or not yet received): 1129185454.9074.15.camel-ECwVeV2eNyQD0+JXs3kMbRL4W9x8LtSr@public.gmane.org

#2 lore raw
Quoted 2 lines · Shaohua Li

This is the updated patch for the topic. In this version, I completely convert devices in ACPI bus into Linux driver model.

This looks like a nice piece of work.

Quoted 3 lines

Now the device
driver for ACPI bus device should be a 'struct acpi_phys_driver'. Old style driver (struct acpi_driver) now can't work for ACPI bus devices.

You now have both 'acpi_driver' and 'acpi_phys_driver' (and 'acpi_device' and 'acpi_phys_device'). What exactly is the distinction? Is the distinction necessary? Can you just stuff the generic device stuff into 'struct acpi_device' itself?

Quoted 2 lines

I tried to convert one ACPI driver (link device driver) to the new model. It's quite easy.

I'd like to see the link device driver patch, just to help me understand all this.

include/acpi/acpi_bus.h struct acpi_device { · line 318

Quoted 4 lines
include/acpi/acpi_bus.h renamed from include/acpi/acpi_bus.h~acpi_bus +4 -0
+struct acpi_phys_driver {
+//	struct list_head node;
+	char name[80];
+	char class[80];

Shouldn't these be "char *" instead of "char []"?

include/acpi/acpi_bus.h struct acpi_device { · line 327

Quoted 9 lines
include/acpi/acpi_bus.h renamed from include/acpi/acpi_bus.h~acpi_bus +9 -0
+	struct module *owner;
+	char *ids;
+	int (*probe)(struct acpi_phys_device *dev);
+	void (*remove) (struct acpi_phys_device *dev);
+	int  (*suspend) (struct acpi_phys_device *dev, pm_message_t state);
+	int  (*resume) (struct acpi_phys_device *dev);
+	void (*shutdown) (struct acpi_phys_device *dev);
+
+	struct device_driver    phys_drv;

I think you should name this item "driver", not "phys_drv".

-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions, and more. http://solutions.newsforge.com/ibmarch.tmpl

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 9 lines · Bjorn Helgaas
Quoted 1 line · Shaohua Li

This is the updated patch for the topic. In this version, I

completely

Quoted 1 line

convert devices in ACPI bus into Linux driver model.

This looks like a nice piece of work.

Quoted 2 lines

Now the device
driver for ACPI bus device should be a 'struct acpi_phys_driver'.

Quoted 8 lines

Old

Quoted 1 line

style driver (struct acpi_driver) now can't work for ACPI bus

devices.

You now have both 'acpi_driver' and 'acpi_phys_driver' (and 'acpi_device' and 'acpi_phys_device'). What exactly is the distinction? Is the distinction necessary? Can you just stuff the generic device stuff into 'struct acpi_device' itself?

Devices in ACPI bus use acpi_phys_driver, other devices use acpi_driver. Converting acpi_device use the generic device stuff might be difficult, since many devices in DSDT aren't ACPI bus devices. The distinction is to do smooth change. That is non ACPI bus devices still can register old style driver. On the future, we might remove the old style driver model. Adam suggested non-acpi bus device directly use ACPI services instead of register a 'acpi_driver', which might be worthy tring.

Quoted 6 lines
Quoted 3 lines

I tried to convert one ACPI driver (link device driver) to the new model. It's quite easy.

I'd like to see the link device driver patch, just to help me understand all this.

Ok, please see attached patch.

Quoted 6 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+struct acpi_phys_driver { 
+//   struct list_head node; 
+     char name[80]; 
+     char class[80];

Shouldn't these be "char *" instead of "char []"?

I'll do code cleanup, if people agree the basic idea.

Quoted 6 lines

Review

Quoted 6 lines
(quoted diff) +5 -0
+     struct module *owner; 
+     char *ids; 
+     int (*probe)(struct acpi_phys_device *dev); 
+     void (*remove) (struct acpi_phys_device *dev); 
+     int  (*suspend) (struct acpi_phys_device *dev, pm_message_t
Quoted 7 lines

state);

Review

Quoted 4 lines
(quoted diff) +4 -0
+     int  (*resume) (struct acpi_phys_device *dev); 
+     void (*shutdown) (struct acpi_phys_device *dev); 
+ 
+     struct device_driver    phys_drv;

I think you should name this item "driver", not "phys_drv".

Ditto.

Thanks,
Shaohua

---

 linux-2.6.14-rc3-root/drivers/acpi/osl.c      |    6 -
 linux-2.6.14-rc3-root/drivers/acpi/pci_link.c |  106 +++++++-------------------
 2 files changed, 33 insertions(+), 79 deletions(-)
drivers/acpi/osl.c renamed from drivers/acpi/osl.c~pci_link_convert +1 -5
diff -puN drivers/acpi/osl.c~pci_link_convert drivers/acpi/osl.c
@@ -136,13 +136,9 @@ void acpi_os_vprintf(const char *fmt, va
 #endif
 }
 
-extern int acpi_in_resume;
 void *acpi_os_allocate(acpi_size size)
 {
-	if (acpi_in_resume)
-		return kmalloc(size, GFP_ATOMIC);
-	else
-		return kmalloc(size, GFP_KERNEL);
+	return kmalloc(size, GFP_KERNEL);
 }
 
 void acpi_os_free(void *ptr)
drivers/acpi/pci_link.c renamed from drivers/acpi/pci_link.c~pci_link_convert +32 -74

152 lines not on this page · raw message

_

A message that is not in the archive (sent off-list or not yet received): 1129252436.3948.12.camel-ECwVeV2eNyQD0+JXs3kMbRL4W9x8LtSr@public.gmane.org

#6 lore raw
Quoted 7 lines · Bjorn Helgaas
Quoted 4 lines

You now have both 'acpi_driver' and 'acpi_phys_driver' (and 'acpi_device' and 'acpi_phys_device'). What exactly is the distinction? Is the distinction necessary? Can you just stuff the generic device stuff into 'struct acpi_device' itself?

Devices in ACPI bus use acpi_phys_driver, other devices use acpi_driver.

I'm sorry to be dense, but this doesn't help me. What do you mean by "devices in ACPI bus" as opposed to other ACPI devices?

Let's say I want to write a driver for something in ACPI, like a fan. How do I decide whether to use acpi_phys_driver or acpi_driver?

Do you mean that I can use either one, and that you want to migrate drivers from acpi_driver to acpi_phys_driver over time?

If that's the case, I think we should end up with acpi_driver, not acpi_phys_driver, because "phys" doesn't tell me anything useful. And there aren't very many ACPI drivers, so I don't really see the problem with just moving them all at once.

-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions, and more. http://solutions.newsforge.com/ibmarch.tmpl

#7 lore raw
In reply to a message not in the archive
Quoted 4 lines · Shaohua Li

This is the updated patch for the topic. In this version, I completely convert devices in ACPI bus into Linux driver model. Now the device driver for ACPI bus device should be a 'struct acpi_phys_driver'. Old style driver (struct acpi_driver) now can't work for ACPI bus devices.

- I don't like names that include "acpi_bus" because there really is no "bus". If PCI, which really DOES have a bus, doesn't need it, ACPI surely doesn't.

- Suspend/resume/probe/remove/shutdown are device-oriented interfaces and should be named with "device", not "bus".

- We should not end up with two interfaces to register ACPI drivers. ACPI is confusing enough as it is. Having both acpi_phys_driver_register() and acpi_bus_register_driver() will be a disaster. (Keeping both temporarily for during a transition is OK, but one should be explicitly deprecated.)

drivers/acpi/scan.c static struct kset acpi_namespace_kset = · line 151

Quoted 2 lines
drivers/acpi/scan.c renamed from drivers/acpi/scan.c~acpi_bus +2 -0
+			length += scnprintf(scratch, buffer_size - length,
+				"COMPTID=%s", cid_list->id[j].value);

This requires a sequence number (COMPTID%d) to support multiple _CIDs, doesn't it?

drivers/acpi/scan.c static struct kset acpi_namespace_kset = · line 184

Quoted 4 lines
drivers/acpi/scan.c renamed from drivers/acpi/scan.c~acpi_bus +4 -0
+static struct bus_type acpi_phys_bus_type = {
+	.name = "acpi",
+	.match = acpi_phys_bus_match,
+	.hotplug = acpi_phys_bus_hotplug,

I think this should be just "acpi_hotplug".

drivers/acpi/scan.c static struct kset acpi_namespace_kset = · line 186

Quoted 2 lines
drivers/acpi/scan.c renamed from drivers/acpi/scan.c~acpi_bus +2 -0
+	.suspend = acpi_phys_bus_suspend,
+	.resume = acpi_phys_bus_resume,

And these should be "acpi_device_suspend" and "acpi_device_resume".

Quoted 1 line

+int acpi_phys_driver_register(struct acpi_phys_driver *drv)

The majority of driver registration interfaces use "register_driver", not "driver_register". So I think this should be "acpi_register_driver", and you should deprecate "acpi_bus_register_driver". I still don't see any fundamental reason for having two ways to register ACPI drivers.

drivers/acpi/scan.c static struct kset acpi_namespace_kset = · line 329

Quoted 8 lines
drivers/acpi/scan.c renamed from drivers/acpi/scan.c~acpi_bus +8 -0
+{
+	int error;
+	/*FIXME: add to a list */
+	drv->phys_drv.name = drv->name;
+	drv->phys_drv.bus = &acpi_phys_bus_type;
+	drv->phys_drv.probe = acpi_phys_device_probe;
+	drv->phys_drv.remove = acpi_phys_device_remove;
+	drv->phys_drv.shutdown = acpi_phys_device_shutdown;

I think these should be "acpi_bus_type", "acpi_device_probe", "acpi_device_remove", and "acpi_device_shutdown".

-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions, and more. http://solutions.newsforge.com/ibmarch.tmpl