Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753596AbcDVHng (ORCPT ); Fri, 22 Apr 2016 03:43:36 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:36781 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751851AbcDVHne (ORCPT ); Fri, 22 Apr 2016 03:43:34 -0400 Date: Fri, 22 Apr 2016 10:43:30 +0300 From: Giedrius =?utf-8?Q?Statkevi=C4=8Dius?= To: Andy Shevchenko Cc: Corentin Chary , "dvhart@infradead.org" , acpi4asus-user@lists.sourceforge.net, platform-driver-x86@vger.kernel.org, "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] asus-laptop: correct error handling in asus_read_brightness() Message-ID: <20160422074329.GA15499@tyrael> References: <1460766432-5390-1-git-send-email-giedrius.statkevicius@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1930 Lines: 39 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 > 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 > > --- > > 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.