| Message ID | 1402552946-14704-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 <naszar@ya.ru> > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 > Cc: stable@vger.kernel.org > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > 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 -- 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 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 <naszar@ya.ru> >> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 >> Cc: stable@vger.kernel.org >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> 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
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 <naszar@ya.ru> > >> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 > >> Cc: stable@vger.kernel.org > >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > >> --- > >> 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? -- 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 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 <naszar@ya.ru> >>>> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >>>> --- >>>> 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
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; } -- 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 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; + } +
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. -- 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 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;
Hi, > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Lan Tianyu > Sent: Monday, June 16, 2014 10:12 AM > To: David Rientjes > Cc: rjw@rjwysocki.net; lenb@kernel.org; naszar@ya.ru; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014?06?16? 10:35, Zheng, Lv wrote: > Hi, > >> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Lan Tianyu >> Sent: Monday, June 16, 2014 10:12 AM >> To: David Rientjes >> Cc: rjw@rjwysocki.net; lenb@kernel.org; naszar@ya.ru; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org >> 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- 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 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. :)
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
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 <naszar@ya.ru> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 Cc: stable@vger.kernel.org Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- drivers/acpi/battery.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)