| Message ID | 1399183626-13152-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi! > Most time of battery resume callback is spent on executing AML code > _BTP, _BIF and _BIF to get battery info, status and set alarm. These > AML methods may access EC operation regions several times and consumes > time. > > These operations are not necessary during devices resume and can run > during POST_SUSPEND/HIBERNATION event when all processes are thawed. Does it mean that userspace will see pre-hibernation battery level for a while after resume? Pavel
On 2014?05?09? 15:38, Pavel Machek wrote: > Hi! > >> Most time of battery resume callback is spent on executing AML code >> _BTP, _BIF and _BIF to get battery info, status and set alarm. These >> AML methods may access EC operation regions several times and consumes >> time. >> >> These operations are not necessary during devices resume and can run >> during POST_SUSPEND/HIBERNATION event when all processes are thawed. > > Does it mean that userspace will see pre-hibernation battery level for a while > after resume? Hi Pavel: Thanks for your review. No, when user access the battery sysfs interface, driver will read battery info and return new data. > > Pavel >
On Sunday, May 04, 2014 02:07:06 PM Lan Tianyu wrote: > Most time of battery resume callback is spent on executing AML code > _BTP, _BIF and _BIF to get battery info, status and set alarm. These > AML methods may access EC operation regions several times and consumes > time. > > These operations are not necessary during devices resume and can run > during POST_SUSPEND/HIBERNATION event when all processes are thawed. > > This also can avoid removing and adding battery sysfs nodes every system > resume even if the battery unit is not actually changed. The original code > updates sysfs nodes without check and this seems not reasonable. > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> Queued up for 3.16, thanks! > --- > This patch is to base on the linux pm tree linux-next branch with > patches "ACPI: Revert "ACPI / Battery: Remove battery's proc directory"" > and "ACPI: Revert "ACPI: Remove CONFIG_ACPI_PROCFS_POWER and cm_sbsc.c"". > > drivers/acpi/battery.c | 36 +++++++++++++++++++++++++++--------- > 1 file changed, 27 insertions(+), 9 deletions(-) > > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index 6e7b2a1..3b4921e 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -696,7 +696,7 @@ static void acpi_battery_quirks(struct acpi_battery *battery) > } > } > > -static int acpi_battery_update(struct acpi_battery *battery) > +static int acpi_battery_update(struct acpi_battery *battery, bool resume) > { > int result, old_present = acpi_battery_present(battery); > result = acpi_battery_get_status(battery); > @@ -707,6 +707,10 @@ static int acpi_battery_update(struct acpi_battery *battery) > battery->update_time = 0; > return 0; > } > + > + if (resume) > + return 0; > + > if (!battery->update_time || > old_present != acpi_battery_present(battery)) { > result = acpi_battery_get_info(battery); > @@ -915,7 +919,7 @@ static print_func acpi_print_funcs[ACPI_BATTERY_NUMFILES] = { > static int acpi_battery_read(int fid, struct seq_file *seq) > { > struct acpi_battery *battery = seq->private; > - int result = acpi_battery_update(battery); > + int result = acpi_battery_update(battery, false); > return acpi_print_funcs[fid](seq, result); > } > > @@ -1030,7 +1034,7 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event) > old = battery->bat.dev; > if (event == ACPI_BATTERY_NOTIFY_INFO) > acpi_battery_refresh(battery); > - acpi_battery_update(battery); > + acpi_battery_update(battery, false); > acpi_bus_generate_netlink_event(device->pnp.device_class, > dev_name(&device->dev), event, > acpi_battery_present(battery)); > @@ -1045,13 +1049,27 @@ static int battery_notify(struct notifier_block *nb, > { > struct acpi_battery *battery = container_of(nb, struct acpi_battery, > pm_nb); > + int result; > + > switch (mode) { > case PM_POST_HIBERNATION: > case PM_POST_SUSPEND: > - if (battery->bat.dev) { > - sysfs_remove_battery(battery); > - sysfs_add_battery(battery); > - } > + if (!acpi_battery_present(battery)) > + return 0; > + > + if (!battery->bat.dev) { > + result = acpi_battery_get_info(battery); > + if (result) > + return result; > + > + result = sysfs_add_battery(battery); > + if (result) > + return result; > + } else > + acpi_battery_refresh(battery); > + > + acpi_battery_init_alarm(battery); > + acpi_battery_get_state(battery); > break; > } > > @@ -1087,7 +1105,7 @@ static int acpi_battery_add(struct acpi_device *device) > mutex_init(&battery->sysfs_lock); > if (acpi_has_method(battery->device->handle, "_BIX")) > set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags); > - result = acpi_battery_update(battery); > + result = acpi_battery_update(battery, false); > if (result) > goto fail; > #ifdef CONFIG_ACPI_PROCFS_POWER > @@ -1149,7 +1167,7 @@ static int acpi_battery_resume(struct device *dev) > return -EINVAL; > > battery->update_time = 0; > - acpi_battery_update(battery); > + acpi_battery_update(battery, true); > return 0; > } > #else >
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index 6e7b2a1..3b4921e 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -696,7 +696,7 @@ static void acpi_battery_quirks(struct acpi_battery *battery) } } -static int acpi_battery_update(struct acpi_battery *battery) +static int acpi_battery_update(struct acpi_battery *battery, bool resume) { int result, old_present = acpi_battery_present(battery); result = acpi_battery_get_status(battery); @@ -707,6 +707,10 @@ static int acpi_battery_update(struct acpi_battery *battery) battery->update_time = 0; return 0; } + + if (resume) + return 0; + if (!battery->update_time || old_present != acpi_battery_present(battery)) { result = acpi_battery_get_info(battery); @@ -915,7 +919,7 @@ static print_func acpi_print_funcs[ACPI_BATTERY_NUMFILES] = { static int acpi_battery_read(int fid, struct seq_file *seq) { struct acpi_battery *battery = seq->private; - int result = acpi_battery_update(battery); + int result = acpi_battery_update(battery, false); return acpi_print_funcs[fid](seq, result); } @@ -1030,7 +1034,7 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event) old = battery->bat.dev; if (event == ACPI_BATTERY_NOTIFY_INFO) acpi_battery_refresh(battery); - acpi_battery_update(battery); + acpi_battery_update(battery, false); acpi_bus_generate_netlink_event(device->pnp.device_class, dev_name(&device->dev), event, acpi_battery_present(battery)); @@ -1045,13 +1049,27 @@ static int battery_notify(struct notifier_block *nb, { struct acpi_battery *battery = container_of(nb, struct acpi_battery, pm_nb); + int result; + switch (mode) { case PM_POST_HIBERNATION: case PM_POST_SUSPEND: - if (battery->bat.dev) { - sysfs_remove_battery(battery); - sysfs_add_battery(battery); - } + if (!acpi_battery_present(battery)) + return 0; + + if (!battery->bat.dev) { + result = acpi_battery_get_info(battery); + if (result) + return result; + + result = sysfs_add_battery(battery); + if (result) + return result; + } else + acpi_battery_refresh(battery); + + acpi_battery_init_alarm(battery); + acpi_battery_get_state(battery); break; } @@ -1087,7 +1105,7 @@ static int acpi_battery_add(struct acpi_device *device) mutex_init(&battery->sysfs_lock); if (acpi_has_method(battery->device->handle, "_BIX")) set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags); - result = acpi_battery_update(battery); + result = acpi_battery_update(battery, false); if (result) goto fail; #ifdef CONFIG_ACPI_PROCFS_POWER @@ -1149,7 +1167,7 @@ static int acpi_battery_resume(struct device *dev) return -EINVAL; battery->update_time = 0; - acpi_battery_update(battery); + acpi_battery_update(battery, true); return 0; } #else
Most time of battery resume callback is spent on executing AML code _BTP, _BIF and _BIF to get battery info, status and set alarm. These AML methods may access EC operation regions several times and consumes time. These operations are not necessary during devices resume and can run during POST_SUSPEND/HIBERNATION event when all processes are thawed. This also can avoid removing and adding battery sysfs nodes every system resume even if the battery unit is not actually changed. The original code updates sysfs nodes without check and this seems not reasonable. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- This patch is to base on the linux pm tree linux-next branch with patches "ACPI: Revert "ACPI / Battery: Remove battery's proc directory"" and "ACPI: Revert "ACPI: Remove CONFIG_ACPI_PROCFS_POWER and cm_sbsc.c"". drivers/acpi/battery.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)