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@xxxxxxxxx>
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@xxxxxxxxxxxxx> (maintainer:X86 PLATFORM DRIVERS)
platform-driver-x86@xxxxxxxxxxxxxxx (open list:X86 PLATFORM DRIVERS)
linux-kernel@xxxxxxxxxxxxxxx (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.
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
