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@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;
"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.
--
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
