| Message ID | 1438959360-20901-1-git-send-email-kvans32@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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,
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. -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. > -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)