* [PATCH] asus-laptop: correct error handling in asus_read_brightness() @ 2016-04-16 0:27 Giedrius Statkevičius 2016-04-21 23:09 ` Andy Shevchenko 0 siblings, 1 reply; 4+ messages in thread From: Giedrius Statkevičius @ 2016-04-16 0:27 UTC (permalink / raw) To: corentin.chary, dvhart Cc: acpi4asus-user, platform-driver-x86, linux-kernel, Giedrius Statkevičius It is possible that acpi_evaluate_integer might fail and value would not be set to any value so correct this defect by returning 0 in case of an error. This is also the correct thing to return because the backlight subsystem will print the old value of brightness in this case. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com> --- drivers/platform/x86/asus-laptop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c index 9a69734..15f1311 100644 --- a/drivers/platform/x86/asus-laptop.c +++ b/drivers/platform/x86/asus-laptop.c @@ -775,8 +775,10 @@ static int asus_read_brightness(struct backlight_device *bd) rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET, NULL, &value); - if (ACPI_FAILURE(rv)) + if (ACPI_FAILURE(rv)) { pr_warn("Error reading brightness\n"); + return 0; + } return value; } -- 2.8.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] asus-laptop: correct error handling in asus_read_brightness() 2016-04-16 0:27 [PATCH] asus-laptop: correct error handling in asus_read_brightness() Giedrius Statkevičius @ 2016-04-21 23:09 ` Andy Shevchenko 2016-04-22 7:43 ` Giedrius Statkevičius 0 siblings, 1 reply; 4+ messages in thread From: Andy Shevchenko @ 2016-04-21 23:09 UTC (permalink / raw) To: Giedrius Statkevičius Cc: Corentin Chary, dvhart, acpi4asus-user, platform-driver-x86, linux-kernel On Sat, Apr 16, 2016 at 3:27 AM, Giedrius Statkevičius <giedrius.statkevicius@gmail.com> wrote: > It is possible that acpi_evaluate_integer might fail and value would not be > set to any value so correct this defect by returning 0 in case of an > error. This is also the correct thing to return because the backlight > subsystem will print the old value of brightness in this case. > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com> > --- > drivers/platform/x86/asus-laptop.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c > index 9a69734..15f1311 100644 > --- a/drivers/platform/x86/asus-laptop.c > +++ b/drivers/platform/x86/asus-laptop.c > @@ -775,8 +775,10 @@ static int asus_read_brightness(struct backlight_device *bd) > > rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET, > NULL, &value); > - if (ACPI_FAILURE(rv)) > + if (ACPI_FAILURE(rv)) { > pr_warn("Error reading brightness\n"); > + return 0; > + } This looks like a workaround. I suppose the real fix is to return here an error code and fix all callers, like drivers/video/backlight/backlight.c. > > return value; > } > -- > 2.8.0 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] asus-laptop: correct error handling in asus_read_brightness() 2016-04-21 23:09 ` Andy Shevchenko @ 2016-04-22 7:43 ` Giedrius Statkevičius 2016-04-25 17:46 ` Darren Hart 0 siblings, 1 reply; 4+ messages in thread From: Giedrius Statkevičius @ 2016-04-22 7:43 UTC (permalink / raw) To: Andy Shevchenko Cc: Corentin Chary, dvhart, acpi4asus-user, platform-driver-x86, linux-kernel On Fri, Apr 22, 2016 at 02:09:22AM +0300, Andy Shevchenko wrote: > On Sat, Apr 16, 2016 at 3:27 AM, Giedrius Statkevičius > <giedrius.statkevicius@gmail.com> wrote: > > It is possible that acpi_evaluate_integer might fail and value would not be > > set to any value so correct this defect by returning 0 in case of an > > error. This is also the correct thing to return because the backlight > > subsystem will print the old value of brightness in this case. > > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com> > > --- > > drivers/platform/x86/asus-laptop.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c > > index 9a69734..15f1311 100644 > > --- a/drivers/platform/x86/asus-laptop.c > > +++ b/drivers/platform/x86/asus-laptop.c > > @@ -775,8 +775,10 @@ static int asus_read_brightness(struct backlight_device *bd) > > > > rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET, > > NULL, &value); > > - if (ACPI_FAILURE(rv)) > > + if (ACPI_FAILURE(rv)) { > > pr_warn("Error reading brightness\n"); > > + return 0; > > + } > > This looks like a workaround. > I suppose the real fix is to return here an error code and fix all callers, like > drivers/video/backlight/backlight.c. > It just fixes the behaviour according to the current code in that file. I suppose that would be nice but I don't think it would make any difference because the backlight core code still prints out ->props.brightness in case ops->get_brightness fails. Just the difference would be that now actual error messages are printed in the drivers themselves instead of generic messages from the backlight core. Anyway, I think the current behaviour is more useful because the drivers know better about what has failed. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] asus-laptop: correct error handling in asus_read_brightness() 2016-04-22 7:43 ` Giedrius Statkevičius @ 2016-04-25 17:46 ` Darren Hart 0 siblings, 0 replies; 4+ messages in thread From: Darren Hart @ 2016-04-25 17:46 UTC (permalink / raw) To: Giedrius Statkevičius Cc: Andy Shevchenko, Corentin Chary, acpi4asus-user, platform-driver-x86, linux-kernel On Fri, Apr 22, 2016 at 10:43:30AM +0300, Giedrius Statkevičius wrote: > On Fri, Apr 22, 2016 at 02:09:22AM +0300, Andy Shevchenko wrote: > > On Sat, Apr 16, 2016 at 3:27 AM, Giedrius Statkevičius > > <giedrius.statkevicius@gmail.com> wrote: > > > It is possible that acpi_evaluate_integer might fail and value would not be > > > set to any value so correct this defect by returning 0 in case of an > > > error. This is also the correct thing to return because the backlight > > > subsystem will print the old value of brightness in this case. > > > > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com> > > > --- > > > drivers/platform/x86/asus-laptop.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c > > > index 9a69734..15f1311 100644 > > > --- a/drivers/platform/x86/asus-laptop.c > > > +++ b/drivers/platform/x86/asus-laptop.c > > > @@ -775,8 +775,10 @@ static int asus_read_brightness(struct backlight_device *bd) > > > > > > rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET, > > > NULL, &value); > > > - if (ACPI_FAILURE(rv)) > > > + if (ACPI_FAILURE(rv)) { > > > pr_warn("Error reading brightness\n"); > > > + return 0; > > > + } > > > > This looks like a workaround. > > I suppose the real fix is to return here an error code and fix all callers, like > > drivers/video/backlight/backlight.c. > > > > It just fixes the behaviour according to the current code in that file. I > suppose that would be nice but I don't think it would make any difference > because the backlight core code still prints out ->props.brightness in case > ops->get_brightness fails. Just the difference would be that now actual error > messages are printed in the drivers themselves instead of generic messages from > the backlight core. Anyway, I think the current behaviour is more useful because > the drivers know better about what has failed. > As a matter of practice, based on feedback from the previous pdx86 maintainers, I prefer to accept patches which fix an immediate issue and leave the rework or broader work as a follow-on, or a TODO item. We should look for a good place to document these TODOs. Queued for 4.7. Thanks, -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-04-25 17:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-16 0:27 [PATCH] asus-laptop: correct error handling in asus_read_brightness() Giedrius Statkevičius 2016-04-21 23:09 ` Andy Shevchenko 2016-04-22 7:43 ` Giedrius Statkevičius 2016-04-25 17:46 ` Darren Hart
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.