On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > Introduce struct acpi_scan_handler for representing objects that > will do configuration tasks depending on ACPI device nodes' > hardware IDs (HIDs). > > Currently, those tasks are done either directly by the ACPI namespace > scanning code or by ACPI device drivers designed specifically for > this purpose. None of the above is desirable, however, because > doing that directly in the namespace scanning code makes that code > overly complicated and difficult to follow and doing that in > "special" device drivers leads to a great deal of confusion about > their role and to confusing interactions with the driver core (for > example, sysfs directories are created for those drivers, but they > are completely unnecessary and only increase the kernel's memory > footprint in vain). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > --- > Documentation/acpi/scan_handlers.txt | 77 +++++++++++++++++++++++++++++++++++ > drivers/acpi/scan.c | 60 ++++++++++++++++++++++++--- > include/acpi/acpi_bus.h | 14 ++++++ > 3 files changed, 144 insertions(+), 7 deletions(-) > > Index: test/include/acpi/acpi_bus.h > =================================================================== > --- test.orig/include/acpi/acpi_bus.h > +++ test/include/acpi/acpi_bus.h > @@ -84,6 +84,18 @@ struct acpi_driver; > struct acpi_device; > > /* > + * ACPI Scan Handler > + * ----------------- > + */ > + > +struct acpi_scan_handler { > + const struct acpi_device_id *ids; > + struct list_head list_node; > + int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id); > + void (*detach)(struct acpi_device *dev); > +}; > + > +/* > * ACPI Driver > * ----------- > */ > @@ -269,6 +281,7 @@ struct acpi_device { > struct acpi_device_wakeup wakeup; > struct acpi_device_perf performance; > struct acpi_device_dir dir; > + struct acpi_scan_handler *handler; > struct acpi_driver *driver; > void *driver_data; > struct device dev; > @@ -382,6 +395,7 @@ int acpi_bus_receive_event(struct acpi_b > static inline int acpi_bus_generate_proc_event(struct acpi_device *device, u8 type, int data) > { return 0; } > #endif > +int acpi_scan_add_handler(struct acpi_scan_handler *handler); > int acpi_bus_register_driver(struct acpi_driver *driver); > void acpi_bus_unregister_driver(struct acpi_driver *driver); > int acpi_bus_scan(acpi_handle handle); > Index: test/drivers/acpi/scan.c > =================================================================== > --- test.orig/drivers/acpi/scan.c > +++ test/drivers/acpi/scan.c > @@ -53,6 +53,7 @@ static const struct acpi_device_id acpi_ > static LIST_HEAD(acpi_device_list); > static LIST_HEAD(acpi_bus_id_list); > static DEFINE_MUTEX(acpi_scan_lock); > +static LIST_HEAD(acpi_scan_handlers_list); > DEFINE_MUTEX(acpi_device_lock); > LIST_HEAD(acpi_wakeup_device_list); > > @@ -62,6 +63,15 @@ struct acpi_device_bus_id{ > struct list_head node; > }; > > +int acpi_scan_add_handler(struct acpi_scan_handler *handler) > +{ > + if (!handler || !handler->attach) > + return -EINVAL; > + > + list_add_tail(&handler->list_node, &acpi_scan_handlers_list); > + return 0; > +} > + > /* > * Creates hid/cid(s) string needed for modalias and uevent > * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get: > @@ -1570,20 +1580,42 @@ static acpi_status acpi_bus_check_add(ac > return AE_OK; > } > > +static int acpi_scan_attach_handler(struct acpi_device *device) > +{ > + struct acpi_scan_handler *handler; > + int ret = 0; > + > + list_for_each_entry(handler, &acpi_scan_handlers_list, list_node) { > + const struct acpi_device_id *id; > + > + id = __acpi_match_device(device, handler->ids); > + if (!id) > + continue; > + > + ret = handler->attach(device, id); > + if (ret > 0) { > + device->handler = handler; > + break; > + } else if (ret < 0) { > + break; > + } > + } > + return ret; > +} Now that we have full control over the attach logic, it would be great if we can update it to match with the ACPI spec -- HID has priority over CIDs, and CIDs are also listed in their priority. For example, Device-X has HID and CID. In this case, this Device-X should be attached to Handler-A since it supports HID. The current logic simply chooses a handler whichever registered before. Device-X: HID PNPID-A, CID PNPID-B Handler-A: PNPID-A Handler-B: PNPID-B So, the attach logic should be something like: list_for_each_entry(hwid, acpi_device->pnp.ids,) { list_for_each_entry(,&acpi_scan_handlers_list,) check if this handler supports a given hwid } Thanks, -Toshi > + > static acpi_status acpi_bus_device_attach(acpi_handle handle, u32 lvl_not_used, > void *not_used, void **ret_not_used) > { > const struct acpi_device_id *id; > - acpi_status status = AE_OK; > struct acpi_device *device; > unsigned long long sta_not_used; > - int type_not_used; > + int ret; > > /* > * Ignore errors ignored by acpi_bus_check_add() to avoid terminating > * namespace walks prematurely. > */ > - if (acpi_bus_type_and_status(handle, &type_not_used, &sta_not_used)) > + if (acpi_bus_type_and_status(handle, &ret, &sta_not_used)) > return AE_OK; > > if (acpi_bus_get_device(handle, &device)) > @@ -1593,10 +1625,15 @@ static acpi_status acpi_bus_device_attac > if (id) { > /* This is a known good platform device. */ > acpi_create_platform_device(device, id->driver_data); > - } else if (device_attach(&device->dev) < 0) { > - status = AE_CTRL_DEPTH; > + return AE_OK; > } > - return status; > + > + ret = acpi_scan_attach_handler(device); > + if (ret) > + return ret > 0 ? AE_OK : AE_CTRL_DEPTH; > + > + ret = device_attach(&device->dev); > + return ret >= 0 ? AE_OK : AE_CTRL_DEPTH; > } > > /** > @@ -1639,8 +1676,17 @@ static acpi_status acpi_bus_device_detac > struct acpi_device *device = NULL; > > if (!acpi_bus_get_device(handle, &device)) { > + struct acpi_scan_handler *dev_handler = device->handler; > + > device->removal_type = ACPI_BUS_REMOVAL_EJECT; > - device_release_driver(&device->dev); > + if (dev_handler) { > + if (dev_handler->detach) > + dev_handler->detach(device); > + > + device->handler = NULL; > + } else { > + device_release_driver(&device->dev); > + } > } > return AE_OK; > } > Index: test/Documentation/acpi/scan_handlers.txt > =================================================================== > --- /dev/null > +++ test/Documentation/acpi/scan_handlers.txt > @@ -0,0 +1,77 @@ > +ACPI Scan Handlers > + > +Copyright (C) 2012, Intel Corporation > +Author: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > + > +During system initialization and ACPI-based device hot-add, the ACPI namespace > +is scanned in search of device objects that generally represent various pieces > +of hardware. This causes a struct acpi_device object to be created and > +registered with the driver core for every device object in the ACPI namespace > +and the hierarchy of those struct acpi_device objects reflects the namespace > +layout (i.e. parent device objects in the namespace are represented by parent > +struct acpi_device objects and analogously for their children). Those struct > +acpi_device objects are referred to as "device nodes" in what follows, but they > +should not be confused with struct device_node objects used by the Device Trees > +parsing code (although their role is analogous to the role of those objects). > + > +During ACPI-based device hot-remove device nodes representing pieces of hardware > +being removed are unregistered and deleted. > + > +The core ACPI namespace scanning code in drivers/acpi/scan.c carries out basic > +initialization of device nodes, such as retrieving common configuration > +information from the device objects represented by them and populating them with > +appropriate data, but some of them require additional handling after they have > +been registered. For example, if the given device node represents a PCI host > +bridge, its registration should cause the PCI bus under that bridge to be > +enumerated and PCI devices on that bus to be registered with the driver core. > +Similarly, if the device node represents a PCI interrupt link, it is necessary > +to configure that link so that the kernel can use it. > + > +Those additional configuration tasks usually depend on the type of the hardware > +component represented by the given device node which can be determined on the > +basis of the device node's hardware ID (HID). They are performed by objects > +called ACPI scan handlers represented by the following structure: > + > +struct acpi_scan_handler { > + const struct acpi_device_id *ids; > + struct list_head list_node; > + int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id); > + void (*detach)(struct acpi_device *dev); > +}; > + > +where ids is the list of IDs of device nodes the given handler is supposed to > +take care of, list_node is the hook to the global list of ACPI scan handlers > +maintained by the ACPI core and the .attach() and .detach() callbacks are > +executed, respectively, after registration of new device nodes and before > +unregistration of device nodes the handler attached to previously. > + > +The namespace scanning function, acpi_bus_scan(), first registers all of the > +device nodes in the given namespace scope with the driver core. Then, it tries > +to match a scan handler against each of them using the ids arrays of the > +available scan handlers. If a matching scan handler is found, its .attach() > +callback is executed for the given device node. If that callback returns 1, > +that means that the handler has claimed the device node and is now responsible > +for carrying out any additional configuration tasks related to it. It also will > +be responsible for preparing the device node for unregistration in that case. > +The device node's handler field is then populated with the address of the scan > +handler that has claimed it. > + > +If the .attach() callback returns 0, it means that the device node is not > +interesting to the given scan handler and may be matched against the next scan > +handler in the list. If it returns a (negative) error code, that means that > +the namespace scan should be terminated due to a serious error. The error code > +returned should then reflect the type of the error. > + > +The namespace trimming function, acpi_bus_trim(), first executes .detach() > +callbacks from the scan handlers of all device nodes in the given namespace > +scope (if they have scan handlers). Next, it unregisters all of the device > +nodes in that scope. > + > +ACPI scan handlers can be added to the list maintained by the ACPI core with the > +help of the acpi_scan_add_handler() function taking a pointer to the new scan > +handler as an argument. The order in which scan handlers are added to the list > +is the order in which they are matched against device nodes during namespace > +scans. > + > +All scan handles must be added to the list before acpi_bus_scan() is run for the > +first time and they cannot be removed from it. > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html