Some machines'(E,G Lenovo Z480) ECs are not stable during boot up
and causes battery driver fails to be probed due to failure of getting
battery information from EC sometimes. After several retries, the
operation will work. This patch is to retry to get battery information 5
times if the first try fails.
Reported-and-tested-by: naszar <[email protected]>
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581
Cc: [email protected]
Signed-off-by: Lan Tianyu <[email protected]>
---
drivers/acpi/battery.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index e48fc98..485009d 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -34,6 +34,7 @@
#include <linux/dmi.h>
#include <linux/slab.h>
#include <linux/suspend.h>
+#include <linux/delay.h>
#include <asm/unaligned.h>
#ifdef CONFIG_ACPI_PROCFS_POWER
@@ -1119,7 +1120,7 @@ static struct dmi_system_id bat_dmi_table[] = {
static int acpi_battery_add(struct acpi_device *device)
{
- int result = 0;
+ int result = 0, retry = 5;
struct acpi_battery *battery = NULL;
if (!device)
@@ -1135,7 +1136,16 @@ 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);
+
+retry_get_info:
result = acpi_battery_update(battery, false);
+
+ if (result && retry) {
+ msleep(20);
+ retry--;
+ goto retry_get_info;
+ }
+
if (result)
goto fail;
#ifdef CONFIG_ACPI_PROCFS_POWER
--
1.8.4.rc0.1.g8f6a3e5.dirty
On Thu, 12 Jun 2014, Lan Tianyu wrote:
> Some machines'(E,G Lenovo Z480) ECs are not stable during boot up
> and causes battery driver fails to be probed due to failure of getting
> battery information from EC sometimes. After several retries, the
> operation will work. This patch is to retry to get battery information 5
> times if the first try fails.
>
> Reported-and-tested-by: naszar <[email protected]>
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581
> Cc: [email protected]
> Signed-off-by: Lan Tianyu <[email protected]>
> ---
> drivers/acpi/battery.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index e48fc98..485009d 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -34,6 +34,7 @@
> #include <linux/dmi.h>
> #include <linux/slab.h>
> #include <linux/suspend.h>
> +#include <linux/delay.h>
> #include <asm/unaligned.h>
>
> #ifdef CONFIG_ACPI_PROCFS_POWER
> @@ -1119,7 +1120,7 @@ static struct dmi_system_id bat_dmi_table[] = {
>
> static int acpi_battery_add(struct acpi_device *device)
> {
> - int result = 0;
> + int result = 0, retry = 5;
> struct acpi_battery *battery = NULL;
>
> if (!device)
> @@ -1135,7 +1136,16 @@ 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);
> +
> +retry_get_info:
> result = acpi_battery_update(battery, false);
> +
> + if (result && retry) {
> + msleep(20);
We're really going to wait up to 20 * 5 = 100ms for acpi_battery_update()
to succeed? How are these the numbers that are determined to be optimal
for probing?
> + retry--;
> + goto retry_get_info;
> + }
This most certainly could be rewritten as a for-loop and remove the ugly
goto.
> +
> if (result)
> goto fail;
> #ifdef CONFIG_ACPI_PROCFS_POWER
On 2014年06月12日 14:55, David Rientjes wrote:
> On Thu, 12 Jun 2014, Lan Tianyu wrote:
>
>> Some machines'(E,G Lenovo Z480) ECs are not stable during boot up
>> and causes battery driver fails to be probed due to failure of getting
>> battery information from EC sometimes. After several retries, the
>> operation will work. This patch is to retry to get battery information 5
>> times if the first try fails.
>>
>> Reported-and-tested-by: naszar <[email protected]>
>> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581
>> Cc: [email protected]
>> Signed-off-by: Lan Tianyu <[email protected]>
>> ---
>> drivers/acpi/battery.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index e48fc98..485009d 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -34,6 +34,7 @@
>> #include <linux/dmi.h>
>> #include <linux/slab.h>
>> #include <linux/suspend.h>
>> +#include <linux/delay.h>
>> #include <asm/unaligned.h>
>>
>> #ifdef CONFIG_ACPI_PROCFS_POWER
>> @@ -1119,7 +1120,7 @@ static struct dmi_system_id bat_dmi_table[] = {
>>
>> static int acpi_battery_add(struct acpi_device *device)
>> {
>> - int result = 0;
>> + int result = 0, retry = 5;
>> struct acpi_battery *battery = NULL;
>>
>> if (!device)
>> @@ -1135,7 +1136,16 @@ 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);
>> +
>> +retry_get_info:
>> result = acpi_battery_update(battery, false);
>> +
>> + if (result && retry) {
>> + msleep(20);
>
Hi David:
Thanks for review.
> We're really going to wait up to 20 * 5 = 100ms for acpi_battery_update()
> to succeed?
No, this depends which retry acpi_battery_update() will succeed. For
most machines, there will be no delay.
> How are these the numbers that are determined to be optimal
> for probing?
So far, it depends the return values of executing ACPI methods. If they
were failed, the probing would not go further.
>
>> + retry--;
>> + goto retry_get_info;
>> + }
>
> This most certainly could be rewritten as a for-loop and remove the ugly
> goto.
Ok. I will update.
>
>> +
>> if (result)
>> goto fail;
>> #ifdef CONFIG_ACPI_PROCFS_POWER
--
Best regards
Tianyu Lan
On Thu, 12 Jun 2014, Lan Tianyu wrote:
> >> Some machines'(E,G Lenovo Z480) ECs are not stable during boot up
> >> and causes battery driver fails to be probed due to failure of getting
> >> battery information from EC sometimes. After several retries, the
> >> operation will work. This patch is to retry to get battery information 5
> >> times if the first try fails.
> >>
> >> Reported-and-tested-by: naszar <[email protected]>
> >> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581
> >> Cc: [email protected]
> >> Signed-off-by: Lan Tianyu <[email protected]>
> >> ---
> >> drivers/acpi/battery.c | 12 +++++++++++-
> >> 1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> >> index e48fc98..485009d 100644
> >> --- a/drivers/acpi/battery.c
> >> +++ b/drivers/acpi/battery.c
> >> @@ -34,6 +34,7 @@
> >> #include <linux/dmi.h>
> >> #include <linux/slab.h>
> >> #include <linux/suspend.h>
> >> +#include <linux/delay.h>
> >> #include <asm/unaligned.h>
> >>
> >> #ifdef CONFIG_ACPI_PROCFS_POWER
> >> @@ -1119,7 +1120,7 @@ static struct dmi_system_id bat_dmi_table[] = {
> >>
> >> static int acpi_battery_add(struct acpi_device *device)
> >> {
> >> - int result = 0;
> >> + int result = 0, retry = 5;
> >> struct acpi_battery *battery = NULL;
> >>
> >> if (!device)
> >> @@ -1135,7 +1136,16 @@ 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);
> >> +
> >> +retry_get_info:
> >> result = acpi_battery_update(battery, false);
> >> +
> >> + if (result && retry) {
> >> + msleep(20);
> >
>
> Hi David:
> Thanks for review.
>
> > We're really going to wait up to 20 * 5 = 100ms for acpi_battery_update()
> > to succeed?
>
> No, this depends which retry acpi_battery_update() will succeed. For
> most machines, there will be no delay.
>
Right, but you're willing to wait up to 100ms for it to succeed? You're
implementing x retries with y ms sleep in between, I'm asking how it is
determined that the optimal values are x = 5 and y = 20. More directly:
is it possible to succeed at 101ms? Is it really likely to succeed after
the first 20ms?
On 2014年06月12日 15:26, David Rientjes wrote:
> On Thu, 12 Jun 2014, Lan Tianyu wrote:
>
>>>> Some machines'(E,G Lenovo Z480) ECs are not stable during boot up
>>>> and causes battery driver fails to be probed due to failure of getting
>>>> battery information from EC sometimes. After several retries, the
>>>> operation will work. This patch is to retry to get battery information 5
>>>> times if the first try fails.
>>>>
>>>> Reported-and-tested-by: naszar <[email protected]>
>>>> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581
>>>> Cc: [email protected]
>>>> Signed-off-by: Lan Tianyu <[email protected]>
>>>> ---
>>>> drivers/acpi/battery.c | 12 +++++++++++-
>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>>>> index e48fc98..485009d 100644
>>>> --- a/drivers/acpi/battery.c
>>>> +++ b/drivers/acpi/battery.c
>>>> @@ -34,6 +34,7 @@
>>>> #include <linux/dmi.h>
>>>> #include <linux/slab.h>
>>>> #include <linux/suspend.h>
>>>> +#include <linux/delay.h>
>>>> #include <asm/unaligned.h>
>>>>
>>>> #ifdef CONFIG_ACPI_PROCFS_POWER
>>>> @@ -1119,7 +1120,7 @@ static struct dmi_system_id bat_dmi_table[] = {
>>>>
>>>> static int acpi_battery_add(struct acpi_device *device)
>>>> {
>>>> - int result = 0;
>>>> + int result = 0, retry = 5;
>>>> struct acpi_battery *battery = NULL;
>>>>
>>>> if (!device)
>>>> @@ -1135,7 +1136,16 @@ 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);
>>>> +
>>>> +retry_get_info:
>>>> result = acpi_battery_update(battery, false);
>>>> +
>>>> + if (result && retry) {
>>>> + msleep(20);
>>>
>>
>> Hi David:
>> Thanks for review.
>>
>>> We're really going to wait up to 20 * 5 = 100ms for acpi_battery_update()
>>> to succeed?
>>
>> No, this depends which retry acpi_battery_update() will succeed. For
>> most machines, there will be no delay.
>>
>
> Right, but you're willing to wait up to 100ms for it to succeed? You're
> implementing x retries with y ms sleep in between, I'm asking how it is
> determined that the optimal values are x = 5 and y = 20. More directly:
> is it possible to succeed at 101ms?
The retry time is set by randomly and not accurate because don't know
when EC will work normally. Set the retry time to 5 just in order to
make sure battery driver probing sucessfully every time,
> Is it really likely to succeed after
> the first 20ms?
>
Yes, it's possible.
>From naszar's test log, acpi_battery_update() failed only once. But not
sure that this happens every time, treat it conservatively and set the
retry time to 5.
https://bugzilla.kernel.org/attachment.cgi?id=139081&action=edit
--
Best regards
Tianyu Lan
On Thu, 12 Jun 2014, Lan Tianyu wrote:
> The retry time is set by randomly and not accurate because don't know
> when EC will work normally. Set the retry time to 5 just in order to
> make sure battery driver probing sucessfully every time,
>
Ok, I was hoping to avoid the excessive wait if it will never actually
succeed but I assume there's some evidence that it can succeed after 40ms,
60ms, etc. Please consider the following instead:
for (i = 0; i < 5; i++) {
/* Comment on why we need a delay in between retries */
if (i)
msleep(20);
result = acpi_battery_update(battery, false);
if (!result)
break;
}
On 2014年06月13日 05:17, David Rientjes wrote:
> On Thu, 12 Jun 2014, Lan Tianyu wrote:
>
>> The retry time is set by randomly and not accurate because don't know
>> when EC will work normally. Set the retry time to 5 just in order to
>> make sure battery driver probing sucessfully every time,
>>
>
> Ok, I was hoping to avoid the excessive wait if it will never actually
> succeed
Ok. The battery driver has used async init and so this will not affect
the speed of boot up.
> but I assume there's some evidence that it can succeed after 40ms,
> 60ms, etc. Please consider the following instead:
>
> for (i = 0; i < 5; i++) {
> /* Comment on why we need a delay in between retries */
> if (i)
> msleep(20);
> result = acpi_battery_update(battery, false);
> if (!result)
> break;
> }
>
How about this?
- result = acpi_battery_update(battery, false);
- if (result)
+
+ /*
+ * Some machines'(E,G Lenovo Z480) ECs are not stable
+ * during boot up and this causes battery driver fails to be
+ * probed due to failure of getting battery information
+ * from EC sometimes. After several retries, the operation
+ * may work. So add retry code here and 20ms sleep between
+ * every retries.
+ */
+ while (acpi_battery_update(battery, false) && retry--)
+ msleep(20);
+ if (!retry) {
+ result = -ENODEV;
goto fail;
+ }
+
--
Best regards
Tianyu Lan
On Fri, 13 Jun 2014, Lan Tianyu wrote:
> How about this?
>
> - result = acpi_battery_update(battery, false);
> - if (result)
> +
> + /*
> + * Some machines'(E,G Lenovo Z480) ECs are not stable
> + * during boot up and this causes battery driver fails to be
> + * probed due to failure of getting battery information
> + * from EC sometimes. After several retries, the operation
> + * may work. So add retry code here and 20ms sleep between
> + * every retries.
> + */
> + while (acpi_battery_update(battery, false) && retry--)
> + msleep(20);
> + if (!retry) {
> + result = -ENODEV;
> goto fail;
> + }
> +
I think you want --retry and not retry--. Otherwise it's possible for the
final call to acpi_battery_update() to succeed and now it's returning
-ENODEV.
On 2014年06月14日 05:46, David Rientjes wrote:
> On Fri, 13 Jun 2014, Lan Tianyu wrote:
>
>> How about this?
>>
>> - result = acpi_battery_update(battery, false);
>> - if (result)
>> +
>> + /*
>> + * Some machines'(E,G Lenovo Z480) ECs are not stable
>> + * during boot up and this causes battery driver fails to be
>> + * probed due to failure of getting battery information
>> + * from EC sometimes. After several retries, the operation
>> + * may work. So add retry code here and 20ms sleep between
>> + * every retries.
>> + */
>> + while (acpi_battery_update(battery, false) && retry--)
>> + msleep(20);
>> + if (!retry) {
>> + result = -ENODEV;
>> goto fail;
>> + }
>> +
>
> I think you want --retry and not retry--.
My original purpose is to retry 5 times after the first try fails.
If use "--retry" here, it just retries 4 times.
> Otherwise it's possible for the
> final call to acpi_battery_update() to succeed and now it's returning
> -ENODEV.
>
Yes, it maybe and I will change code like the following.
while ((result = acpi_battery_update(battery, false)) && retry--)
msleep(20);
if (result)
goto fail;
--
Best regards
Tianyu Lan
Hi,
> From: [email protected] [mailto:[email protected]] On Behalf Of Lan Tianyu
> Sent: Monday, June 16, 2014 10:12 AM
> To: David Rientjes
> Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
>
> On 2014年06月14日 05:46, David Rientjes wrote:
> > On Fri, 13 Jun 2014, Lan Tianyu wrote:
> >
> >> How about this?
> >>
> >> - result = acpi_battery_update(battery, false);
> >> - if (result)
> >> +
> >> + /*
> >> + * Some machines'(E,G Lenovo Z480) ECs are not stable
Just a reminder.
This statement may not be true.
The issue may be caused by the EC driver itself.
So we need to investigate.
> >> + * during boot up and this causes battery driver fails to be
> >> + * probed due to failure of getting battery information
> >> + * from EC sometimes. After several retries, the operation
> >> + * may work. So add retry code here and 20ms sleep between
> >> + * every retries.
> >> + */
> >> + while (acpi_battery_update(battery, false) && retry--)
If EC hardware is stable, why we need to do retry here?
Thanks and best regards
-Lv
> >> + msleep(20);
> >> + if (!retry) {
> >> + result = -ENODEV;
> >> goto fail;
> >> + }
> >> +
> >
> > I think you want --retry and not retry--.
>
> My original purpose is to retry 5 times after the first try fails.
> If use "--retry" here, it just retries 4 times.
>
> > Otherwise it's possible for the
> > final call to acpi_battery_update() to succeed and now it's returning
> > -ENODEV.
> >
>
> Yes, it maybe and I will change code like the following.
>
> while ((result = acpi_battery_update(battery, false)) && retry--)
> msleep(20);
> if (result)
> goto fail;
>
>
> --
> Best regards
> Tianyu Lan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 2014年06月16日 10:35, Zheng, Lv wrote:
> Hi,
>
>> From: [email protected] [mailto:[email protected]] On Behalf Of Lan Tianyu
>> Sent: Monday, June 16, 2014 10:12 AM
>> To: David Rientjes
>> Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
>> Subject: Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
>>
>> On 2014年06月14日 05:46, David Rientjes wrote:
>>> On Fri, 13 Jun 2014, Lan Tianyu wrote:
>>>
>>>> How about this?
>>>>
>>>> - result = acpi_battery_update(battery, false);
>>>> - if (result)
>>>> +
>>>> + /*
>>>> + * Some machines'(E,G Lenovo Z480) ECs are not stable
>
> Just a reminder.
>
> This statement may not be true.
> The issue may be caused by the EC driver itself.
> So we need to investigate.
Not sure. The bug doesn't happen every time. 5-10% during boot up.
>
>>>> + * during boot up and this causes battery driver fails to be
>>>> + * probed due to failure of getting battery information
>>>> + * from EC sometimes. After several retries, the operation
>>>> + * may work. So add retry code here and 20ms sleep between
>>>> + * every retries.
>>>> + */
>>>> + while (acpi_battery_update(battery, false) && retry--)
>
> If EC hardware is stable, why we need to do retry here?
>
Yes, if it can work normally every time, we don't need retry here.
> Thanks and best regards
> -Lv
>
>>>> + msleep(20);
>>>> + if (!retry) {
>>>> + result = -ENODEV;
>>>> goto fail;
>>>> + }
>>>> +
>>>
>>> I think you want --retry and not retry--.
>>
>> My original purpose is to retry 5 times after the first try fails.
>> If use "--retry" here, it just retries 4 times.
>>
>>> Otherwise it's possible for the
>>> final call to acpi_battery_update() to succeed and now it's returning
>>> -ENODEV.
>>>
>>
>> Yes, it maybe and I will change code like the following.
>>
>> while ((result = acpi_battery_update(battery, false)) && retry--)
>> msleep(20);
>> if (result)
>> goto fail;
>>
>>
>> --
>> Best regards
>> Tianyu Lan
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best regards
Tianyu Lan
On Mon, 16 Jun 2014, Lan Tianyu wrote:
> >> How about this?
> >>
> >> - result = acpi_battery_update(battery, false);
> >> - if (result)
> >> +
> >> + /*
> >> + * Some machines'(E,G Lenovo Z480) ECs are not stable
> >> + * during boot up and this causes battery driver fails to be
> >> + * probed due to failure of getting battery information
> >> + * from EC sometimes. After several retries, the operation
> >> + * may work. So add retry code here and 20ms sleep between
> >> + * every retries.
> >> + */
> >> + while (acpi_battery_update(battery, false) && retry--)
> >> + msleep(20);
> >> + if (!retry) {
> >> + result = -ENODEV;
> >> goto fail;
> >> + }
> >> +
> >
> > I think you want --retry and not retry--.
>
> My original purpose is to retry 5 times after the first try fails.
> If use "--retry" here, it just retries 4 times.
>
> > Otherwise it's possible for the
> > final call to acpi_battery_update() to succeed and now it's returning
> > -ENODEV.
> >
>
> Yes, it maybe and I will change code like the following.
>
> while ((result = acpi_battery_update(battery, false)) && retry--)
> msleep(20);
> if (result)
> goto fail;
>
Looks good.
On 2014年06月17日 09:27, David Rientjes wrote:
> On Mon, 16 Jun 2014, Lan Tianyu wrote:
>
>>>> How about this?
>>>>
>>>> - result = acpi_battery_update(battery, false);
>>>> - if (result)
>>>> +
>>>> + /*
>>>> + * Some machines'(E,G Lenovo Z480) ECs are not stable
>>>> + * during boot up and this causes battery driver fails to be
>>>> + * probed due to failure of getting battery information
>>>> + * from EC sometimes. After several retries, the operation
>>>> + * may work. So add retry code here and 20ms sleep between
>>>> + * every retries.
>>>> + */
>>>> + while (acpi_battery_update(battery, false) && retry--)
>>>> + msleep(20);
>>>> + if (!retry) {
>>>> + result = -ENODEV;
>>>> goto fail;
>>>> + }
>>>> +
>>>
>>> I think you want --retry and not retry--.
>>
>> My original purpose is to retry 5 times after the first try fails.
>> If use "--retry" here, it just retries 4 times.
>>
>>> Otherwise it's possible for the
>>> final call to acpi_battery_update() to succeed and now it's returning
>>> -ENODEV.
>>>
>>
>> Yes, it maybe and I will change code like the following.
>>
>> while ((result = acpi_battery_update(battery, false)) && retry--)
>> msleep(20);
>> if (result)
>> goto fail;
>>
>
> Looks good.
>
Great. Thanks for review. :)
--
Best regards
Tianyu Lan