| Message ID | 1408351.aVGQgvLeMo@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi Rafael, 2013/01/28 21:59, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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@intel.com> > --- Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> I have a comment. Please see below. > 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; > +} > + > 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; acpi_scan_attach_hanlder() returns only 0 or -EINVAL. How about just return AE_CTRL_DEPTH? Thanks, Yasuaki Ishimatsu > + > + 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@intel.com> > + > +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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/01/29 11:04, Yasuaki Ishimatsu wrote: > Hi Rafael, > > 2013/01/28 21:59, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> 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@intel.com> >> --- > Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> > > I have a comment. Please see below. > >> 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; >> +} >> + >> 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; > > acpi_scan_attach_hanlder() returns only 0 or -EINVAL. > How about just return AE_CTRL_DEPTH? I am wrong. Please forget it. Thanks, Yasuaki Ishimatsu > > Thanks, > Yasuaki Ishimatsu > >> + >> + 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@intel.com> >> + >> +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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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@intel.com> > --- > 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@intel.com> > + > +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, January 28, 2013 07:35:39 PM Toshi Kani wrote: > On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > 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@intel.com> > > --- > > 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 > } OK, I see the problem, but I think it's better to address it in a separate patch on top of the current series. I'm not sure what approach is best, though. Do you think there should be two passes the first of which will check HIDs only and the second one will check CIDs? Or do you have something different in mind? Rafael
On Tue, 2013-01-29 at 12:28 +0100, Rafael J. Wysocki wrote: > On Monday, January 28, 2013 07:35:39 PM Toshi Kani wrote: > > On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > 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@intel.com> > > > --- > > > 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 > > } > > OK, I see the problem, but I think it's better to address it in a separate > patch on top of the current series. I agree. > I'm not sure what approach is best, though. Do you think there should be two > passes the first of which will check HIDs only and the second one will check > CIDs? Or do you have something different in mind? HID and CIDs are already listed in their priority order in acpi_device->pnp.ids. So, the single pass like I described above should work. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, January 29, 2013 07:50:43 AM Toshi Kani wrote: > On Tue, 2013-01-29 at 12:28 +0100, Rafael J. Wysocki wrote: > > On Monday, January 28, 2013 07:35:39 PM Toshi Kani wrote: > > > On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > 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@intel.com> > > > > --- > > > > 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 > > > } > > > > OK, I see the problem, but I think it's better to address it in a separate > > patch on top of the current series. > > I agree. > > > I'm not sure what approach is best, though. Do you think there should be two > > passes the first of which will check HIDs only and the second one will check > > CIDs? Or do you have something different in mind? > > HID and CIDs are already listed in their priority order in > acpi_device->pnp.ids. So, the single pass like I described above should > work. Well, I'm not sure if I understand you correctly. The device is given and we need to find a handler for it. So, it looks like we first should check if any handler matches the HID. This has to be a pass through all handlers. If there's no match, we need to check if any handler matches any of the device IDs. That will be the second pass, won't it? The difficulty is that the first item in pnp.ids need not be *the* HID. It only will be the HID if ACPI_VALID_HID is set in the device info in acpi_device_set_id(). So perhaps we need to add a hid_valid bit in device->flags and only do the "HID pass" if that is set? Rafael
On Tue, 2013-01-29 at 22:32 +0100, Rafael J. Wysocki wrote: > On Tuesday, January 29, 2013 07:50:43 AM Toshi Kani wrote: > > On Tue, 2013-01-29 at 12:28 +0100, Rafael J. Wysocki wrote: > > > On Monday, January 28, 2013 07:35:39 PM Toshi Kani wrote: > > > > On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > 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@intel.com> > > > > > --- > > > > > 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 > > > > } > > > > > > OK, I see the problem, but I think it's better to address it in a separate > > > patch on top of the current series. > > > > I agree. > > > > > I'm not sure what approach is best, though. Do you think there should be two > > > passes the first of which will check HIDs only and the second one will check > > > CIDs? Or do you have something different in mind? > > > > HID and CIDs are already listed in their priority order in > > acpi_device->pnp.ids. So, the single pass like I described above should > > work. > > Well, I'm not sure if I understand you correctly. > > The device is given and we need to find a handler for it. So, it looks like > we first should check if any handler matches the HID. This has to be a pass > through all handlers. If there's no match, we need to check if any handler > matches any of the device IDs. That will be the second pass, won't it? acpi_device->pnp.ids has a list of HID->CID[0]->CID[1]..CID[n] since acpi_device_set_id() checks HID before CIDs. So, the first pass is to check with the first entry of pnp.ids. If no handler is found, then check the second entry, and so on. > The difficulty is that the first item in pnp.ids need not be *the* HID. > It only will be the HID if ACPI_VALID_HID is set in the device info in > acpi_device_set_id(). So perhaps we need to add a hid_valid bit in > device->flags and only do the "HID pass" if that is set? I do not think such bit is necessary for this. _HID is required (unless it has _ADR), but _CID is optional. So, the valid cases are that a device object has HID only, has both HID and CID(s), or has none of them (i.e. _ADR device). If there is a device with CID only, this is a FW bug and we just have to check with CID then. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, January 29, 2013 03:57:10 PM Toshi Kani wrote: > On Tue, 2013-01-29 at 22:32 +0100, Rafael J. Wysocki wrote: > > On Tuesday, January 29, 2013 07:50:43 AM Toshi Kani wrote: > > > On Tue, 2013-01-29 at 12:28 +0100, Rafael J. Wysocki wrote: > > > > On Monday, January 28, 2013 07:35:39 PM Toshi Kani wrote: > > > > > On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > 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@intel.com> > > > > > > --- > > > > > > 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 > > > > > } > > > > > > > > OK, I see the problem, but I think it's better to address it in a separate > > > > patch on top of the current series. > > > > > > I agree. > > > > > > > I'm not sure what approach is best, though. Do you think there should be two > > > > passes the first of which will check HIDs only and the second one will check > > > > CIDs? Or do you have something different in mind? > > > > > > HID and CIDs are already listed in their priority order in > > > acpi_device->pnp.ids. So, the single pass like I described above should > > > work. > > > > Well, I'm not sure if I understand you correctly. > > > > The device is given and we need to find a handler for it. So, it looks like > > we first should check if any handler matches the HID. This has to be a pass > > through all handlers. If there's no match, we need to check if any handler > > matches any of the device IDs. That will be the second pass, won't it? > > acpi_device->pnp.ids has a list of HID->CID[0]->CID[1]..CID[n] since > acpi_device_set_id() checks HID before CIDs. So, the first pass is to > check with the first entry of pnp.ids. If no handler is found, then > check the second entry, and so on. I'd prefer to check HID and then all CIDs at once in accordance with the following rule: 1) Use the first handler that matches HID exactly. If there's none: 2) Use the first handler compatible with the given device. > > The difficulty is that the first item in pnp.ids need not be *the* HID. > > It only will be the HID if ACPI_VALID_HID is set in the device info in > > acpi_device_set_id(). So perhaps we need to add a hid_valid bit in > > device->flags and only do the "HID pass" if that is set? > > I do not think such bit is necessary for this. _HID is required (unless > it has _ADR), but _CID is optional. So, the valid cases are that a > device object has HID only, has both HID and CID(s), or has none of them > (i.e. _ADR device). If there is a device with CID only, this is a FW > bug and we just have to check with CID then. In addition to that there's a number of objects that we assign artificial HIDs. I think we should treat them as CIDs rather than as real HIDs in this regard. Thanks, Rafael
On Wed, 2013-01-30 at 00:19 +0100, Rafael J. Wysocki wrote: > On Tuesday, January 29, 2013 03:57:10 PM Toshi Kani wrote: > > On Tue, 2013-01-29 at 22:32 +0100, Rafael J. Wysocki wrote: > > > On Tuesday, January 29, 2013 07:50:43 AM Toshi Kani wrote: > > > > On Tue, 2013-01-29 at 12:28 +0100, Rafael J. Wysocki wrote: > > > > > On Monday, January 28, 2013 07:35:39 PM Toshi Kani wrote: > > > > > > On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > > > 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@intel.com> > > > > > > > --- > > > > > > > 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 > > > > > > } > > > > > > > > > > OK, I see the problem, but I think it's better to address it in a separate > > > > > patch on top of the current series. > > > > > > > > I agree. > > > > > > > > > I'm not sure what approach is best, though. Do you think there should be two > > > > > passes the first of which will check HIDs only and the second one will check > > > > > CIDs? Or do you have something different in mind? > > > > > > > > HID and CIDs are already listed in their priority order in > > > > acpi_device->pnp.ids. So, the single pass like I described above should > > > > work. > > > > > > Well, I'm not sure if I understand you correctly. > > > > > > The device is given and we need to find a handler for it. So, it looks like > > > we first should check if any handler matches the HID. This has to be a pass > > > through all handlers. If there's no match, we need to check if any handler > > > matches any of the device IDs. That will be the second pass, won't it? > > > > acpi_device->pnp.ids has a list of HID->CID[0]->CID[1]..CID[n] since > > acpi_device_set_id() checks HID before CIDs. So, the first pass is to > > check with the first entry of pnp.ids. If no handler is found, then > > check the second entry, and so on. > > I'd prefer to check HID and then all CIDs at once in accordance with the > following rule: > 1) Use the first handler that matches HID exactly. If there's none: > 2) Use the first handler compatible with the given device. What do you mean by all CIDs at once? CIDs also have priority when there are multiple CIDs. That is, CID[0] takes priority over CID[1]. So, going down the pnp.ids list handles the ordering correctly. > > > The difficulty is that the first item in pnp.ids need not be *the* HID. > > > It only will be the HID if ACPI_VALID_HID is set in the device info in > > > acpi_device_set_id(). So perhaps we need to add a hid_valid bit in > > > device->flags and only do the "HID pass" if that is set? > > > > I do not think such bit is necessary for this. _HID is required (unless > > it has _ADR), but _CID is optional. So, the valid cases are that a > > device object has HID only, has both HID and CID(s), or has none of them > > (i.e. _ADR device). If there is a device with CID only, this is a FW > > bug and we just have to check with CID then. > > In addition to that there's a number of objects that we assign artificial HIDs. > I think we should treat them as CIDs rather than as real HIDs in this regard. When an object does not support the _HID/_CID combination, it means that this type of objects can only support a single characteristic, which is the same as a device object with HID only. Therefore, an artificial ID should be considered as an HID. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, January 29, 2013 04:27:28 PM Toshi Kani wrote: > On Wed, 2013-01-30 at 00:19 +0100, Rafael J. Wysocki wrote: > > On Tuesday, January 29, 2013 03:57:10 PM Toshi Kani wrote: > > > On Tue, 2013-01-29 at 22:32 +0100, Rafael J. Wysocki wrote: > > > > On Tuesday, January 29, 2013 07:50:43 AM Toshi Kani wrote: > > > > > On Tue, 2013-01-29 at 12:28 +0100, Rafael J. Wysocki wrote: > > > > > > On Monday, January 28, 2013 07:35:39 PM Toshi Kani wrote: > > > > > > > On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote: > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > > > > > 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@intel.com> > > > > > > > > --- > > > > > > > > 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 > > > > > > > } > > > > > > > > > > > > OK, I see the problem, but I think it's better to address it in a separate > > > > > > patch on top of the current series. > > > > > > > > > > I agree. > > > > > > > > > > > I'm not sure what approach is best, though. Do you think there should be two > > > > > > passes the first of which will check HIDs only and the second one will check > > > > > > CIDs? Or do you have something different in mind? > > > > > > > > > > HID and CIDs are already listed in their priority order in > > > > > acpi_device->pnp.ids. So, the single pass like I described above should > > > > > work. > > > > > > > > Well, I'm not sure if I understand you correctly. > > > > > > > > The device is given and we need to find a handler for it. So, it looks like > > > > we first should check if any handler matches the HID. This has to be a pass > > > > through all handlers. If there's no match, we need to check if any handler > > > > matches any of the device IDs. That will be the second pass, won't it? > > > > > > acpi_device->pnp.ids has a list of HID->CID[0]->CID[1]..CID[n] since > > > acpi_device_set_id() checks HID before CIDs. So, the first pass is to > > > check with the first entry of pnp.ids. If no handler is found, then > > > check the second entry, and so on. > > > > I'd prefer to check HID and then all CIDs at once in accordance with the > > following rule: > > 1) Use the first handler that matches HID exactly. If there's none: > > 2) Use the first handler compatible with the given device. > > What do you mean by all CIDs at once? CIDs also have priority when > there are multiple CIDs. That is, CID[0] takes priority over CID[1]. > So, going down the pnp.ids list handles the ordering correctly. OK, I see what you mean now. > > > > The difficulty is that the first item in pnp.ids need not be *the* HID. > > > > It only will be the HID if ACPI_VALID_HID is set in the device info in > > > > acpi_device_set_id(). So perhaps we need to add a hid_valid bit in > > > > device->flags and only do the "HID pass" if that is set? > > > > > > I do not think such bit is necessary for this. _HID is required (unless > > > it has _ADR), but _CID is optional. So, the valid cases are that a > > > device object has HID only, has both HID and CID(s), or has none of them > > > (i.e. _ADR device). If there is a device with CID only, this is a FW > > > bug and we just have to check with CID then. > > > > In addition to that there's a number of objects that we assign artificial HIDs. > > I think we should treat them as CIDs rather than as real HIDs in this regard. > > When an object does not support the _HID/_CID combination, it means that > this type of objects can only support a single characteristic, which is > the same as a device object with HID only. Therefore, an artificial ID > should be considered as an HID. I'm not sure about that, but it shouldn't really matter a lot anyway. :-) Thanks, Rafael
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; +} + 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@intel.com> + +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.