* [PATCH] hp-wmi: limit hotkey enable @ 2015-08-07 14:56 Kyle Evans 2015-08-28 18:42 ` Darren Hart 0 siblings, 1 reply; 21+ messages in thread From: Kyle Evans @ 2015-08-07 14:56 UTC (permalink / raw) To: platform-driver-x86; +Cc: Kyle Evans 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 <kvans32@gmail.com> --- 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) { + 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; @@ -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(); status = wmi_install_notify_handler(HPWMI_EVENT_GUID, hp_wmi_notify, NULL); -- 1.8.5.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-08-07 14:56 [PATCH] hp-wmi: limit hotkey enable Kyle Evans @ 2015-08-28 18:42 ` Darren Hart 2015-08-29 15:26 ` Kyle Evans 0 siblings, 1 reply; 21+ messages in thread From: Darren Hart @ 2015-08-28 18:42 UTC (permalink / raw) To: Kyle Evans; +Cc: platform-driver-x86, Rafael Wysocki, dvhart 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 <kvans32@gmail.com> 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 <dvhart@infradead.org> (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. 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? Thanks, -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-08-28 18:42 ` Darren Hart @ 2015-08-29 15:26 ` Kyle Evans 2015-09-06 18:03 ` Darren Hart 0 siblings, 1 reply; 21+ messages in thread From: Kyle Evans @ 2015-08-29 15:26 UTC (permalink / raw) To: Darren Hart; +Cc: platform-driver-x86, 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 <kvans32@gmail.com> > > 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 <dvhart@infradead.org> (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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-08-29 15:26 ` Kyle Evans @ 2015-09-06 18:03 ` Darren Hart 2015-09-08 15:58 ` Kyle Evans 2015-09-08 18:29 ` Kyle Evans 0 siblings, 2 replies; 21+ messages in thread From: Darren Hart @ 2015-09-06 18:03 UTC (permalink / raw) To: Kyle Evans; +Cc: platform-driver-x86, Rafael Wysocki On Sat, Aug 29, 2015 at 10:26:40AM -0500, Kyle Evans wrote: > 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 <kvans32@gmail.com> > > > >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 <dvhart@infradead.org> (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; "Reverse Christmas Tree" ordering please (longest to shortest, and it's OK to group like types: int value = 0x6e; int query; int ret; It's also fine to group like types, preferred by some maintainers, I'm not particular, but do appreciate consistency. int value = 0x6e; int query, ret; Both are acceptable. Is there a reason 0x6e doesn't merit some kind of a HP_WMI_QUERY_XYZ define? > > - 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; The problem with this is it doesn't distinguish between the feature not being present, and an actual failure, since it doesn't capture the ret of FEATURE2_QUERY. Consider the possible return values for FEATURE2_QUERY on devices with the feature and devices without, does the above code do the right thing in all cases? > > > > > >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. Right, rolling it together with a semi-related change is OK for things like in my opinion. But do make note of it in the changelog. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-09-06 18:03 ` Darren Hart @ 2015-09-08 15:58 ` Kyle Evans 2015-09-08 18:29 ` Kyle Evans 1 sibling, 0 replies; 21+ messages in thread From: Kyle Evans @ 2015-09-08 15:58 UTC (permalink / raw) To: Darren Hart; +Cc: platform-driver-x86, Rafael Wysocki On 09/06/2015 01:03 PM, Darren Hart wrote: > On Sat, Aug 29, 2015 at 10:26:40AM -0500, Kyle Evans wrote: >> 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 <kvans32@gmail.com> >>> >>> 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 <dvhart@infradead.org> (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; > > "Reverse Christmas Tree" ordering please (longest to shortest, and it's OK to > group like types: > > int value = 0x6e; > int query; > int ret; > > It's also fine to group like types, preferred by some maintainers, I'm not > particular, but do appreciate consistency. > > int value = 0x6e; > int query, ret; > > Both are acceptable. > > Is there a reason 0x6e doesn't merit some kind of a HP_WMI_QUERY_XYZ define? > Probably not. 0x6e is a magic value that goes into a magic EC register, 0xe6. There are a small handful of laptop models that need this value for hotkeys to work. I think these all came out in the 2008 time frame. Models after that period seem to have a different values, but as far as I know, the value is present at boot and the hotkeys just work. I've not heard of anyone else having a broken laptop and needing to write a different value. If one crops up then that is something that should be done, but it does not fit into the context of any existing enum. >> >> - 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; > > The problem with this is it doesn't distinguish between the feature not being > present, and an actual failure, since it doesn't capture the ret of > FEATURE2_QUERY. > > Consider the possible return values for FEATURE2_QUERY on devices with the > feature and devices without, does the above code do the right thing in all > cases? > My line of thinking was that if the feature query fails for any reason then we probably don't want to perform the write. But I do understand building code for future use. How about I break that query out into it's own function like the 2009_later query. I'm not sure what it should be called because I'm not sure what it's actually for or when it actually appeared, but how about hp_wmi_bios_2008_later? > >> >> >>> >>> 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. > > Right, rolling it together with a semi-related change is OK for things like in > my opinion. But do make note of it in the changelog. > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-09-06 18:03 ` Darren Hart 2015-09-08 15:58 ` Kyle Evans @ 2015-09-08 18:29 ` Kyle Evans 2015-09-08 20:22 ` Darren Hart 1 sibling, 1 reply; 21+ messages in thread From: Kyle Evans @ 2015-09-08 18:29 UTC (permalink / raw) To: Darren Hart; +Cc: platform-driver-x86, Rafael Wysocki From 7d11e942d2c84919ded37b46a72be59f34141c5d Mon Sep 17 00:00:00 2001 From: Kyle Evans <kvans32@gmail.com> Date: Tue, 1 Sep 2015 18:50:45 -0500 Subject: [PATCH] hp-wmi: limit hotkey enable Do not write initialize magic on systems that do not have feature query 0xb. Fixes Bug #82451. Define a new feature query to differentiate older systems and rename FEATURE_QUERY, 0xd, to FEATURE2_QUERY for code consistency. Also, some return value magic number cleanup. --- drivers/platform/x86/hp-wmi.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 0669731..c0a7817 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -54,8 +54,9 @@ 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_FEATURE_QUERY 0xb #define HPWMI_HOTKEY_QUERY 0xc -#define HPWMI_FEATURE_QUERY 0xd +#define HPWMI_FEATURE2_QUERY 0xd #define HPWMI_WIRELESS2_QUERY 0x1b #define HPWMI_POSTCODEERROR_QUERY 0x2a @@ -295,7 +296,7 @@ static int hp_wmi_tablet_state(void) return (state & 0x4) ? 1 : 0; } -static int __init hp_wmi_bios_2009_later(void) +static int __init hp_wmi_bios_2008_later(void) { int state = 0; int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, 0, &state, @@ -306,14 +307,22 @@ static int __init hp_wmi_bios_2009_later(void) return (state & 0x10) ? 1 : 0; } -static int hp_wmi_enable_hotkeys(void) +static int __init hp_wmi_bios_2009_later(void) { - int ret; - int query = 0x6e; + int state = 0; + int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &state, + sizeof(state), sizeof(state)); + if (ret) + return ret; - ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), - 0); + return (state & 0x10) ? 1 : 0; +} +static int hp_wmi_enable_hotkeys(void) +{ + int value = 0x6e; + int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, + sizeof(value), 0); if (ret) return -EINVAL; return 0; @@ -663,8 +672,9 @@ 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) - hp_wmi_enable_hotkeys(); + if (hp_wmi_bios_2009_later() == HPWMI_RET_UNKNOWN_CMDTYPE) + if !(hp_wmi_2008_later() == HPWMI_RET_UNKNOWN_CMDTYPE) + hp_wmi_enable_hotkeys(); status = wmi_install_notify_handler(HPWMI_EVENT_GUID, hp_wmi_notify, NULL); if (ACPI_FAILURE(status)) { -- 1.8.5.5 If this is ok I'll add my sign-off and resend. I agree that the return state of hp_wmi_enable_hotkeys is lacking, but the only thing that I see error handling realistically doing in this case is adding code cruft and processing cycles. If you would like anything further, let me know. ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-09-08 18:29 ` Kyle Evans @ 2015-09-08 20:22 ` Darren Hart 2015-09-09 20:32 ` Kyle Evans 0 siblings, 1 reply; 21+ messages in thread From: Darren Hart @ 2015-09-08 20:22 UTC (permalink / raw) To: Kyle Evans; +Cc: platform-driver-x86, Rafael Wysocki On Tue, Sep 08, 2015 at 01:29:39PM -0500, Kyle Evans wrote: > From 7d11e942d2c84919ded37b46a72be59f34141c5d Mon Sep 17 00:00:00 2001 > From: Kyle Evans <kvans32@gmail.com> > Date: Tue, 1 Sep 2015 18:50:45 -0500 > Subject: [PATCH] hp-wmi: limit hotkey enable In the future, please submit as [PATCH v2] as a separate thread. This one took a little bit of minor wrangling to apply coming in as it did. > > Do not write initialize magic on systems that do not have > feature query 0xb. Fixes Bug #82451. > > Define a new feature query to differentiate older systems and rename > FEATURE_QUERY, 0xd, to FEATURE2_QUERY for code consistency. > Also, some return value magic number cleanup. > --- In the future, this is where you should include a changelog: Since v1: - Refactored FEATURE2 test into separate function Or similar. See Documentation/SubmittingPatches section 14. > drivers/platform/x86/hp-wmi.c | 28 +++++++++++++++++++--------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index 0669731..c0a7817 100644 > --- a/drivers/platform/x86/hp-wmi.c > +++ b/drivers/platform/x86/hp-wmi.c > @@ -54,8 +54,9 @@ 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_FEATURE_QUERY 0xb > #define HPWMI_HOTKEY_QUERY 0xc > -#define HPWMI_FEATURE_QUERY 0xd > +#define HPWMI_FEATURE2_QUERY 0xd I didn't understand why you renamed FEATURE to FEATURE2 and then used FEATURE for the new one - rather than just adding FEATURE2. It seems like unecessary churn. > #define HPWMI_WIRELESS2_QUERY 0x1b > #define HPWMI_POSTCODEERROR_QUERY 0x2a > > @@ -295,7 +296,7 @@ static int hp_wmi_tablet_state(void) > return (state & 0x4) ? 1 : 0; > } > > -static int __init hp_wmi_bios_2009_later(void) > +static int __init hp_wmi_bios_2008_later(void) > { > int state = 0; > int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, 0, &state, > @@ -306,14 +307,22 @@ static int __init hp_wmi_bios_2009_later(void) > return (state & 0x10) ? 1 : 0; > } > > -static int hp_wmi_enable_hotkeys(void) > +static int __init hp_wmi_bios_2009_later(void) > { > - int ret; > - int query = 0x6e; > + int state = 0; > + int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &state, > + sizeof(state), sizeof(state)); > + if (ret) > + return ret; > > - ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), > - 0); > + return (state & 0x10) ? 1 : 0; > +} > > +static int hp_wmi_enable_hotkeys(void) > +{ > + int value = 0x6e; > + int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, > + sizeof(value), 0); > if (ret) > return -EINVAL; > return 0; > @@ -663,8 +672,9 @@ 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) > - hp_wmi_enable_hotkeys(); > + if (hp_wmi_bios_2009_later() == HPWMI_RET_UNKNOWN_CMDTYPE) > + if !(hp_wmi_2008_later() == HPWMI_RET_UNKNOWN_CMDTYPE) > + hp_wmi_enable_hotkeys(); I really don't like the semantics behind these hp_wmi_bios_200*_later() calls. They read as though they should be boolean functions, but we test for bizarre magic return codes, and are actually testing for a specific feature. For the uninitiated, the above block is quite difficult to understand what it's testing for. At the very least, it needs a comment describing what is going on. I believe the logic we're looking for is: if older than 2009 (doesn't have FEATURE 0xd) if newer than 2008 (does have FEATURE2 0xb) attempt BIOS_QUERY 0x9 Which one might expect to read as: if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later()) hp_wmi_enable_hotkeys(); Which would involve rewriting the 2009 function and the new 2008 function to return 1 if later, 0 if not, or <0 on error. The ugly details of HPWMI_RET_UNKNOWN_CMDTYPE can be contained within those wrappers, which is arguably the point of a wrapper function - to abstract away things like that. For the original use of hp_wmi_bios_2009_later, only the == 4 case would need to be updated to deal with the boolean logic. Perhaps I'm missing something about how these work, or some corner case, but the above seems far cleaner to me. Would you agree or not? Thanks, -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-09-08 20:22 ` Darren Hart @ 2015-09-09 20:32 ` Kyle Evans 2015-09-10 3:19 ` Darren Hart 0 siblings, 1 reply; 21+ messages in thread From: Kyle Evans @ 2015-09-09 20:32 UTC (permalink / raw) To: Darren Hart; +Cc: platform-driver-x86, Rafael Wysocki On 09/08/2015 04:22 PM, Darren Hart wrote: > On Tue, Sep 08, 2015 at 01:29:39PM -0500, Kyle Evans wrote: >> From 7d11e942d2c84919ded37b46a72be59f34141c5d Mon Sep 17 00:00:00 2001 >> From: Kyle Evans <kvans32@gmail.com> >> Date: Tue, 1 Sep 2015 18:50:45 -0500 >> Subject: [PATCH] hp-wmi: limit hotkey enable > > In the future, please submit as [PATCH v2] as a separate thread. This one took a > little bit of minor wrangling to apply coming in as it did. > >> >> Do not write initialize magic on systems that do not have >> feature query 0xb. Fixes Bug #82451. >> >> Define a new feature query to differentiate older systems and rename >> FEATURE_QUERY, 0xd, to FEATURE2_QUERY for code consistency. >> Also, some return value magic number cleanup. >> --- > > In the future, this is where you should include a changelog: > > Since v1: > - Refactored FEATURE2 test into separate function > > Or similar. See Documentation/SubmittingPatches section 14. > > >> drivers/platform/x86/hp-wmi.c | 28 +++++++++++++++++++--------- >> 1 file changed, 19 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c >> index 0669731..c0a7817 100644 >> --- a/drivers/platform/x86/hp-wmi.c >> +++ b/drivers/platform/x86/hp-wmi.c >> @@ -54,8 +54,9 @@ 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_FEATURE_QUERY 0xb >> #define HPWMI_HOTKEY_QUERY 0xc >> -#define HPWMI_FEATURE_QUERY 0xd >> +#define HPWMI_FEATURE2_QUERY 0xd > > I didn't understand why you renamed FEATURE to FEATURE2 and then used FEATURE > for the new one - rather than just adding FEATURE2. It seems like unecessary > churn. > It is just proactive code cleanup. Since 0xb comes before 0xd it would follow that FEATURE should come before FEATURE2. I wasn't happy being the one to blame for it being jumbled around. >> #define HPWMI_WIRELESS2_QUERY 0x1b >> #define HPWMI_POSTCODEERROR_QUERY 0x2a >> >> @@ -295,7 +296,7 @@ static int hp_wmi_tablet_state(void) >> return (state & 0x4) ? 1 : 0; >> } >> >> -static int __init hp_wmi_bios_2009_later(void) >> +static int __init hp_wmi_bios_2008_later(void) >> { >> int state = 0; >> int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, 0, &state, >> @@ -306,14 +307,22 @@ static int __init hp_wmi_bios_2009_later(void) >> return (state & 0x10) ? 1 : 0; >> } >> >> -static int hp_wmi_enable_hotkeys(void) >> +static int __init hp_wmi_bios_2009_later(void) >> { >> - int ret; >> - int query = 0x6e; >> + int state = 0; >> + int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &state, >> + sizeof(state), sizeof(state)); >> + if (ret) >> + return ret; >> >> - ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), >> - 0); >> + return (state & 0x10) ? 1 : 0; >> +} >> >> +static int hp_wmi_enable_hotkeys(void) >> +{ >> + int value = 0x6e; >> + int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, >> + sizeof(value), 0); >> if (ret) >> return -EINVAL; Would -EIO be a better error code? >> return 0; >> @@ -663,8 +672,9 @@ 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) >> - hp_wmi_enable_hotkeys(); >> + if (hp_wmi_bios_2009_later() == HPWMI_RET_UNKNOWN_CMDTYPE) >> + if !(hp_wmi_2008_later() == HPWMI_RET_UNKNOWN_CMDTYPE) >> + hp_wmi_enable_hotkeys(); > > I really don't like the semantics behind these hp_wmi_bios_200*_later() calls. > They read as though they should be boolean functions, but we test for bizarre > magic return codes, and are actually testing for a specific feature. For the > uninitiated, the above block is quite difficult to understand what it's testing > for. At the very least, it needs a comment describing what is going on. > > I believe the logic we're looking for is: > > if older than 2009 (doesn't have FEATURE 0xd) > if newer than 2008 (does have FEATURE2 0xb) > attempt BIOS_QUERY 0x9 > > Which one might expect to read as: > > if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later()) > hp_wmi_enable_hotkeys(); > > Which would involve rewriting the 2009 function and the new 2008 function to > return 1 if later, 0 if not, or <0 on error. The ugly details of > HPWMI_RET_UNKNOWN_CMDTYPE can be contained within those wrappers, which is > arguably the point of a wrapper function - to abstract away things like that. > > For the original use of hp_wmi_bios_2009_later, only the == 4 case would need to > be updated to deal with the boolean logic. > > Perhaps I'm missing something about how these work, or some corner case, but the > above seems far cleaner to me. > > Would you agree or not? Yes, I agree. I was also confused by the bitwise return logic. However, I did not inspect the DSDT to see what it was doing with state. I only tested the usefulness of the function. Since this now is looking to be the only use for these functions, I am happy to clear them up. > > Thanks, > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-09-09 20:32 ` Kyle Evans @ 2015-09-10 3:19 ` Darren Hart 2015-09-10 16:45 ` [PATCHv3] " Kyle Evans ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Darren Hart @ 2015-09-10 3:19 UTC (permalink / raw) To: Kyle Evans; +Cc: platform-driver-x86, Rafael Wysocki On Wed, Sep 09, 2015 at 04:32:34PM -0400, Kyle Evans wrote: > > > On 09/08/2015 04:22 PM, Darren Hart wrote: > >On Tue, Sep 08, 2015 at 01:29:39PM -0500, Kyle Evans wrote: > >> From 7d11e942d2c84919ded37b46a72be59f34141c5d Mon Sep 17 00:00:00 2001 > >>From: Kyle Evans <kvans32@gmail.com> > >>Date: Tue, 1 Sep 2015 18:50:45 -0500 > >>Subject: [PATCH] hp-wmi: limit hotkey enable > > > >In the future, please submit as [PATCH v2] as a separate thread. This one took a > >little bit of minor wrangling to apply coming in as it did. > > > >> > >>Do not write initialize magic on systems that do not have > >>feature query 0xb. Fixes Bug #82451. > >> > >>Define a new feature query to differentiate older systems and rename > >>FEATURE_QUERY, 0xd, to FEATURE2_QUERY for code consistency. > >>Also, some return value magic number cleanup. > >>--- > > > >In the future, this is where you should include a changelog: > > > >Since v1: > > - Refactored FEATURE2 test into separate function > > > >Or similar. See Documentation/SubmittingPatches section 14. > > > > > >> drivers/platform/x86/hp-wmi.c | 28 +++++++++++++++++++--------- > >> 1 file changed, 19 insertions(+), 9 deletions(-) > >> > >>diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > >>index 0669731..c0a7817 100644 > >>--- a/drivers/platform/x86/hp-wmi.c > >>+++ b/drivers/platform/x86/hp-wmi.c > >>@@ -54,8 +54,9 @@ 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_FEATURE_QUERY 0xb > >> #define HPWMI_HOTKEY_QUERY 0xc > >>-#define HPWMI_FEATURE_QUERY 0xd > >>+#define HPWMI_FEATURE2_QUERY 0xd > > > >I didn't understand why you renamed FEATURE to FEATURE2 and then used FEATURE > >for the new one - rather than just adding FEATURE2. It seems like unecessary > >churn. I'd argue it's worse to change an established define, and I don't see anything wrong with the FEATURE ordering aligning with the encoded value. Not your fault you don't have any documentation, if you had, I'm sure you could have come up with something better than "FEATURE" :-) That said, it's your code, so if you feel strongly about it, there isn't much harm in changing it. Your call. > > > > It is just proactive code cleanup. Since 0xb comes before 0xd it would > follow that FEATURE should come before FEATURE2. I wasn't happy being the > one to blame for it being jumbled around. > > >> #define HPWMI_WIRELESS2_QUERY 0x1b > >> #define HPWMI_POSTCODEERROR_QUERY 0x2a > >> > >>@@ -295,7 +296,7 @@ static int hp_wmi_tablet_state(void) > >> return (state & 0x4) ? 1 : 0; > >> } > >> > >>-static int __init hp_wmi_bios_2009_later(void) > >>+static int __init hp_wmi_bios_2008_later(void) > >> { > >> int state = 0; > >> int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, 0, &state, > >>@@ -306,14 +307,22 @@ static int __init hp_wmi_bios_2009_later(void) > >> return (state & 0x10) ? 1 : 0; > >> } > >> > >>-static int hp_wmi_enable_hotkeys(void) > >>+static int __init hp_wmi_bios_2009_later(void) > >> { > >>- int ret; > >>- int query = 0x6e; > >>+ int state = 0; > >>+ int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &state, > >>+ sizeof(state), sizeof(state)); > >>+ if (ret) > >>+ return ret; > >> > >>- ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), > >>- 0); > >>+ return (state & 0x10) ? 1 : 0; > >>+} > >> > >>+static int hp_wmi_enable_hotkeys(void) > >>+{ > >>+ int value = 0x6e; > >>+ int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, > >>+ sizeof(value), 0); > >> if (ret) > >> return -EINVAL; > > Would -EIO be a better error code? Tough call since we aren't distinguishing between "not supported" (-EIO) and "operation failed" (-ENXIO). In this case, ret!=0 indicates "not supported" I believe, and -EIO would be the most appropriate for that semantic. That said, we ignore the return code anyway .... so perhaps leave this for a separate cleanup :-) > >> return 0; > >>@@ -663,8 +672,9 @@ 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) > >>- hp_wmi_enable_hotkeys(); > >>+ if (hp_wmi_bios_2009_later() == HPWMI_RET_UNKNOWN_CMDTYPE) > >>+ if !(hp_wmi_2008_later() == HPWMI_RET_UNKNOWN_CMDTYPE) > >>+ hp_wmi_enable_hotkeys(); > > > >I really don't like the semantics behind these hp_wmi_bios_200*_later() calls. > >They read as though they should be boolean functions, but we test for bizarre > >magic return codes, and are actually testing for a specific feature. For the > >uninitiated, the above block is quite difficult to understand what it's testing > >for. At the very least, it needs a comment describing what is going on. > > > >I believe the logic we're looking for is: > > > >if older than 2009 (doesn't have FEATURE 0xd) > > if newer than 2008 (does have FEATURE2 0xb) > > attempt BIOS_QUERY 0x9 > > > >Which one might expect to read as: > > > >if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later()) > > hp_wmi_enable_hotkeys(); > > > >Which would involve rewriting the 2009 function and the new 2008 function to > >return 1 if later, 0 if not, or <0 on error. The ugly details of > >HPWMI_RET_UNKNOWN_CMDTYPE can be contained within those wrappers, which is > >arguably the point of a wrapper function - to abstract away things like that. > > > >For the original use of hp_wmi_bios_2009_later, only the == 4 case would need to > >be updated to deal with the boolean logic. > > > >Perhaps I'm missing something about how these work, or some corner case, but the > >above seems far cleaner to me. > > > >Would you agree or not? > > Yes, I agree. I was also confused by the bitwise return logic. However, I > did not inspect the DSDT to see what it was doing with state. I only tested > the usefulness of the function. Since this now is looking to be the only use > for these functions, I am happy to clear them up. Great, I'll look for one more version from you. Thanks for sticking with it. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv3] hp-wmi: limit hotkey enable 2015-09-10 3:19 ` Darren Hart @ 2015-09-10 16:45 ` Kyle Evans 2015-09-10 17:46 ` Darren Hart 2015-09-10 17:45 ` [PATCHv4] " Kyle Evans 2015-09-11 15:40 ` [PATCHv5] " Kyle Evans 2 siblings, 1 reply; 21+ messages in thread From: Kyle Evans @ 2015-09-10 16:45 UTC (permalink / raw) To: Darren Hart; +Cc: platform-driver-x86, Rafael Wysocki From 0e2c0ae37e1fbc326c54a60b2845eb0c7566d389 Mon Sep 17 00:00:00 2001 From: Kyle Evans <kvans32@gmail.com> Date: Tue, 1 Sep 2015 18:50:45 -0500 Subject: [PATCH] hp-wmi: limit hotkey enable Do not write initialize magic on systems that do not have feature query 0xb. Fixes Bug #82451. Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd for code clearity. Add a new test function, hp_wmi_bios_2008_later() & simplify hp_wmi_bios_2009_later(), which fixes a bug in cases where an improper value is returned. May also fix Bug #69131. Signed-off-by: Kyle Evans <kvans32@gmail.com> --- Since v1: - Refactored feature query 0xb into separate function - Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd Since v2: - Simplify hp_wmi_bios_200x_later functions. No longer returns true (4) when the test fails. However, if state is somehow useful, that is lost. drivers/platform/x86/hp-wmi.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 0669731..45a9eaa 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -54,8 +54,9 @@ 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_FEATURE_QUERY 0xb #define HPWMI_HOTKEY_QUERY 0xc -#define HPWMI_FEATURE_QUERY 0xd +#define HPWMI_FEATURE2_QUERY 0xd #define HPWMI_WIRELESS2_QUERY 0x1b #define HPWMI_POSTCODEERROR_QUERY 0x2a @@ -295,25 +296,33 @@ static int hp_wmi_tablet_state(void) return (state & 0x4) ? 1 : 0; } -static int __init hp_wmi_bios_2009_later(void) +static int __init hp_wmi_bios_2008_later(void) { int state = 0; int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, 0, &state, sizeof(state), sizeof(state)); - if (ret) - return ret; + if (!ret) + return 1; - return (state & 0x10) ? 1 : 0; + return (ret == HPWMI_RET_UNKNOWN_CMDTYPE) ? 0 : -ENXIO; } -static int hp_wmi_enable_hotkeys(void) +static int __init hp_wmi_bios_2009_later(void) { - int ret; - int query = 0x6e; + int state = 0; + int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &state, + sizeof(state), sizeof(state)); + if (!ret) + return 1; - ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), - 0); + return (ret == HPWMI_RET_UNKNOWN_CMDTYPE) ? 0 : -ENXIO; +} +static int hp_wmi_enable_hotkeys(void) +{ + int value = 0x6e; + int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, + sizeof(value), 0); if (ret) return -EINVAL; return 0; @@ -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() && hp_wmi_2008_later()) hp_wmi_enable_hotkeys(); status = wmi_install_notify_handler(HPWMI_EVENT_GUID, hp_wmi_notify, NULL); -- 1.8.5.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCHv3] hp-wmi: limit hotkey enable 2015-09-10 16:45 ` [PATCHv3] " Kyle Evans @ 2015-09-10 17:46 ` Darren Hart 2015-09-10 19:47 ` Kyle Evans 0 siblings, 1 reply; 21+ messages in thread From: Darren Hart @ 2015-09-10 17:46 UTC (permalink / raw) To: Kyle Evans; +Cc: platform-driver-x86, Rafael Wysocki On Thu, Sep 10, 2015 at 11:45:05AM -0500, Kyle Evans wrote: > From 0e2c0ae37e1fbc326c54a60b2845eb0c7566d389 Mon Sep 17 00:00:00 2001 > From: Kyle Evans <kvans32@gmail.com> > Date: Tue, 1 Sep 2015 18:50:45 -0500 > Subject: [PATCH] hp-wmi: limit hotkey enable > > Do not write initialize magic on systems that do not have > feature query 0xb. Fixes Bug #82451. > > Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd > for code clearity. > > Add a new test function, hp_wmi_bios_2008_later() & simplify > hp_wmi_bios_2009_later(), which fixes a bug in cases where > an improper value is returned. May also fix Bug #69131. > > Signed-off-by: Kyle Evans <kvans32@gmail.com> Thanks Kyle, I think this looks much better. Unfortunately, it doesn't apply to either Linus' master nor my for-next branch, with all 3 hunks failing even with the more forgiving manual patch application. Please check your base and regenerate based on either Linus' master (the usual approach) or my for-next branch. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHv3] hp-wmi: limit hotkey enable 2015-09-10 17:46 ` Darren Hart @ 2015-09-10 19:47 ` Kyle Evans 0 siblings, 0 replies; 21+ messages in thread From: Kyle Evans @ 2015-09-10 19:47 UTC (permalink / raw) To: Darren Hart; +Cc: platform-driver-x86, Rafael Wysocki On 09/10/2015 12:46 PM, Darren Hart wrote: > On Thu, Sep 10, 2015 at 11:45:05AM -0500, Kyle Evans wrote: >> From 0e2c0ae37e1fbc326c54a60b2845eb0c7566d389 Mon Sep 17 00:00:00 2001 >> From: Kyle Evans <kvans32@gmail.com> >> Date: Tue, 1 Sep 2015 18:50:45 -0500 >> Subject: [PATCH] hp-wmi: limit hotkey enable >> >> Do not write initialize magic on systems that do not have >> feature query 0xb. Fixes Bug #82451. >> >> Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd >> for code clearity. >> >> Add a new test function, hp_wmi_bios_2008_later() & simplify >> hp_wmi_bios_2009_later(), which fixes a bug in cases where >> an improper value is returned. May also fix Bug #69131. >> >> Signed-off-by: Kyle Evans <kvans32@gmail.com> > > Thanks Kyle, > > I think this looks much better. Unfortunately, it doesn't apply to either Linus' > master nor my for-next branch, with all 3 hunks failing even with the more > forgiving manual patch application. Please check your base and regenerate based > on either Linus' master (the usual approach) or my for-next branch. > Please try my v4 patch. It is based on 4.3-rc1. b8889c4fc6ba03e289cec6a4d692f6f080a55e53 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv4] hp-wmi: limit hotkey enable 2015-09-10 3:19 ` Darren Hart 2015-09-10 16:45 ` [PATCHv3] " Kyle Evans @ 2015-09-10 17:45 ` Kyle Evans 2015-09-10 21:21 ` Darren Hart 2015-09-11 15:40 ` [PATCHv5] " Kyle Evans 2 siblings, 1 reply; 21+ messages in thread From: Kyle Evans @ 2015-09-10 17:45 UTC (permalink / raw) To: dvhart; +Cc: platform-driver-x86, rjw, Kyle Evans Do not write initialize magic on systems that do not have feature query 0xb. Fixes Bug #82451. Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd for code clearity. Add a new test function, hp_wmi_bios_2008_later() & simplify hp_wmi_bios_2009_later(), which fixes a bug in cases where an improper value is returned. Probably also fixes Bug #69131. Signed-off-by: Kyle Evans <kvans32@gmail.com> --- Since v1: - Refactored feature query 0xb into separate function - Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd Since v2: - Simplify hp_wmi_bios_200x_later functions. No longer returns true (4) when the test fails. However, if state is somehow useful, that is lost. Since v3: - Fix whitespace, email client reformatting. drivers/platform/x86/hp-wmi.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 0669731..45a9eaa 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -54,8 +54,9 @@ 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_FEATURE_QUERY 0xb #define HPWMI_HOTKEY_QUERY 0xc -#define HPWMI_FEATURE_QUERY 0xd +#define HPWMI_FEATURE2_QUERY 0xd #define HPWMI_WIRELESS2_QUERY 0x1b #define HPWMI_POSTCODEERROR_QUERY 0x2a @@ -295,25 +296,33 @@ static int hp_wmi_tablet_state(void) return (state & 0x4) ? 1 : 0; } -static int __init hp_wmi_bios_2009_later(void) +static int __init hp_wmi_bios_2008_later(void) { int state = 0; int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, 0, &state, sizeof(state), sizeof(state)); - if (ret) - return ret; + if (!ret) + return 1; - return (state & 0x10) ? 1 : 0; + return (ret == HPWMI_RET_UNKNOWN_CMDTYPE) ? 0 : -ENXIO; } -static int hp_wmi_enable_hotkeys(void) +static int __init hp_wmi_bios_2009_later(void) { - int ret; - int query = 0x6e; + int state = 0; + int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &state, + sizeof(state), sizeof(state)); + if (!ret) + return 1; - ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), - 0); + return (ret == HPWMI_RET_UNKNOWN_CMDTYPE) ? 0 : -ENXIO; +} +static int hp_wmi_enable_hotkeys(void) +{ + int value = 0x6e; + int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, + sizeof(value), 0); if (ret) return -EINVAL; return 0; @@ -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() && hp_wmi_2008_later()) hp_wmi_enable_hotkeys(); status = wmi_install_notify_handler(HPWMI_EVENT_GUID, hp_wmi_notify, NULL); -- 1.8.5.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCHv4] hp-wmi: limit hotkey enable 2015-09-10 17:45 ` [PATCHv4] " Kyle Evans @ 2015-09-10 21:21 ` Darren Hart 2015-09-11 15:05 ` Kyle Evans 0 siblings, 1 reply; 21+ messages in thread From: Darren Hart @ 2015-09-10 21:21 UTC (permalink / raw) To: Kyle Evans; +Cc: platform-driver-x86, rjw On Thu, Sep 10, 2015 at 12:45:00PM -0500, Kyle Evans wrote: > Do not write initialize magic on systems that do not have > feature query 0xb. Fixes Bug #82451. > > Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd > for code clearity. > > Add a new test function, hp_wmi_bios_2008_later() & simplify > hp_wmi_bios_2009_later(), which fixes a bug in cases where > an improper value is returned. Probably also fixes Bug #69131. > > Signed-off-by: Kyle Evans <kvans32@gmail.com> > --- > Since v1: > - Refactored feature query 0xb into separate function > - Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd > > Since v2: > - Simplify hp_wmi_bios_200x_later functions. No longer returns true > (4) when the test fails. However, if state is somehow useful, that is lost. > > Since v3: > - Fix whitespace, email client reformatting. Thanks, this one applies cleanly, however: drivers/platform/x86/hp-wmi.c: In function ‘hp_wmi_input_setup’: drivers/platform/x86/hp-wmi.c:675:2: error: implicit declaration of function ‘hp_wmi_2008_later’ [-Werror=implicit-function-declaration] if (!hp_wmi_bios_2009_later() && hp_wmi_2008_later()) ^ drivers/platform/x86/hp-wmi.c: At top level: drivers/platform/x86/hp-wmi.c:299:19: warning: ‘hp_wmi_bios_2008_later’ defined but not used [-Wunused-function] static int __init hp_wmi_bios_2008_later(void) Looks like you missed "bios" in the call to hp_wmi_bios_2008_later. Which suggests this version was not compile tested. As I cannot test the code myself without hardware, I depend on submitters even more to do the testing, so it is really important that you have tested the exact patch that I send to Linus. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHv4] hp-wmi: limit hotkey enable 2015-09-10 21:21 ` Darren Hart @ 2015-09-11 15:05 ` Kyle Evans 0 siblings, 0 replies; 21+ messages in thread From: Kyle Evans @ 2015-09-11 15:05 UTC (permalink / raw) To: Darren Hart; +Cc: platform-driver-x86, rjw On 09/10/2015 04:21 PM, Darren Hart wrote: > On Thu, Sep 10, 2015 at 12:45:00PM -0500, Kyle Evans wrote: >> Do not write initialize magic on systems that do not have >> feature query 0xb. Fixes Bug #82451. >> >> Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd >> for code clearity. >> >> Add a new test function, hp_wmi_bios_2008_later() & simplify >> hp_wmi_bios_2009_later(), which fixes a bug in cases where >> an improper value is returned. Probably also fixes Bug #69131. >> >> Signed-off-by: Kyle Evans <kvans32@gmail.com> >> --- >> Since v1: >> - Refactored feature query 0xb into separate function >> - Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd >> >> Since v2: >> - Simplify hp_wmi_bios_200x_later functions. No longer returns true >> (4) when the test fails. However, if state is somehow useful, that is lost. >> >> Since v3: >> - Fix whitespace, email client reformatting. > > Thanks, this one applies cleanly, however: > > drivers/platform/x86/hp-wmi.c: In function ‘hp_wmi_input_setup’: > drivers/platform/x86/hp-wmi.c:675:2: error: implicit declaration of function ‘hp_wmi_2008_later’ [-Werror=implicit-function-declaration] > if (!hp_wmi_bios_2009_later() && hp_wmi_2008_later()) > ^ > drivers/platform/x86/hp-wmi.c: At top level: > drivers/platform/x86/hp-wmi.c:299:19: warning: ‘hp_wmi_bios_2008_later’ defined but not used [-Wunused-function] > static int __init hp_wmi_bios_2008_later(void) > > Looks like you missed "bios" in the call to hp_wmi_bios_2008_later. Which > suggests this version was not compile tested. As I cannot test the code myself > without hardware, I depend on submitters even more to do the testing, so it is > really important that you have tested the exact patch that I send to Linus. > Sorry for all the rookie mistakes. I did run a compile and reboot script. Unfortunately, the error logic was commented out when I was debugging the reboot code and never re-enabled. Lesson learned. I'm on it. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5] hp-wmi: limit hotkey enable 2015-09-10 3:19 ` Darren Hart 2015-09-10 16:45 ` [PATCHv3] " Kyle Evans 2015-09-10 17:45 ` [PATCHv4] " Kyle Evans @ 2015-09-11 15:40 ` Kyle Evans 2015-09-11 15:55 ` Darren Hart 2 siblings, 1 reply; 21+ messages in thread From: Kyle Evans @ 2015-09-11 15:40 UTC (permalink / raw) To: dvhart; +Cc: platform-driver-x86, rjw, Kyle Evans Do not write initialize magic on systems that do not have feature query 0xb. Fixes Bug #82451. Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd for code clearity. Add a new test function, hp_wmi_bios_2008_later() & simplify hp_wmi_bios_2009_later(), which fixes a bug in cases where an improper value is returned. Probably also fixes Bug #69131. Add missing __init tag. Signed-off-by: Kyle Evans <kvans32@gmail.com> --- Since v1: - Refactored feature query 0xb into separate function - Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd Since v2: - Simplify hp_wmi_bios_200x_later functions. No longer returns true (4) when the test fails. However, if state is somehow useful, that is lost. Since v3: - Fix whitespace, email client reformatting. Since v4: - Fix "bios" typo. - Add __init tag. drivers/platform/x86/hp-wmi.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 0669731..fb4dd7b 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -54,8 +54,9 @@ 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_FEATURE_QUERY 0xb #define HPWMI_HOTKEY_QUERY 0xc -#define HPWMI_FEATURE_QUERY 0xd +#define HPWMI_FEATURE2_QUERY 0xd #define HPWMI_WIRELESS2_QUERY 0x1b #define HPWMI_POSTCODEERROR_QUERY 0x2a @@ -295,25 +296,33 @@ static int hp_wmi_tablet_state(void) return (state & 0x4) ? 1 : 0; } -static int __init hp_wmi_bios_2009_later(void) +static int __init hp_wmi_bios_2008_later(void) { int state = 0; int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, 0, &state, sizeof(state), sizeof(state)); - if (ret) - return ret; + if (!ret) + return 1; - return (state & 0x10) ? 1 : 0; + return (ret == HPWMI_RET_UNKNOWN_CMDTYPE) ? 0 : -ENXIO; } -static int hp_wmi_enable_hotkeys(void) +static int __init hp_wmi_bios_2009_later(void) { - int ret; - int query = 0x6e; + int state = 0; + int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &state, + sizeof(state), sizeof(state)); + if (!ret) + return 1; - ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &query, sizeof(query), - 0); + return (ret == HPWMI_RET_UNKNOWN_CMDTYPE) ? 0 : -ENXIO; +} +static int __init hp_wmi_enable_hotkeys(void) +{ + int value = 0x6e; + int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, + sizeof(value), 0); if (ret) return -EINVAL; return 0; @@ -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() && hp_wmi_bios_2008_later()) hp_wmi_enable_hotkeys(); status = wmi_install_notify_handler(HPWMI_EVENT_GUID, hp_wmi_notify, NULL); -- 1.8.5.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCHv5] hp-wmi: limit hotkey enable 2015-09-11 15:40 ` [PATCHv5] " Kyle Evans @ 2015-09-11 15:55 ` Darren Hart 0 siblings, 0 replies; 21+ messages in thread From: Darren Hart @ 2015-09-11 15:55 UTC (permalink / raw) To: Kyle Evans; +Cc: platform-driver-x86, rjw On Fri, Sep 11, 2015 at 10:40:17AM -0500, Kyle Evans wrote: > Do not write initialize magic on systems that do not have > feature query 0xb. Fixes Bug #82451. > > Redefine FEATURE_QUERY to align with 0xb and FEATURE2 with 0xd > for code clearity. > > Add a new test function, hp_wmi_bios_2008_later() & simplify > hp_wmi_bios_2009_later(), which fixes a bug in cases where > an improper value is returned. Probably also fixes Bug #69131. > > Add missing __init tag. > > Signed-off-by: Kyle Evans <kvans32@gmail.com> Thanks Kyle, pushed to "testing". -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* HP Compaq 6510b FN keys for brightness not working after boot @ 2015-03-13 5:33 Aaron Lu 2015-03-27 18:33 ` [PATCH] hp-wmi: limit hotkey enable Kyle Evans 0 siblings, 1 reply; 21+ messages in thread From: Aaron Lu @ 2015-03-13 5:33 UTC (permalink / raw) To: Kyle Evans; +Cc: platform-driver-x86, Bertrand Vieille, linux-kernel-org Hi Kyle, It seems that your commit: commit f82bdd0d77b6bf0dea08a1d957ab45d503f328b1 Author: Kyle Evans <kvans32@gmail.com> Date: Mon Jun 9 12:26:06 2014 -0500 hp-wmi: Enable hotkeys on some systems has caused some trouble for some HP 6510b users as shown in this bug: https://bugzilla.kernel.org/show_bug.cgi?id=82451 Can you please take a look at that? Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] hp-wmi: limit hotkey enable 2015-03-13 5:33 HP Compaq 6510b FN keys for brightness not working after boot Aaron Lu @ 2015-03-27 18:33 ` Kyle Evans 2015-03-27 20:17 ` vieille.bertrand 2015-04-06 15:57 ` Marcus Pollice 0 siblings, 2 replies; 21+ messages in thread From: Kyle Evans @ 2015-03-27 18:33 UTC (permalink / raw) To: Aaron Lu, platform-driver-x86, Bertrand Vieille; +Cc: Kyle Evans 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. Signed-off-by: Kyle Evans <kvans32@gmail.com> --- drivers/platform/x86/hp-wmi.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 0ab2b37..4a4d6e3 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; + 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 (!ret && !query) { + 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; -- 1.8.5.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-03-27 18:33 ` [PATCH] hp-wmi: limit hotkey enable Kyle Evans @ 2015-03-27 20:17 ` vieille.bertrand 2015-04-06 15:57 ` Marcus Pollice 1 sibling, 0 replies; 21+ messages in thread From: vieille.bertrand @ 2015-03-27 20:17 UTC (permalink / raw) To: platform-driver-x86; +Cc: Aaron Lu, Kyle Evans Patch successfully tested on my HP6510b with 3.19.3 kernel. Thanks Bertrand Vieille ----- Mail original ----- De: "Kyle Evans" <kvans32@gmail.com> À: "Aaron Lu" <aaron.lu@intel.com>, platform-driver-x86@vger.kernel.org, "Bertrand Vieille" <vieille.bertrand@free.Fr> Cc: "Kyle Evans" <kvans32@gmail.com> Envoyé: Vendredi 27 Mars 2015 19:33:11 Objet: [PATCH] hp-wmi: limit hotkey enable 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. Signed-off-by: Kyle Evans <kvans32@gmail.com> --- drivers/platform/x86/hp-wmi.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 0ab2b37..4a4d6e3 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; + 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 (!ret && !query) { + 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; -- 1.8.5.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] hp-wmi: limit hotkey enable 2015-03-27 18:33 ` [PATCH] hp-wmi: limit hotkey enable Kyle Evans 2015-03-27 20:17 ` vieille.bertrand @ 2015-04-06 15:57 ` Marcus Pollice [not found] ` <13CD30DA-0450-4879-89ED-9B5C6D434652@gmail.com> 1 sibling, 1 reply; 21+ messages in thread From: Marcus Pollice @ 2015-04-06 15:57 UTC (permalink / raw) To: Kyle Evans, Aaron Lu, platform-driver-x86, Bertrand Vieille Patch tested successfully on a HP Compaq nx6310 with kernel 3.19, that had the same issue introduced by the same commit. I wonder what I did wrong when reporting the issue to this list almost 3 months prior to Bertrand. I went through the same bisection steps and since he had another issue as well it would have saved him some time. I'd like to know how I can improve reporting such issues in the future. Marcus Pollice On 27.03.2015 19:33, 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. > > Signed-off-by: Kyle Evans <kvans32@gmail.com> > --- > drivers/platform/x86/hp-wmi.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index 0ab2b37..4a4d6e3 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; > + 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 (!ret && !query) { > + 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; > ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <13CD30DA-0450-4879-89ED-9B5C6D434652@gmail.com>]
* Re: [PATCH] hp-wmi: limit hotkey enable [not found] ` <13CD30DA-0450-4879-89ED-9B5C6D434652@gmail.com> @ 2015-06-11 22:08 ` Marcus Pollice 0 siblings, 0 replies; 21+ messages in thread From: Marcus Pollice @ 2015-06-11 22:08 UTC (permalink / raw) To: Kyle Evans, platform-driver-x86 Out of curiosity, when will this patch be mainlined? Marcus Pollice On 07.04.2015 20:37, Kyle Evans wrote: > Did you CC the author, in this case me. If you did I am sorry for not > catching it. If not, that would be the ticket. > > On April 6, 2015 11:57:05 AM EDT, Marcus Pollice > <marcus.pollice@gmail.com> wrote: > > Patch tested successfully on a HP Compaq nx6310 with kernel 3.19, that > had the same issue introduced by the same commit. > > I wonder what I did wrong when reporting the issue to this list almost 3 > months prior to Bertrand. I went through the same bisection steps and > since he had another issue as well it would have saved him some time. > I'd like to know how I can improve reporting such issues in the future. > > Marcus Pollice > > On 27.03.2015 19:33, 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. Signed-off-by: Kyle Evans > <kvans32@gmail.com> --- drivers/platform/x86/hp-wmi.c | 15 > ++++++++++++--- 1 file changed, 12 insertions(+), 3 > deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c > b/drivers/platform/x86/hp-wmi.c index 0ab2b37..4a4d6e3 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; + 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 (!ret && !query) { + 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; > > > > -- > Sent from my Android device with K-9 Mail. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2015-09-11 15:55 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-07 14:56 [PATCH] hp-wmi: limit hotkey enable Kyle Evans 2015-08-28 18:42 ` Darren Hart 2015-08-29 15:26 ` Kyle Evans 2015-09-06 18:03 ` Darren Hart 2015-09-08 15:58 ` Kyle Evans 2015-09-08 18:29 ` Kyle Evans 2015-09-08 20:22 ` Darren Hart 2015-09-09 20:32 ` Kyle Evans 2015-09-10 3:19 ` Darren Hart 2015-09-10 16:45 ` [PATCHv3] " Kyle Evans 2015-09-10 17:46 ` Darren Hart 2015-09-10 19:47 ` Kyle Evans 2015-09-10 17:45 ` [PATCHv4] " Kyle Evans 2015-09-10 21:21 ` Darren Hart 2015-09-11 15:05 ` Kyle Evans 2015-09-11 15:40 ` [PATCHv5] " Kyle Evans 2015-09-11 15:55 ` Darren Hart -- strict thread matches above, loose matches on Subject: below -- 2015-03-13 5:33 HP Compaq 6510b FN keys for brightness not working after boot Aaron Lu 2015-03-27 18:33 ` [PATCH] hp-wmi: limit hotkey enable Kyle Evans 2015-03-27 20:17 ` vieille.bertrand 2015-04-06 15:57 ` Marcus Pollice [not found] ` <13CD30DA-0450-4879-89ED-9B5C6D434652@gmail.com> 2015-06-11 22:08 ` Marcus Pollice
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.