Re: [PATCH 04/11] platform/x86: asus-wmi: Add quirk to force DSTS WMI method detection
Wow that is a great thing you done, thanks a lot for your time and your
kind words! Your suggestion indeed sounds good judging by your results.
Both of my devices have UID "ATK" (actually FX505 also has two other
PNP0C14 devices with HIDs SampleDev and TestDev, but I think they are
disabled by default).
It looks like the driver is loaded already only if ASUS_WMI_MGMT_GUID is
present, so I guess that could be used for wmi_driver_register. A little
obstacle is that ACPI device pointer is stored in wmi_block structure,
which is internal to the wmi.c. This means we would have to add a new
method to wmi.h / wmi.c for getting the UID of WMI ACPI device.
I guess it might be possible to somehow navigate the device tree back to
the platform driver of WMI module, but it would definitely be more obscure,
and searching for the device by HID again is not only slower but generally
would require a guarantee that it's the same device. So adding a new
function looks reasonable to me. It might also be useful to someone
implementing similar things for other vendors in the future.
I'm going to implement this in updated patch.
Thanks,
Yurii
On 11.04.19 12:55, Daniel Drake wrote:
> On Thu, Apr 11, 2019 at 4:28 AM Yurii Pavlovskyi
> wrote:
>> The DSTS method detection fails, as nothing is returned if method is not
>> defined in WMNB. As a result the control of keyboard backlight is not
>> functional for TUF Gaming series laptops (at the time the only
>> functionality of the driver on this model implemented with WMI methods).
>>
>> Patch was tested on a newer TUF Gaming FX505GM and older K54C model.
>>
>> FX505GM:
>> Method (WMNB, 3, Serialized)
>> { ...
>> If ((Local0 == 0x53545344))
>> {
>> ...
>> Return (Zero)
>> }
>> ...
>> // No return
>> }
>>
>> K54C:
>> Method (WMNB, 3, Serialized)
>> { ...
>> If ((Local0 == 0x53545344))
>> {
>> ...
>> Return (0x02)
>> }
>> ...
>> Return (0xFFFE)
>> }
>>
>> The non-existing method ASUS_WMI_METHODID_DSTS=0x53544344 (actually it is
>> DCTS in little endian ASCII) is selected in asus->dsts.
>>
>> One way to fix this would be to call both for every known device ID until
>> some answers - this would increase module load time.
>>
>> Another option is to check some device that is known to exist on every
>> model - none known at the time.
>>
>> Last option, which is implemented, is to check for presence of the
>> ASUS7000 device in ACPI tree (it is a dummy device), which is the
>> condition used for loading the vendor driver for this model. This might
>> not fix every affected model ever produced, but it likely does not
>> introduce any regressions. The patch introduces a quirk that is enabled
>> when ASUS7000 is found.
>>
>> Scope (_SB)
>> {
>> Device (ATK)
>> {
>> Name (_HID, "ASUS7000") // _HID: Hardware ID
>> }
>> }
>
> Hmm, tricky! But about time we did something about the DSTS vs DCTS guessing.
>
> While we don't have definitive knowledge of the right thing to do
> here, I think I have enough info available to solidify some
> assumptions we'd otherwise be afraid to make, and then we can
> implement a better approach here.
>
> In our database of 144 Asus DSDTs, 14 of them respond to DCTS:
>
> AS_D520MT
> D425MC
> D640SA
> D320SF-K
> D415MT
> D830MT
> G11DF
> M32CD4-K
> V221ID
> V272UN_SKU3
> Z240IE
> ZN220IC-K
> ZN241IC
> ZN270IE
>
> Of those 14, they all additionally respond to DSTS, except for D415MT
> and AS_D520MT.
>
> In all 14 cases, the DCTS handling is done inside a device with _UID
> ASUSWMI. None of the other 130 products have a device with that _UID.
>
> Furthermore, we recently received access to the ASUS spec, which
> confirms that the Instance Name for EeePC is "ACPI\PNP0C14\ASUSWMI_0"
> whereas the Instance Name for other notebooks is "ACPI\PNP0C14\ATK_0".
>
> The 12 devices that respond to both DCTS and DSTS, do it on separate
> different devices, albeit with the same _WDG UUID. The one with _UID
> ASUSWMI responds to DCTS, and the one with _UID ATK responds to DSTS.
>
> So that seems fairly convincing. My thinking is that we can check the
> _UID of the underlying device, and use DCTS for ASUSWMI, DSTS
> otherwise. To do that, I think you'd have to rework the driver probing
> so that it happens through wmi_driver_register(). Then from the probe
> function onwards you will get a wmi_device, and hopefully you can get
> the _UID through that instance somehow.
>
> There's a separate issue of what happens on those 12 machines where
> there are two WMI devs, with the same _WDG GUID.
> drivers/platform/x86/wmi.c drops duplicates, so one of them is being
> ignored in that case. We'd ideally find a way to ignore the ASUSWMI
> node and go with ATK in that situation. But I think this can be
> separated from your work here.
>
> Thanks for these patches and welcome to the world of kernel development!
>
> Daniel
>
Re: [PATCH 04/11] platform/x86: asus-wmi: Add quirk to force DSTS WMI method detection
On Thu, Apr 11, 2019 at 4:28 AM Yurii Pavlovskyi
wrote:
> The DSTS method detection fails, as nothing is returned if method is not
> defined in WMNB. As a result the control of keyboard backlight is not
> functional for TUF Gaming series laptops (at the time the only
> functionality of the driver on this model implemented with WMI methods).
>
> Patch was tested on a newer TUF Gaming FX505GM and older K54C model.
>
> FX505GM:
> Method (WMNB, 3, Serialized)
> { ...
> If ((Local0 == 0x53545344))
> {
> ...
> Return (Zero)
> }
> ...
> // No return
> }
>
> K54C:
> Method (WMNB, 3, Serialized)
> { ...
> If ((Local0 == 0x53545344))
> {
> ...
> Return (0x02)
> }
> ...
> Return (0xFFFE)
> }
>
> The non-existing method ASUS_WMI_METHODID_DSTS=0x53544344 (actually it is
> DCTS in little endian ASCII) is selected in asus->dsts.
>
> One way to fix this would be to call both for every known device ID until
> some answers - this would increase module load time.
>
> Another option is to check some device that is known to exist on every
> model - none known at the time.
>
> Last option, which is implemented, is to check for presence of the
> ASUS7000 device in ACPI tree (it is a dummy device), which is the
> condition used for loading the vendor driver for this model. This might
> not fix every affected model ever produced, but it likely does not
> introduce any regressions. The patch introduces a quirk that is enabled
> when ASUS7000 is found.
>
> Scope (_SB)
> {
> Device (ATK)
> {
> Name (_HID, "ASUS7000") // _HID: Hardware ID
> }
> }
Hmm, tricky! But about time we did something about the DSTS vs DCTS guessing.
While we don't have definitive knowledge of the right thing to do
here, I think I have enough info available to solidify some
assumptions we'd otherwise be afraid to make, and then we can
implement a better approach here.
In our database of 144 Asus DSDTs, 14 of them respond to DCTS:
AS_D520MT
D425MC
D640SA
D320SF-K
D415MT
D830MT
G11DF
M32CD4-K
V221ID
V272UN_SKU3
Z240IE
ZN220IC-K
ZN241IC
ZN270IE
Of those 14, they all additionally respond to DSTS, except for D415MT
and AS_D520MT.
In all 14 cases, the DCTS handling is done inside a device with _UID
ASUSWMI. None of the other 130 products have a device with that _UID.
Furthermore, we recently received access to the ASUS spec, which
confirms that the Instance Name for EeePC is "ACPI\PNP0C14\ASUSWMI_0"
whereas the Instance Name for other notebooks is "ACPI\PNP0C14\ATK_0".
The 12 devices that respond to both DCTS and DSTS, do it on separate
different devices, albeit with the same _WDG UUID. The one with _UID
ASUSWMI responds to DCTS, and the one with _UID ATK responds to DSTS.
So that seems fairly convincing. My thinking is that we can check the
_UID of the underlying device, and use DCTS for ASUSWMI, DSTS
otherwise. To do that, I think you'd have to rework the driver probing
so that it happens through wmi_driver_register(). Then from the probe
function onwards you will get a wmi_device, and hopefully you can get
the _UID through that instance somehow.
There's a separate issue of what happens on those 12 machines where
there are two WMI devs, with the same _WDG GUID.
drivers/platform/x86/wmi.c drops duplicates, so one of them is being
ignored in that case. We'd ideally find a way to ignore the ASUSWMI
node and go with ATK in that situation. But I think this can be
separated from your work here.
Thanks for these patches and welcome to the world of kernel development!
Daniel
[PATCH 04/11] platform/x86: asus-wmi: Add quirk to force DSTS WMI method detection
The DSTS method detection fails, as nothing is returned if method is not
defined in WMNB. As a result the control of keyboard backlight is not
functional for TUF Gaming series laptops (at the time the only
functionality of the driver on this model implemented with WMI methods).
Patch was tested on a newer TUF Gaming FX505GM and older K54C model.
FX505GM:
Method (WMNB, 3, Serialized)
{ ...
If ((Local0 == 0x53545344))
{
...
Return (Zero)
}
...
// No return
}
K54C:
Method (WMNB, 3, Serialized)
{ ...
If ((Local0 == 0x53545344))
{
...
Return (0x02)
}
...
Return (0xFFFE)
}
The non-existing method ASUS_WMI_METHODID_DSTS=0x53544344 (actually it is
DCTS in little endian ASCII) is selected in asus->dsts.
One way to fix this would be to call both for every known device ID until
some answers - this would increase module load time.
Another option is to check some device that is known to exist on every
model - none known at the time.
Last option, which is implemented, is to check for presence of the
ASUS7000 device in ACPI tree (it is a dummy device), which is the
condition used for loading the vendor driver for this model. This might
not fix every affected model ever produced, but it likely does not
introduce any regressions. The patch introduces a quirk that is enabled
when ASUS7000 is found.
Scope (_SB)
{
Device (ATK)
{
Name (_HID, "ASUS7000") // _HID: Hardware ID
}
}
Signed-off-by: Yurii Pavlovskyi
---
drivers/platform/x86/asus-nb-wmi.c | 5 +
drivers/platform/x86/asus-wmi.c| 16 +---
drivers/platform/x86/asus-wmi.h| 5 +
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/asus-nb-wmi.c
b/drivers/platform/x86/asus-nb-wmi.c
index b6f2ff95c3ed..cc5f0765a8d9 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include "asus-wmi.h"
@@ -434,6 +435,10 @@ static void asus_nb_wmi_quirks(struct asus_wmi_driver
*driver)
}
pr_info("Using i8042 filter function for receiving events\n");
}
+
+ if (acpi_dev_found("ASUS7000")) {
+ driver->quirks->force_dsts = true;
+ }
}
static const struct key_entry asus_nb_wmi_keymap[] = {
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index cfccfc0b8c2f..58890d87d50c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -24,7 +24,7 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define PR KBUILD_MODNAME ": "
#include
#include
@@ -1885,11 +1885,21 @@ static int asus_wmi_platform_init(struct asus_wmi *asus)
* Note, on most Eeepc, there is no way to check if a method exist
* or note, while on notebooks, they returns 0xFFFE on failure,
* but once again, SPEC may probably be used for that kind of things.
+*
+* Additionally at least TUF Gaming series laptops return 0 for unknown
+* methods, so the detection in this way is not possible and method must
+* be forced. Likely the presence of ACPI device ASUS7000 indicates
+* this.
*/
- if (!asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS, 0, 0, NULL))
+ if (asus->driver->quirks->force_dsts) {
+ pr_info(PR "DSTS method forced\n");
+ asus->dsts_id = ASUS_WMI_METHODID_DSTS2;
+ } else if (!asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS,
+ 0, 0, NULL)) {
asus->dsts_id = ASUS_WMI_METHODID_DSTS;
- else
+ } else {
asus->dsts_id = ASUS_WMI_METHODID_DSTS2;
+ }
/* CWAP allow to define the behavior of the Fn+F2 key,
* this method doesn't seems to be present on Eee PCs */
diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
index 6c1311f4b04d..94056da02fde 100644
--- a/drivers/platform/x86/asus-wmi.h
+++ b/drivers/platform/x86/asus-wmi.h
@@ -54,6 +54,11 @@ struct quirk_entry {
*/
int no_display_toggle;
u32 xusb2pr;
+ /**
+* Force DSTS instead of DSCS and skip detection. Useful if WMNB
+* returns nothing on unknown method call.
+*/
+ bool force_dsts;
bool (*i8042_filter)(unsigned char data, unsigned char str,
struct serio *serio);
--
2.17.1
