From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kyle Evans Subject: Re: [PATCH] hp-wmi: limit hotkey enable Date: Sat, 29 Aug 2015 10:26:40 -0500 Message-ID: <55E1CF30.10501@gmail.com> References: <1438959360-20901-1-git-send-email-kvans32@gmail.com> <20150828184228.GA64484@vmdeb7> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-io0-f179.google.com ([209.85.223.179]:32826 "EHLO mail-io0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753030AbbH2P0m (ORCPT ); Sat, 29 Aug 2015 11:26:42 -0400 Received: by iods203 with SMTP id s203so120847706iod.0 for ; Sat, 29 Aug 2015 08:26:42 -0700 (PDT) In-Reply-To: <20150828184228.GA64484@vmdeb7> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Darren Hart Cc: platform-driver-x86@vger.kernel.org, Rafael Wysocki On 08/28/2015 01:42 PM, Darren Hart wrote: > On Fri, Aug 07, 2015 at 09:56:00AM -0500, Kyle Evans wrote: >> Do not attempt to initialize hotkeys if the query returns a value. >> Furthermore, do not write initialize magic on systems that do not have >> feature query 0xb. Fixes Bug #82451. >> >> Signed-off-by: Kyle Evans > > Hi Kyle, > > Please always include the maintainer from MAINTAINERS on Cc when submitting > kernel patches. See Documentation/SubmittingPatches. > > For example: > > $ scripts/get_maintainer.pl -f drivers/platform/x86/hp-wmi.c > Darren Hart (maintainer:X86 PLATFORM DRIVERS) > platform-driver-x86@vger.kernel.org (open list:X86 PLATFORM DRIVERS) > linux-kernel@vger.kernel.org (open list) > > This will ensure a more timely response. > >> --- >> drivers/platform/x86/hp-wmi.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c >> index 0669731..557650f 100644 >> --- a/drivers/platform/x86/hp-wmi.c >> +++ b/drivers/platform/x86/hp-wmi.c >> @@ -54,6 +54,7 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4"); >> #define HPWMI_HARDWARE_QUERY 0x4 >> #define HPWMI_WIRELESS_QUERY 0x5 >> #define HPWMI_BIOS_QUERY 0x9 >> +#define HPWMI_FEATURE2_QUERY 0xb >> #define HPWMI_HOTKEY_QUERY 0xc >> #define HPWMI_FEATURE_QUERY 0xd >> #define HPWMI_WIRELESS2_QUERY 0x1b >> @@ -309,10 +310,18 @@ static int __init hp_wmi_bios_2009_later(void) >> static int hp_wmi_enable_hotkeys(void) >> { >> int ret; >> - int query = 0x6e; >> + int query = 0xff; >> + int value = 0x6e; >> >> - ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), >> - 0); >> + ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 0, &query, >> + 0, sizeof(query)); >> + >> + if (!query) { > > I suspect this should come after the test for ret. If there is a more > fundamental error, it would make sense to exit with -EINVAL first. Despite query > being initialized to 0xff, we have no guarantee the firmware won't set it to 0 > and still return an error. That makes sense. Another sticky bit is, do we want to fail on a device that doesn't need this? Not really. How about I throw out the initial read, because, the test for FEATURE2_QUERY is the bit that actually fixes the bug. The read is just fearful bug prevention. How is this? @@ -309,10 +310,13 @@ static int __init hp_wmi_bios_2009_later(void) static int hp_wmi_enable_hotkeys(void) { int ret; - int query = 0x6e; + int query; + int value = 0x6e; - ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), - 0); + if (!hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &query, + 0, sizeof(query))) + ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, + sizeof(value), 0); if (ret) return -EINVAL; > > Rafael, would you agree? > > And technically, EINVAL isn't the right error for a general error (but that's a > preexisting problem). You don't have to fix that to get this in. > > >> + if (!hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &query, >> + 0, sizeof(query))) >> + ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, > > Careful with indentation, use tabs please. checkpatch.pl would have caught this. > > >> + sizeof(value), 0); >> + } >> >> if (ret) >> return -EINVAL; >> @@ -663,7 +672,7 @@ static int __init hp_wmi_input_setup(void) >> hp_wmi_tablet_state()); >> input_sync(hp_wmi_input_dev); >> >> - if (hp_wmi_bios_2009_later() == 4) >> + if (hp_wmi_bios_2009_later() == HPWMI_RET_UNKNOWN_CMDTYPE) >> hp_wmi_enable_hotkeys(); > > This bit is fine, but no magic number cleanup is mentioned in the change log. > Was this change intentional? It was intentional but I didn't think it was worth a patch request. I had forgot that I made the change when creating a new patch and was on the fence about what to do about it so I didn't do anything. I'll be sure to call out that sort of thing in the future. > > Thanks, > Thank you for the comments. I'm taking it in.