| Message ID | d8e0f65c-8c4e-7b60-93bd-3bf90e2da344@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | asus-wmi: Support of ASUS TUF Gaming series laptops | expand |
On Thu, Apr 11, 2019 at 4:28 AM Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com> 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 (0xFFFFFFFE) > } > > 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
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 > <yurii.pavlovskyi@gmail.com> 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 (0xFFFFFFFE) >> } >> >> 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 >
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 <linux/fb.h> #include <linux/dmi.h> #include <linux/i8042.h> +#include <linux/acpi.h> #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 <linux/kernel.h> #include <linux/module.h> @@ -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 0xFFFFFFFE 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);
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 (0xFFFFFFFE) } 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 <yurii.pavlovskyi@gmail.com> --- 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(-)