Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Wed, Apr 19, 2017 at 07:23:39PM +0300, Andy Shevchenko wrote: > On Fri, Apr 14, 2017 at 2:07 AM, Darren Hartwrote: > > On Thu, Apr 13, 2017 at 10:09:43PM +0200, Carlo Caione wrote: > >> On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart wrote: > >> > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: > > > EINVAL is being used to broadly here. If the input values are incorrect, > > then > > yes -EINVAL is the right response. However, if the query was unsuccessful, > > that > > is more appropriately -EIO. > > > > If the handle/method doesn't exist, that would be -ENXIO. > > > > However, your changes make the driver self-consistent and I'll apply them > > as is > > to testing. > > Darren, if it's not yet in for-next, I would like Carlo to update his > patch 1. I think it's better if we not shadow negative error code from > the callee. See my response in the V2 Carlo submitted. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Wed, Apr 19, 2017 at 07:23:39PM +0300, Andy Shevchenko wrote: > On Fri, Apr 14, 2017 at 2:07 AM, Darren Hart wrote: > > On Thu, Apr 13, 2017 at 10:09:43PM +0200, Carlo Caione wrote: > >> On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart wrote: > >> > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: > > > EINVAL is being used to broadly here. If the input values are incorrect, > > then > > yes -EINVAL is the right response. However, if the query was unsuccessful, > > that > > is more appropriately -EIO. > > > > If the handle/method doesn't exist, that would be -ENXIO. > > > > However, your changes make the driver self-consistent and I'll apply them > > as is > > to testing. > > Darren, if it's not yet in for-next, I would like Carlo to update his > patch 1. I think it's better if we not shadow negative error code from > the callee. See my response in the V2 Carlo submitted. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Fri, Apr 14, 2017 at 2:07 AM, Darren Hartwrote: > On Thu, Apr 13, 2017 at 10:09:43PM +0200, Carlo Caione wrote: >> On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart wrote: >> > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: > EINVAL is being used to broadly here. If the input values are incorrect, then > yes -EINVAL is the right response. However, if the query was unsuccessful, > that > is more appropriately -EIO. > > If the handle/method doesn't exist, that would be -ENXIO. > > However, your changes make the driver self-consistent and I'll apply them as > is > to testing. Darren, if it's not yet in for-next, I would like Carlo to update his patch 1. I think it's better if we not shadow negative error code from the callee. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Fri, Apr 14, 2017 at 2:07 AM, Darren Hart wrote: > On Thu, Apr 13, 2017 at 10:09:43PM +0200, Carlo Caione wrote: >> On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart wrote: >> > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: > EINVAL is being used to broadly here. If the input values are incorrect, then > yes -EINVAL is the right response. However, if the query was unsuccessful, > that > is more appropriately -EIO. > > If the handle/method doesn't exist, that would be -ENXIO. > > However, your changes make the driver self-consistent and I'll apply them as > is > to testing. Darren, if it's not yet in for-next, I would like Carlo to update his patch 1. I think it's better if we not shadow negative error code from the callee. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Thu, Apr 13, 2017 at 10:09:43PM +0200, Carlo Caione wrote: > On Thu, Apr 13, 2017 at 8:21 PM, Darren Hartwrote: > > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: > >> From: Carlo Caione > > /cut > >> @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void) > >> { > >> acpi_status status; > >> int err; > >> + int val; > >> > >> hp_wmi_input_dev = input_allocate_device(); > >> if (!hp_wmi_input_dev) > >> @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void) > >> hp_wmi_input_dev->id.bustype = BUS_HOST; > >> > >> __set_bit(EV_SW, hp_wmi_input_dev->evbit); > >> - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > >> - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > >> + > >> + /* Dock */ > >> + val = hp_wmi_dock_state(); > >> + if (!(val < 0)) { > >> + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > >> + input_report_switch(hp_wmi_input_dev, SW_DOCK, val); > >> + } > > > > In general, these are fine and can go in. I did want to get your opinion on > > one > > thought though. > > > > This adds some complexity to deal with what appears to be an unknown failure > > mode (the query fails, we don't know why, so we don't set the bit on the > > input > > dev for that feature). Since we don't know why it fails, can we be > > confident it > > will always fail? > > That's not exactly true, at least for the firmware I have on the > laptop I'm working on. > > For this hardware (can we assume for all the HP models?) when the WMI > calls returns the value of 0x04, that means that the query > (HPWMI_HARDWARE_QUERY in this case) is not implemented at all in the > SSDT. > In general reading the disassembled AML code when the WMI query fails > and returns a positive value this can be: > - 0x04: Query ID is unknown / not implemented but valid > - 0x02: Wrong signature > - 0x05: Wrong / invalid query number (?) > > The problem here is that: (1) this is my personal interpretation of > the AML code obtained by disassembling the SSDT and (2) we cannot be > sure that this is the same on all the HP firmwares around. > For sure in general in all the cases I extracted from the SSDT table > on this hardware if the call failed the first time all the chances are > that it is going to fail also in the future. > > In [1] is the SSDT table, the WMI method is WMAA if you want to check > my interpretation. > > > Could it succeed at init here, but then fail later and leave > > us in the same situation we are in now? > > I think that this is really unlikely > > > If so, have you considered just returning 0 on error and using a WARN_ONCE > > print > > statement to report the error? This would simplify a lot of this logic that > > you're adding in here to handle something we could just report and ignore. > > Yes, I thought to report just 0 but in that case we are advertising to > userspace fake capabilities for the hardware, like dockability or > laptop mode that in most cases are not even implemented on the > hardware (like on this laptop). OK, I'm convinced that this approach is correct. > > > That being said, your version avoids the input_report_switch() in the event > > of a > > failure at init. In practice, I don't know if this is worth the added > > complexity. > > > > Your thoughts? > > AFAICT we can fail in hp_wmi_perform_query (as written in the comment > to the function): > 1) with -EINVAL if the query was not successful or the output buffer > size exceeds buffersize. In this case I don't see how the next calls > could be successful. EINVAL is being used to broadly here. If the input values are incorrect, then yes -EINVAL is the right response. However, if the query was unsuccessful, that is more appropriately -EIO. If the handle/method doesn't exist, that would be -ENXIO. However, your changes make the driver self-consistent and I'll apply them as is to testing. You, me, or someone could prepare a follow up series to clean this driver up a bit. > 2) with a positive error code returned from the WMI method. Given my > interpretation of this positive code reported before I don't see why > we should fail only on init and not on all the subsequent calls > > So I'm still convinced that my implementation is correct and that > probably adding complexity on top is not really worth it. But of > course this is your call as maintainer :) Thanks for the additional context. > > Thanks, > > [1] > https://paste.fedoraproject.org/paste/bBnqUlazz1tAjKsJKq7NHl5M1UNdIGYhyRLivL9gydE= > > -- > Carlo Caione | +39.340.80.30.096 | Endless > -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Thu, Apr 13, 2017 at 10:09:43PM +0200, Carlo Caione wrote: > On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart wrote: > > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: > >> From: Carlo Caione > > /cut > >> @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void) > >> { > >> acpi_status status; > >> int err; > >> + int val; > >> > >> hp_wmi_input_dev = input_allocate_device(); > >> if (!hp_wmi_input_dev) > >> @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void) > >> hp_wmi_input_dev->id.bustype = BUS_HOST; > >> > >> __set_bit(EV_SW, hp_wmi_input_dev->evbit); > >> - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > >> - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > >> + > >> + /* Dock */ > >> + val = hp_wmi_dock_state(); > >> + if (!(val < 0)) { > >> + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > >> + input_report_switch(hp_wmi_input_dev, SW_DOCK, val); > >> + } > > > > In general, these are fine and can go in. I did want to get your opinion on > > one > > thought though. > > > > This adds some complexity to deal with what appears to be an unknown failure > > mode (the query fails, we don't know why, so we don't set the bit on the > > input > > dev for that feature). Since we don't know why it fails, can we be > > confident it > > will always fail? > > That's not exactly true, at least for the firmware I have on the > laptop I'm working on. > > For this hardware (can we assume for all the HP models?) when the WMI > calls returns the value of 0x04, that means that the query > (HPWMI_HARDWARE_QUERY in this case) is not implemented at all in the > SSDT. > In general reading the disassembled AML code when the WMI query fails > and returns a positive value this can be: > - 0x04: Query ID is unknown / not implemented but valid > - 0x02: Wrong signature > - 0x05: Wrong / invalid query number (?) > > The problem here is that: (1) this is my personal interpretation of > the AML code obtained by disassembling the SSDT and (2) we cannot be > sure that this is the same on all the HP firmwares around. > For sure in general in all the cases I extracted from the SSDT table > on this hardware if the call failed the first time all the chances are > that it is going to fail also in the future. > > In [1] is the SSDT table, the WMI method is WMAA if you want to check > my interpretation. > > > Could it succeed at init here, but then fail later and leave > > us in the same situation we are in now? > > I think that this is really unlikely > > > If so, have you considered just returning 0 on error and using a WARN_ONCE > > print > > statement to report the error? This would simplify a lot of this logic that > > you're adding in here to handle something we could just report and ignore. > > Yes, I thought to report just 0 but in that case we are advertising to > userspace fake capabilities for the hardware, like dockability or > laptop mode that in most cases are not even implemented on the > hardware (like on this laptop). OK, I'm convinced that this approach is correct. > > > That being said, your version avoids the input_report_switch() in the event > > of a > > failure at init. In practice, I don't know if this is worth the added > > complexity. > > > > Your thoughts? > > AFAICT we can fail in hp_wmi_perform_query (as written in the comment > to the function): > 1) with -EINVAL if the query was not successful or the output buffer > size exceeds buffersize. In this case I don't see how the next calls > could be successful. EINVAL is being used to broadly here. If the input values are incorrect, then yes -EINVAL is the right response. However, if the query was unsuccessful, that is more appropriately -EIO. If the handle/method doesn't exist, that would be -ENXIO. However, your changes make the driver self-consistent and I'll apply them as is to testing. You, me, or someone could prepare a follow up series to clean this driver up a bit. > 2) with a positive error code returned from the WMI method. Given my > interpretation of this positive code reported before I don't see why > we should fail only on init and not on all the subsequent calls > > So I'm still convinced that my implementation is correct and that > probably adding complexity on top is not really worth it. But of > course this is your call as maintainer :) Thanks for the additional context. > > Thanks, > > [1] > https://paste.fedoraproject.org/paste/bBnqUlazz1tAjKsJKq7NHl5M1UNdIGYhyRLivL9gydE= > > -- > Carlo Caione | +39.340.80.30.096 | Endless > -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Thu, Apr 13, 2017 at 8:21 PM, Darren Hartwrote: > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: >> From: Carlo Caione /cut >> @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void) >> { >> acpi_status status; >> int err; >> + int val; >> >> hp_wmi_input_dev = input_allocate_device(); >> if (!hp_wmi_input_dev) >> @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void) >> hp_wmi_input_dev->id.bustype = BUS_HOST; >> >> __set_bit(EV_SW, hp_wmi_input_dev->evbit); >> - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); >> - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); >> + >> + /* Dock */ >> + val = hp_wmi_dock_state(); >> + if (!(val < 0)) { >> + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); >> + input_report_switch(hp_wmi_input_dev, SW_DOCK, val); >> + } > > In general, these are fine and can go in. I did want to get your opinion on > one > thought though. > > This adds some complexity to deal with what appears to be an unknown failure > mode (the query fails, we don't know why, so we don't set the bit on the input > dev for that feature). Since we don't know why it fails, can we be confident > it > will always fail? That's not exactly true, at least for the firmware I have on the laptop I'm working on. For this hardware (can we assume for all the HP models?) when the WMI calls returns the value of 0x04, that means that the query (HPWMI_HARDWARE_QUERY in this case) is not implemented at all in the SSDT. In general reading the disassembled AML code when the WMI query fails and returns a positive value this can be: - 0x04: Query ID is unknown / not implemented but valid - 0x02: Wrong signature - 0x05: Wrong / invalid query number (?) The problem here is that: (1) this is my personal interpretation of the AML code obtained by disassembling the SSDT and (2) we cannot be sure that this is the same on all the HP firmwares around. For sure in general in all the cases I extracted from the SSDT table on this hardware if the call failed the first time all the chances are that it is going to fail also in the future. In [1] is the SSDT table, the WMI method is WMAA if you want to check my interpretation. > Could it succeed at init here, but then fail later and leave > us in the same situation we are in now? I think that this is really unlikely > If so, have you considered just returning 0 on error and using a WARN_ONCE > print > statement to report the error? This would simplify a lot of this logic that > you're adding in here to handle something we could just report and ignore. Yes, I thought to report just 0 but in that case we are advertising to userspace fake capabilities for the hardware, like dockability or laptop mode that in most cases are not even implemented on the hardware (like on this laptop). > That being said, your version avoids the input_report_switch() in the event > of a > failure at init. In practice, I don't know if this is worth the added > complexity. > > Your thoughts? AFAICT we can fail in hp_wmi_perform_query (as written in the comment to the function): 1) with -EINVAL if the query was not successful or the output buffer size exceeds buffersize. In this case I don't see how the next calls could be successful. 2) with a positive error code returned from the WMI method. Given my interpretation of this positive code reported before I don't see why we should fail only on init and not on all the subsequent calls So I'm still convinced that my implementation is correct and that probably adding complexity on top is not really worth it. But of course this is your call as maintainer :) Thanks, [1] https://paste.fedoraproject.org/paste/bBnqUlazz1tAjKsJKq7NHl5M1UNdIGYhyRLivL9gydE= -- Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart wrote: > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: >> From: Carlo Caione /cut >> @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void) >> { >> acpi_status status; >> int err; >> + int val; >> >> hp_wmi_input_dev = input_allocate_device(); >> if (!hp_wmi_input_dev) >> @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void) >> hp_wmi_input_dev->id.bustype = BUS_HOST; >> >> __set_bit(EV_SW, hp_wmi_input_dev->evbit); >> - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); >> - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); >> + >> + /* Dock */ >> + val = hp_wmi_dock_state(); >> + if (!(val < 0)) { >> + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); >> + input_report_switch(hp_wmi_input_dev, SW_DOCK, val); >> + } > > In general, these are fine and can go in. I did want to get your opinion on > one > thought though. > > This adds some complexity to deal with what appears to be an unknown failure > mode (the query fails, we don't know why, so we don't set the bit on the input > dev for that feature). Since we don't know why it fails, can we be confident > it > will always fail? That's not exactly true, at least for the firmware I have on the laptop I'm working on. For this hardware (can we assume for all the HP models?) when the WMI calls returns the value of 0x04, that means that the query (HPWMI_HARDWARE_QUERY in this case) is not implemented at all in the SSDT. In general reading the disassembled AML code when the WMI query fails and returns a positive value this can be: - 0x04: Query ID is unknown / not implemented but valid - 0x02: Wrong signature - 0x05: Wrong / invalid query number (?) The problem here is that: (1) this is my personal interpretation of the AML code obtained by disassembling the SSDT and (2) we cannot be sure that this is the same on all the HP firmwares around. For sure in general in all the cases I extracted from the SSDT table on this hardware if the call failed the first time all the chances are that it is going to fail also in the future. In [1] is the SSDT table, the WMI method is WMAA if you want to check my interpretation. > Could it succeed at init here, but then fail later and leave > us in the same situation we are in now? I think that this is really unlikely > If so, have you considered just returning 0 on error and using a WARN_ONCE > print > statement to report the error? This would simplify a lot of this logic that > you're adding in here to handle something we could just report and ignore. Yes, I thought to report just 0 but in that case we are advertising to userspace fake capabilities for the hardware, like dockability or laptop mode that in most cases are not even implemented on the hardware (like on this laptop). > That being said, your version avoids the input_report_switch() in the event > of a > failure at init. In practice, I don't know if this is worth the added > complexity. > > Your thoughts? AFAICT we can fail in hp_wmi_perform_query (as written in the comment to the function): 1) with -EINVAL if the query was not successful or the output buffer size exceeds buffersize. In this case I don't see how the next calls could be successful. 2) with a positive error code returned from the WMI method. Given my interpretation of this positive code reported before I don't see why we should fail only on init and not on all the subsequent calls So I'm still convinced that my implementation is correct and that probably adding complexity on top is not really worth it. But of course this is your call as maintainer :) Thanks, [1] https://paste.fedoraproject.org/paste/bBnqUlazz1tAjKsJKq7NHl5M1UNdIGYhyRLivL9gydE= -- Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: > From: Carlo Caione> > The current driver code is not checking for the error values returned by > 'hp_wmi_dock_state()' and 'hp_wmi_tablet_state()' before passing the > returned values down to 'input_report_switch()'. This error code is > being translated to '1' in the input subsystem, reporting the wrong > status. > > The biggest problem caused by this issue is that several laptops are > wrongly reported by the driver as docked, preventing them to be put to > sleep using the LID (and in most cases they are not even dockable). > > With this patch we create the report switches only if we are able to > read the dock and tablet mode status correctly from ACPI. > > Signed-off-by: Carlo Caione > --- > drivers/platform/x86/hp-wmi.c | 40 +++- > 1 file changed, 27 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index 13ba65c..2b721fd 100644 > --- a/drivers/platform/x86/hp-wmi.c > +++ b/drivers/platform/x86/hp-wmi.c > @@ -572,10 +572,12 @@ static void hp_wmi_notify(u32 value, void *context) > > switch (event_id) { > case HPWMI_DOCK_EVENT: > - input_report_switch(hp_wmi_input_dev, SW_DOCK, > - hp_wmi_dock_state()); > - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, > - hp_wmi_tablet_state()); > + if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit)) > + input_report_switch(hp_wmi_input_dev, SW_DOCK, > + hp_wmi_dock_state()); > + if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit)) > + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, > + hp_wmi_tablet_state()); > input_sync(hp_wmi_input_dev); > break; > case HPWMI_PARK_HDD: > @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void) > { > acpi_status status; > int err; > + int val; > > hp_wmi_input_dev = input_allocate_device(); > if (!hp_wmi_input_dev) > @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void) > hp_wmi_input_dev->id.bustype = BUS_HOST; > > __set_bit(EV_SW, hp_wmi_input_dev->evbit); > - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > + > + /* Dock */ > + val = hp_wmi_dock_state(); > + if (!(val < 0)) { > + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > + input_report_switch(hp_wmi_input_dev, SW_DOCK, val); > + } In general, these are fine and can go in. I did want to get your opinion on one thought though. This adds some complexity to deal with what appears to be an unknown failure mode (the query fails, we don't know why, so we don't set the bit on the input dev for that feature). Since we don't know why it fails, can we be confident it will always fail? Could it succeed at init here, but then fail later and leave us in the same situation we are in now? If so, have you considered just returning 0 on error and using a WARN_ONCE print statement to report the error? This would simplify a lot of this logic that you're adding in here to handle something we could just report and ignore. That being said, your version avoids the input_report_switch() in the event of a failure at init. In practice, I don't know if this is worth the added complexity. Your thoughts? > + > + /* Tablet mode */ > + val = hp_wmi_tablet_state(); > + if (!(val < 0)) { > + __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); > + } > > err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL); > if (err) > goto err_free_dev; > > /* Set initial hardware state */ > - input_report_switch(hp_wmi_input_dev, SW_DOCK, hp_wmi_dock_state()); > - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, > - hp_wmi_tablet_state()); > input_sync(hp_wmi_input_dev); > > if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later()) > @@ -950,10 +962,12 @@ static int hp_wmi_resume_handler(struct device *device) >* changed. >*/ > if (hp_wmi_input_dev) { > - input_report_switch(hp_wmi_input_dev, SW_DOCK, > - hp_wmi_dock_state()); > - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, > - hp_wmi_tablet_state()); > + if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit)) > + input_report_switch(hp_wmi_input_dev, SW_DOCK, > + hp_wmi_dock_state()); > + if (test_bit(SW_TABLET_MODE,
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote: > From: Carlo Caione > > The current driver code is not checking for the error values returned by > 'hp_wmi_dock_state()' and 'hp_wmi_tablet_state()' before passing the > returned values down to 'input_report_switch()'. This error code is > being translated to '1' in the input subsystem, reporting the wrong > status. > > The biggest problem caused by this issue is that several laptops are > wrongly reported by the driver as docked, preventing them to be put to > sleep using the LID (and in most cases they are not even dockable). > > With this patch we create the report switches only if we are able to > read the dock and tablet mode status correctly from ACPI. > > Signed-off-by: Carlo Caione > --- > drivers/platform/x86/hp-wmi.c | 40 +++- > 1 file changed, 27 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index 13ba65c..2b721fd 100644 > --- a/drivers/platform/x86/hp-wmi.c > +++ b/drivers/platform/x86/hp-wmi.c > @@ -572,10 +572,12 @@ static void hp_wmi_notify(u32 value, void *context) > > switch (event_id) { > case HPWMI_DOCK_EVENT: > - input_report_switch(hp_wmi_input_dev, SW_DOCK, > - hp_wmi_dock_state()); > - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, > - hp_wmi_tablet_state()); > + if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit)) > + input_report_switch(hp_wmi_input_dev, SW_DOCK, > + hp_wmi_dock_state()); > + if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit)) > + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, > + hp_wmi_tablet_state()); > input_sync(hp_wmi_input_dev); > break; > case HPWMI_PARK_HDD: > @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void) > { > acpi_status status; > int err; > + int val; > > hp_wmi_input_dev = input_allocate_device(); > if (!hp_wmi_input_dev) > @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void) > hp_wmi_input_dev->id.bustype = BUS_HOST; > > __set_bit(EV_SW, hp_wmi_input_dev->evbit); > - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > + > + /* Dock */ > + val = hp_wmi_dock_state(); > + if (!(val < 0)) { > + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > + input_report_switch(hp_wmi_input_dev, SW_DOCK, val); > + } In general, these are fine and can go in. I did want to get your opinion on one thought though. This adds some complexity to deal with what appears to be an unknown failure mode (the query fails, we don't know why, so we don't set the bit on the input dev for that feature). Since we don't know why it fails, can we be confident it will always fail? Could it succeed at init here, but then fail later and leave us in the same situation we are in now? If so, have you considered just returning 0 on error and using a WARN_ONCE print statement to report the error? This would simplify a lot of this logic that you're adding in here to handle something we could just report and ignore. That being said, your version avoids the input_report_switch() in the event of a failure at init. In practice, I don't know if this is worth the added complexity. Your thoughts? > + > + /* Tablet mode */ > + val = hp_wmi_tablet_state(); > + if (!(val < 0)) { > + __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); > + } > > err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL); > if (err) > goto err_free_dev; > > /* Set initial hardware state */ > - input_report_switch(hp_wmi_input_dev, SW_DOCK, hp_wmi_dock_state()); > - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, > - hp_wmi_tablet_state()); > input_sync(hp_wmi_input_dev); > > if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later()) > @@ -950,10 +962,12 @@ static int hp_wmi_resume_handler(struct device *device) >* changed. >*/ > if (hp_wmi_input_dev) { > - input_report_switch(hp_wmi_input_dev, SW_DOCK, > - hp_wmi_dock_state()); > - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, > - hp_wmi_tablet_state()); > + if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit)) > + input_report_switch(hp_wmi_input_dev, SW_DOCK, > + hp_wmi_dock_state()); > + if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit)) > +
[PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
From: Carlo CaioneThe current driver code is not checking for the error values returned by 'hp_wmi_dock_state()' and 'hp_wmi_tablet_state()' before passing the returned values down to 'input_report_switch()'. This error code is being translated to '1' in the input subsystem, reporting the wrong status. The biggest problem caused by this issue is that several laptops are wrongly reported by the driver as docked, preventing them to be put to sleep using the LID (and in most cases they are not even dockable). With this patch we create the report switches only if we are able to read the dock and tablet mode status correctly from ACPI. Signed-off-by: Carlo Caione --- drivers/platform/x86/hp-wmi.c | 40 +++- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 13ba65c..2b721fd 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -572,10 +572,12 @@ static void hp_wmi_notify(u32 value, void *context) switch (event_id) { case HPWMI_DOCK_EVENT: - input_report_switch(hp_wmi_input_dev, SW_DOCK, - hp_wmi_dock_state()); - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, - hp_wmi_tablet_state()); + if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit)) + input_report_switch(hp_wmi_input_dev, SW_DOCK, + hp_wmi_dock_state()); + if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit)) + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, + hp_wmi_tablet_state()); input_sync(hp_wmi_input_dev); break; case HPWMI_PARK_HDD: @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void) { acpi_status status; int err; + int val; hp_wmi_input_dev = input_allocate_device(); if (!hp_wmi_input_dev) @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void) hp_wmi_input_dev->id.bustype = BUS_HOST; __set_bit(EV_SW, hp_wmi_input_dev->evbit); - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); + + /* Dock */ + val = hp_wmi_dock_state(); + if (!(val < 0)) { + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); + input_report_switch(hp_wmi_input_dev, SW_DOCK, val); + } + + /* Tablet mode */ + val = hp_wmi_tablet_state(); + if (!(val < 0)) { + __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); + } err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL); if (err) goto err_free_dev; /* Set initial hardware state */ - input_report_switch(hp_wmi_input_dev, SW_DOCK, hp_wmi_dock_state()); - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, - hp_wmi_tablet_state()); input_sync(hp_wmi_input_dev); if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later()) @@ -950,10 +962,12 @@ static int hp_wmi_resume_handler(struct device *device) * changed. */ if (hp_wmi_input_dev) { - input_report_switch(hp_wmi_input_dev, SW_DOCK, - hp_wmi_dock_state()); - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, - hp_wmi_tablet_state()); + if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit)) + input_report_switch(hp_wmi_input_dev, SW_DOCK, + hp_wmi_dock_state()); + if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit)) + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, + hp_wmi_tablet_state()); input_sync(hp_wmi_input_dev); } -- 2.9.3
[PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
From: Carlo Caione The current driver code is not checking for the error values returned by 'hp_wmi_dock_state()' and 'hp_wmi_tablet_state()' before passing the returned values down to 'input_report_switch()'. This error code is being translated to '1' in the input subsystem, reporting the wrong status. The biggest problem caused by this issue is that several laptops are wrongly reported by the driver as docked, preventing them to be put to sleep using the LID (and in most cases they are not even dockable). With this patch we create the report switches only if we are able to read the dock and tablet mode status correctly from ACPI. Signed-off-by: Carlo Caione --- drivers/platform/x86/hp-wmi.c | 40 +++- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 13ba65c..2b721fd 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -572,10 +572,12 @@ static void hp_wmi_notify(u32 value, void *context) switch (event_id) { case HPWMI_DOCK_EVENT: - input_report_switch(hp_wmi_input_dev, SW_DOCK, - hp_wmi_dock_state()); - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, - hp_wmi_tablet_state()); + if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit)) + input_report_switch(hp_wmi_input_dev, SW_DOCK, + hp_wmi_dock_state()); + if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit)) + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, + hp_wmi_tablet_state()); input_sync(hp_wmi_input_dev); break; case HPWMI_PARK_HDD: @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void) { acpi_status status; int err; + int val; hp_wmi_input_dev = input_allocate_device(); if (!hp_wmi_input_dev) @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void) hp_wmi_input_dev->id.bustype = BUS_HOST; __set_bit(EV_SW, hp_wmi_input_dev->evbit); - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); + + /* Dock */ + val = hp_wmi_dock_state(); + if (!(val < 0)) { + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); + input_report_switch(hp_wmi_input_dev, SW_DOCK, val); + } + + /* Tablet mode */ + val = hp_wmi_tablet_state(); + if (!(val < 0)) { + __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); + } err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL); if (err) goto err_free_dev; /* Set initial hardware state */ - input_report_switch(hp_wmi_input_dev, SW_DOCK, hp_wmi_dock_state()); - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, - hp_wmi_tablet_state()); input_sync(hp_wmi_input_dev); if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later()) @@ -950,10 +962,12 @@ static int hp_wmi_resume_handler(struct device *device) * changed. */ if (hp_wmi_input_dev) { - input_report_switch(hp_wmi_input_dev, SW_DOCK, - hp_wmi_dock_state()); - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, - hp_wmi_tablet_state()); + if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit)) + input_report_switch(hp_wmi_input_dev, SW_DOCK, + hp_wmi_dock_state()); + if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit)) + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, + hp_wmi_tablet_state()); input_sync(hp_wmi_input_dev); } -- 2.9.3