HP Compaq 6510b FN keys for brightness not working after boot

HP Compaq 6510b FN keys for brightness not working after boot

[Aaron Lu]

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.

[PATCH] hp-wmi: limit hotkey enable

[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

Re: [PATCH] hp-wmi: limit hotkey enable

[vieille.bertrand]

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

Re: [PATCH] hp-wmi: limit hotkey enable

[Marcus Pollice]

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;
> 

Re: [PATCH] hp-wmi: limit hotkey enable

[Marcus Pollice]

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. 

Re: [PATCH] hp-wmi: limit hotkey enable

[Darren Hart]

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

Re: [PATCH] hp-wmi: limit hotkey enable

[Kyle Evans]

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,
>

Re: [PATCH] hp-wmi: limit hotkey enable

[Darren Hart]

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

Re: [PATCH] hp-wmi: limit hotkey enable

[Kyle Evans]

 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.

Re: [PATCH] hp-wmi: limit hotkey enable

[Kyle Evans]

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.
>

Re: [PATCH] hp-wmi: limit hotkey enable

[Darren Hart]

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

Re: [PATCH] hp-wmi: limit hotkey enable

[Kyle Evans]

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.

Re: [PATCH] hp-wmi: limit hotkey enable

[Darren Hart]

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

[PATCH] hp-wmi: limit hotkey enable

[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