Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
On 2014年06月16日 10:35, Zheng, Lv wrote: > Hi, > >> From: linux-acpi-ow...@vger.kernel.org >> [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Lan Tianyu >> Sent: Monday, June 16, 2014 10:12 AM >> To: David Rientjes >> Cc: r...@rjwysocki.net; l...@kernel.org; nas...@ya.ru; >> linux-a...@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 majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best regards Tianyu Lan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
Hi, > From: linux-acpi-ow...@vger.kernel.org > [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Lan Tianyu > Sent: Monday, June 16, 2014 10:12 AM > To: David Rientjes > Cc: r...@rjwysocki.net; l...@kernel.org; nas...@ya.ru; > linux-a...@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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >> +* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 +* 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
Hi, From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Lan Tianyu Sent: Monday, June 16, 2014 10:12 AM To: David Rientjes Cc: r...@rjwysocki.net; l...@kernel.org; nas...@ya.ru; linux-a...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
On 2014年06月16日 10:35, Zheng, Lv wrote: Hi, From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Lan Tianyu Sent: Monday, June 16, 2014 10:12 AM To: David Rientjes Cc: r...@rjwysocki.net; l...@kernel.org; nas...@ya.ru; linux-a...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best regards Tianyu Lan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 Cc: sta...@vger.kernel.org Signed-off-by: Lan Tianyu --- 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 #include #include +#include #include #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(>sysfs_lock); if (acpi_has_method(battery->device->handle, "_BIX")) set_bit(ACPI_BATTERY_XINFO_PRESENT, >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=edit -- Best regards Tianyu Lan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 > >> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 > >> Cc: sta...@vger.kernel.org > >> Signed-off-by: Lan Tianyu > >> --- > >> 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 > >> #include > >> #include > >> +#include > >> #include > >> > >> #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(>sysfs_lock); > >>if (acpi_has_method(battery->device->handle, "_BIX")) > >>set_bit(ACPI_BATTERY_XINFO_PRESENT, >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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 >> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 >> Cc: sta...@vger.kernel.org >> Signed-off-by: Lan Tianyu >> --- >> 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 >> #include >> #include >> +#include >> #include >> >> #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(>sysfs_lock); >> if (acpi_has_method(battery->device->handle, "_BIX")) >> set_bit(ACPI_BATTERY_XINFO_PRESENT, >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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 > Cc: sta...@vger.kernel.org > Signed-off-by: Lan Tianyu > --- > 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 > #include > #include > +#include > #include > > #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(>sysfs_lock); > if (acpi_has_method(battery->device->handle, "_BIX")) > set_bit(ACPI_BATTERY_XINFO_PRESENT, >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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 Cc: sta...@vger.kernel.org Signed-off-by: Lan Tianyu --- 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 #include #include +#include #include #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(>sysfs_lock); if (acpi_has_method(battery->device->handle, "_BIX")) set_bit(ACPI_BATTERY_XINFO_PRESENT, >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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 nas...@ya.ru Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 Cc: sta...@vger.kernel.org Signed-off-by: Lan Tianyu tianyu@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); + retry--; + goto retry_get_info; + } + if (result) goto fail; #ifdef CONFIG_ACPI_PROCFS_POWER -- 1.8.4.rc0.1.g8f6a3e5.dirty -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 nas...@ya.ru Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 Cc: sta...@vger.kernel.org Signed-off-by: Lan Tianyu tianyu@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 nas...@ya.ru Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 Cc: sta...@vger.kernel.org Signed-off-by: Lan Tianyu tianyu@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 -- Best regards Tianyu Lan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 nas...@ya.ru Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 Cc: sta...@vger.kernel.org Signed-off-by: Lan Tianyu tianyu@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 nas...@ya.ru Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75581 Cc: sta...@vger.kernel.org Signed-off-by: Lan Tianyu tianyu@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=139081action=edit -- Best regards Tianyu Lan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/