| Message ID | 1414422584-2638-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Mon, Oct 27, 2014 at 11:09:44PM +0800, Lan Tianyu wrote: > ACPI 5.0 introduces _DEP to designate device objects that OSPM should > assign a higher priority in start ordering due to future operation region > accesses. > > On Asus T100TA, ACPI battery info are read from a I2C slave device via > I2C operation region. Before I2C operation region handler is installed, > battery _STA always returns 0. There is a _DEP method of designating > start order under battery device node. > > This patch is to implement _DEP feature to fix battery issue on the Asus T100TA. > Introducing acpi_dep_list and adding dep_unmet count in the struct > acpi_device. During ACPI namespace scan, create struct acpi_dep_data for a > valid pair of master (device pointed to by _DEP)/slave(device with _DEP), record > master's and slave's ACPI handle in it and put it into acpi_dep_list. The dep_unmet > count will increase by one if there is a device under its _DEP. Driver's probe() should > return EPROBE_DEFER when find dep_unmet larger than 0. When I2C operation > region handler is installed, remove all struct acpi_dep_data on the acpi_dep_list > whose master is pointed to I2C host controller and decrease slave's dep_unmet. > When dep_unmet decreases to 0, all _DEP conditions are met and then do acpi_bus_attach() > for the device in order to resolve battery _STA issue on the Asus T100TA. Is there a reason why the driver core can't handle this automatically in such way that when there are unmet dependencies, it will not probe the device? Or am I missing something? Now it looks like the driver itself needs to know about these and handle them somehow. I'm asking this because there are other things like devices using GPIOs that also take advantage of _DEP, like this on Asus T100: Device (SDHB) { Name (_ADR, Zero) // _ADR: Address Name (_HID, "INT33BB") // _HID: Hardware ID ... Name (_DEP, Package (0x02) // _DEP: Dependencies { PEPD, GPO2 }) I don't know what PEPD is but GPO2 is the GPIO controller. -- 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 Wednesday, November 05, 2014 10:54:14 AM Mika Westerberg wrote: > On Mon, Oct 27, 2014 at 11:09:44PM +0800, Lan Tianyu wrote: > > ACPI 5.0 introduces _DEP to designate device objects that OSPM should > > assign a higher priority in start ordering due to future operation region > > accesses. > > > > On Asus T100TA, ACPI battery info are read from a I2C slave device via > > I2C operation region. Before I2C operation region handler is installed, > > battery _STA always returns 0. There is a _DEP method of designating > > start order under battery device node. > > > > This patch is to implement _DEP feature to fix battery issue on the Asus T100TA. > > Introducing acpi_dep_list and adding dep_unmet count in the struct > > acpi_device. During ACPI namespace scan, create struct acpi_dep_data for a > > valid pair of master (device pointed to by _DEP)/slave(device with _DEP), record > > master's and slave's ACPI handle in it and put it into acpi_dep_list. The dep_unmet > > count will increase by one if there is a device under its _DEP. Driver's probe() should > > return EPROBE_DEFER when find dep_unmet larger than 0. When I2C operation > > region handler is installed, remove all struct acpi_dep_data on the acpi_dep_list > > whose master is pointed to I2C host controller and decrease slave's dep_unmet. > > When dep_unmet decreases to 0, all _DEP conditions are met and then do acpi_bus_attach() > > for the device in order to resolve battery _STA issue on the Asus T100TA. > > Is there a reason why the driver core can't handle this automatically in > such way that when there are unmet dependencies, it will not probe the > device? Or am I missing something? Yes, there is a reason: There's no way to represent such dependencies in the driver core. :-) That's been discussed for several times (and I've even mentioned _DEP in one of those discussions) and it's always ended up being seen as "complexity nightmare". > Now it looks like the driver itself needs to know about these and handle > them somehow. The patch is missing the user of it which is going to be the battery driver only for the time being. Tianyu, can you please include the battery changes into the patch at least for now? > I'm asking this because there are other things like devices using GPIOs > that also take advantage of _DEP, like this on Asus T100: > > Device (SDHB) > { > Name (_ADR, Zero) // _ADR: Address > Name (_HID, "INT33BB") // _HID: Hardware ID > ... > Name (_DEP, Package (0x02) // _DEP: Dependencies > { > PEPD, > GPO2 > }) > > I don't know what PEPD is but GPO2 is the GPIO controller. Well, if you read the definition of _DEP in ACPI 5.1, it is all about operation regions, so this smells like abuse of sorts. In any case, it is hard to argue that _DEP is not internal to ACPI because of that definition.
On Wed, Nov 05, 2014 at 10:55:23PM +0100, Rafael J. Wysocki wrote: > On Wednesday, November 05, 2014 10:54:14 AM Mika Westerberg wrote: > > On Mon, Oct 27, 2014 at 11:09:44PM +0800, Lan Tianyu wrote: > > > ACPI 5.0 introduces _DEP to designate device objects that OSPM should > > > assign a higher priority in start ordering due to future operation region > > > accesses. > > > > > > On Asus T100TA, ACPI battery info are read from a I2C slave device via > > > I2C operation region. Before I2C operation region handler is installed, > > > battery _STA always returns 0. There is a _DEP method of designating > > > start order under battery device node. > > > > > > This patch is to implement _DEP feature to fix battery issue on the Asus T100TA. > > > Introducing acpi_dep_list and adding dep_unmet count in the struct > > > acpi_device. During ACPI namespace scan, create struct acpi_dep_data for a > > > valid pair of master (device pointed to by _DEP)/slave(device with _DEP), record > > > master's and slave's ACPI handle in it and put it into acpi_dep_list. The dep_unmet > > > count will increase by one if there is a device under its _DEP. Driver's probe() should > > > return EPROBE_DEFER when find dep_unmet larger than 0. When I2C operation > > > region handler is installed, remove all struct acpi_dep_data on the acpi_dep_list > > > whose master is pointed to I2C host controller and decrease slave's dep_unmet. > > > When dep_unmet decreases to 0, all _DEP conditions are met and then do acpi_bus_attach() > > > for the device in order to resolve battery _STA issue on the Asus T100TA. > > > > Is there a reason why the driver core can't handle this automatically in > > such way that when there are unmet dependencies, it will not probe the > > device? Or am I missing something? > > Yes, there is a reason: There's no way to represent such dependencies in the > driver core. :-) > > That's been discussed for several times (and I've even mentioned _DEP in one > of those discussions) and it's always ended up being seen as "complexity > nightmare". OK, I see. > > Now it looks like the driver itself needs to know about these and handle > > them somehow. > > The patch is missing the user of it which is going to be the battery driver > only for the time being. Tianyu, can you please include the battery changes > into the patch at least for now? > > > I'm asking this because there are other things like devices using GPIOs > > that also take advantage of _DEP, like this on Asus T100: > > > > Device (SDHB) > > { > > Name (_ADR, Zero) // _ADR: Address > > Name (_HID, "INT33BB") // _HID: Hardware ID > > ... > > Name (_DEP, Package (0x02) // _DEP: Dependencies > > { > > PEPD, > > GPO2 > > }) > > > > I don't know what PEPD is but GPO2 is the GPIO controller. > > Well, if you read the definition of _DEP in ACPI 5.1, it is all about operation > regions, so this smells like abuse of sorts. In any case, it is hard to argue > that _DEP is not internal to ACPI because of that definition. So if we have a device, like SDHB above (SDIO controller), that needs GPIOs for powering the SDIO card and that is done by using GPIO operation regions how do we ensure that the GPIO controller driver is there? I think the answer is that we just make sure the the GPIO driver is there before anything that is going to use it. E.g make it subsys_initcall() or so? -- 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 Thursday, November 06, 2014 08:19:52 AM Mika Westerberg wrote: > On Wed, Nov 05, 2014 at 10:55:23PM +0100, Rafael J. Wysocki wrote: > > On Wednesday, November 05, 2014 10:54:14 AM Mika Westerberg wrote: > > > On Mon, Oct 27, 2014 at 11:09:44PM +0800, Lan Tianyu wrote: > > > > ACPI 5.0 introduces _DEP to designate device objects that OSPM should > > > > assign a higher priority in start ordering due to future operation region > > > > accesses. > > > > > > > > On Asus T100TA, ACPI battery info are read from a I2C slave device via > > > > I2C operation region. Before I2C operation region handler is installed, > > > > battery _STA always returns 0. There is a _DEP method of designating > > > > start order under battery device node. > > > > > > > > This patch is to implement _DEP feature to fix battery issue on the Asus T100TA. > > > > Introducing acpi_dep_list and adding dep_unmet count in the struct > > > > acpi_device. During ACPI namespace scan, create struct acpi_dep_data for a > > > > valid pair of master (device pointed to by _DEP)/slave(device with _DEP), record > > > > master's and slave's ACPI handle in it and put it into acpi_dep_list. The dep_unmet > > > > count will increase by one if there is a device under its _DEP. Driver's probe() should > > > > return EPROBE_DEFER when find dep_unmet larger than 0. When I2C operation > > > > region handler is installed, remove all struct acpi_dep_data on the acpi_dep_list > > > > whose master is pointed to I2C host controller and decrease slave's dep_unmet. > > > > When dep_unmet decreases to 0, all _DEP conditions are met and then do acpi_bus_attach() > > > > for the device in order to resolve battery _STA issue on the Asus T100TA. > > > > > > Is there a reason why the driver core can't handle this automatically in > > > such way that when there are unmet dependencies, it will not probe the > > > device? Or am I missing something? > > > > Yes, there is a reason: There's no way to represent such dependencies in the > > driver core. :-) > > > > That's been discussed for several times (and I've even mentioned _DEP in one > > of those discussions) and it's always ended up being seen as "complexity > > nightmare". > > OK, I see. > > > > Now it looks like the driver itself needs to know about these and handle > > > them somehow. > > > > The patch is missing the user of it which is going to be the battery driver > > only for the time being. Tianyu, can you please include the battery changes > > into the patch at least for now? > > > > > I'm asking this because there are other things like devices using GPIOs > > > that also take advantage of _DEP, like this on Asus T100: > > > > > > Device (SDHB) > > > { > > > Name (_ADR, Zero) // _ADR: Address > > > Name (_HID, "INT33BB") // _HID: Hardware ID > > > ... > > > Name (_DEP, Package (0x02) // _DEP: Dependencies > > > { > > > PEPD, > > > GPO2 > > > }) > > > > > > I don't know what PEPD is but GPO2 is the GPIO controller. > > > > Well, if you read the definition of _DEP in ACPI 5.1, it is all about operation > > regions, so this smells like abuse of sorts. In any case, it is hard to argue > > that _DEP is not internal to ACPI because of that definition. > > So if we have a device, like SDHB above (SDIO controller), that needs > GPIOs for powering the SDIO card and that is done by using GPIO > operation regions how do we ensure that the GPIO controller driver is > there? > > I think the answer is that we just make sure the the GPIO driver is > there before anything that is going to use it. E.g make it > subsys_initcall() or so? The current approach, which arguably is lame, has been to (1) compile in all drivers that *may* be depended on (like all drviers registering operation region handlers) and then (2) trick the devices to be registered in the right order. I'd rather have a more robust mechanism than that, but so far no one has proposed anything remotely usable. As far as _DEP is concerned, it seems that in *practice* it is used to reflect functional dependencies between devices rather than the operation regions thing only (as specified). In that case we may decide to follow the practice rather than the spec (and move to update the spec to reflect the practice at the same time), but that requires some consideration.
On Fri, Nov 07, 2014 at 12:00:34AM +0100, Rafael J. Wysocki wrote: > > So if we have a device, like SDHB above (SDIO controller), that needs > > GPIOs for powering the SDIO card and that is done by using GPIO > > operation regions how do we ensure that the GPIO controller driver is > > there? > > > > I think the answer is that we just make sure the the GPIO driver is > > there before anything that is going to use it. E.g make it > > subsys_initcall() or so? > > The current approach, which arguably is lame, has been to (1) compile in > all drivers that *may* be depended on (like all drviers registering operation > region handlers) and then (2) trick the devices to be registered in the right > order. Right, I kind of expected that it be the case :-/ > I'd rather have a more robust mechanism than that, but so far no one has > proposed anything remotely usable. > > As far as _DEP is concerned, it seems that in *practice* it is used to > reflect functional dependencies between devices rather than the operation > regions thing only (as specified). In that case we may decide to follow > the practice rather than the spec (and move to update the spec to reflect > the practice at the same time), but that requires some consideration. Indeed it looks very much that it contains more than just operation depedencies. In T100 for example there is this PEPD device that does not have any operation regions and it is still in the _DEP list of the SDIO controller device. -- 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
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 4da55d8..bfa9c77 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -36,6 +36,8 @@ bool acpi_force_hot_remove; static const char *dummy_hid = "device"; +static LIST_HEAD(acpi_dep_list); +static DEFINE_MUTEX(acpi_dep_list_lock); static LIST_HEAD(acpi_bus_id_list); static DEFINE_MUTEX(acpi_scan_lock); static LIST_HEAD(acpi_scan_handlers_list); @@ -43,6 +45,12 @@ DEFINE_MUTEX(acpi_device_lock); LIST_HEAD(acpi_wakeup_device_list); static DEFINE_MUTEX(acpi_hp_context_lock); +struct acpi_dep_data { + struct list_head node; + acpi_handle master; + acpi_handle slave; +}; + struct acpi_device_bus_id{ char bus_id[15]; unsigned int instance_no; @@ -2121,6 +2129,63 @@ static void acpi_scan_init_hotplug(struct acpi_device *adev) } } +static void acpi_device_dep_initialize(struct acpi_device *adev) +{ + struct acpi_dep_data *dep; + struct acpi_handle_list dep_devices; + struct acpi_device_info *info; + acpi_status status; + int i, skip; + + if (!acpi_has_method(adev->handle, "_DEP")) + return; + + status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, + &dep_devices); + if (ACPI_FAILURE(status)) { + dev_err(&adev->dev, "Failed to evaluate _DEP.\n"); + return; + } + + for (i = 0; i < dep_devices.count; i++) { + + status = acpi_get_object_info(dep_devices.handles[i], &info); + if (ACPI_FAILURE(status)) { + dev_err(&adev->dev, "Error reading device info\n"); + continue; + } + + /* + * Skip the dependency of Windows System Power + * Management Controller + */ + if (info->valid & ACPI_VALID_HID + && !strcmp(info->hardware_id.string, "INT3396")) + skip = 1; + else + skip = 0; + + kfree(info); + + if (skip) + continue; + + dep = kzalloc(sizeof(struct acpi_dep_data), GFP_KERNEL); + if (!dep) { + dev_err(&adev->dev, "Not enough memory for _DEP list entry.\n"); + return; + } + + dep->master = dep_devices.handles[i]; + dep->slave = adev->handle; + adev->dep_unmet++; + + mutex_lock(&acpi_dep_list_lock); + list_add_tail(&dep->node , &acpi_dep_list); + mutex_unlock(&acpi_dep_list_lock); + } +} + static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used, void *not_used, void **return_value) { @@ -2147,6 +2212,7 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used, return AE_CTRL_DEPTH; acpi_scan_init_hotplug(device); + acpi_device_dep_initialize(device); out: if (!*return_value) @@ -2267,6 +2333,31 @@ static void acpi_bus_attach(struct acpi_device *device) device->handler->hotplug.notify_online(device); } +int acpi_walk_dep_device_list(acpi_handle handle) +{ + struct acpi_dep_data *dep, *tmp; + struct acpi_device *adev; + + mutex_lock(&acpi_dep_list_lock); + list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) { + if (dep->master == handle) { + acpi_bus_get_device(dep->slave, &adev); + if (!adev) + continue; + + adev->dep_unmet--; + if (!adev->dep_unmet) + acpi_bus_attach(adev); + list_del(&dep->node); + kfree(dep); + } + } + mutex_unlock(&acpi_dep_list_lock); + + return 0; +} +EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list); + /** * acpi_bus_scan - Add ACPI device node objects in a given namespace scope. * @handle: Root of the namespace scope to scan. diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 126e182..d5495ca 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -402,6 +402,7 @@ static int acpi_i2c_install_space_handler(struct i2c_adapter *adapter) return -ENOMEM; } + acpi_walk_dep_device_list(handle); return 0; } diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 98cd723..269f068 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -367,6 +367,7 @@ struct acpi_device { void *driver_data; struct device dev; unsigned int physical_node_count; + unsigned int dep_unmet; struct list_head physical_node_list; struct mutex physical_node_lock; void (*remove)(struct acpi_device *); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 2b9c235..cec5f55 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -428,6 +428,7 @@ extern bool acpi_driver_match_device(struct device *dev, const struct device_driver *drv); int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); int acpi_device_modalias(struct device *, char *, int); +int acpi_walk_dep_device_list(acpi_handle handle); #define ACPI_PTR(_ptr) (_ptr) @@ -446,6 +447,8 @@ static inline const char *acpi_dev_name(struct acpi_device *adev) static inline void acpi_early_init(void) { } +static inline int acpi_walk_dep_device_list(acpi_handle handle) { } + static inline int early_acpi_boot_init(void) { return 0;
ACPI 5.0 introduces _DEP to designate device objects that OSPM should assign a higher priority in start ordering due to future operation region accesses. On Asus T100TA, ACPI battery info are read from a I2C slave device via I2C operation region. Before I2C operation region handler is installed, battery _STA always returns 0. There is a _DEP method of designating start order under battery device node. This patch is to implement _DEP feature to fix battery issue on the Asus T100TA. Introducing acpi_dep_list and adding dep_unmet count in the struct acpi_device. During ACPI namespace scan, create struct acpi_dep_data for a valid pair of master (device pointed to by _DEP)/slave(device with _DEP), record master's and slave's ACPI handle in it and put it into acpi_dep_list. The dep_unmet count will increase by one if there is a device under its _DEP. Driver's probe() should return EPROBE_DEFER when find dep_unmet larger than 0. When I2C operation region handler is installed, remove all struct acpi_dep_data on the acpi_dep_list whose master is pointed to I2C host controller and decrease slave's dep_unmet. When dep_unmet decreases to 0, all _DEP conditions are met and then do acpi_bus_attach() for the device in order to resolve battery _STA issue on the Asus T100TA. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- Change since V1: Rework struct acpi_dep_data, record master and slave's ACPI handle. Rename dep_present with dep_unmet and make it as a count. Increase dep_unmet during bootup and decrease it when associated operation region is installed. drivers/acpi/scan.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++ drivers/i2c/i2c-core.c | 1 + include/acpi/acpi_bus.h | 1 + include/linux/acpi.h | 3 ++ 4 files changed, 96 insertions(+)