From: Darren Hart <dvhart@infradead.org>
To: Carlo Caione <carlo@endlessm.com>
Cc: Carlo Caione <carlo@caione.org>,
andy@infradead.org, platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org,
Linux Upstreaming Team <linux@endlessm.com>
Subject: Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
Date: Thu, 13 Apr 2017 16:07:49 -0700 [thread overview]
Message-ID: <20170413230749.GA11189@fury> (raw)
In-Reply-To: <CAL9uMOG_3Ov6+bgDoHWYUgSKG9YbXs-vk0XsnOYAt+rDV48e7w@mail.gmail.com>
On Thu, Apr 13, 2017 at 10:09:43PM +0200, Carlo Caione wrote:
> On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart <dvhart@infradead.org> wrote:
> > On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote:
> >> From: Carlo Caione <carlo@endlessm.com>
>
> /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
next prev parent reply other threads:[~2017-04-13 23:07 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-09 13:56 [PATCH 0/2] hp-wmi: Fix dock status and tablet mode reporting Carlo Caione
2017-04-09 13:56 ` [PATCH 1/2] hp-wmi: Fix error value for hp_wmi_tablet_state Carlo Caione
2017-04-19 16:21 ` Andy Shevchenko
2017-04-19 16:24 ` Carlo Caione
2017-04-19 16:26 ` Andy Shevchenko
2017-04-09 13:56 ` [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode Carlo Caione
2017-04-13 18:21 ` Darren Hart
2017-04-13 20:09 ` Carlo Caione
2017-04-13 23:07 ` Darren Hart [this message]
2017-04-19 16:23 ` Andy Shevchenko
2017-04-19 20:12 ` Darren Hart
2017-04-13 6:28 ` [PATCH 0/2] hp-wmi: Fix dock status and tablet mode reporting Carlo Caione
2017-04-13 17:23 ` Darren Hart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170413230749.GA11189@fury \
--to=dvhart@infradead.org \
--cc=andy@infradead.org \
--cc=carlo@caione.org \
--cc=carlo@endlessm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@endlessm.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).