Re: [RFC PATCH 00/21] alienware-wmi driver rework

[Armin Wolf]

Am 05.12.24 um 01:27 schrieb Kurt Borja:

> Hi :)
>
> This series are a follow-up to this discussion [1], in which I proposed
> migrating the alienware-wmi driver to use:
>
> 1. State container driver model
> 2. Modern WMI driver design
> 3. Drop use of deprecated WMI methods
>
> Of course, this was much harder than expected to do cleanly. Main
> problem was that this driver "drives" two completely different devices
> (I'm not referring to the WMI devices, which also happen to be two).
>
> Throughout these series we will call these devices AlienFX and AWCC.
>
> As a preamble
> =============
>
> AlienFX exposes a LED, hdmi, amplifier and deepsleep interface to
> userspace through a platform device named "alienware-wmi". Historically
> this driver handled this by leveraging on two WMI devices as a backend.
> This devices named LEGACY and WMAX were very similar, the only
> difference was that WMAX had more features, but share all features
> LEGACY had. Although it's a stretch, it could be argued this WMI devices
> are the "same", just different GUID.
>
> Later Dell repurposed the WMAX WMI device to serve as a thermal control
> interface for all newer "gaming" laptops. This new WMAX device has an
> ACPI UID = "AWCC" (I discovered this recently). So it could also be
> argued that old WMAX and AWCC WMAX are not the same device, just same
> GUID.
>
> This drivers manages all these features using deprecated WMI methods.

I think there is a misunderstanding here.

The WMAX WMI device is identical with the AWCC WMI device, only the UID might be different.
The reason why the thermal control WMI methods are not available on older WMAX devices is
that Dell seemed to have introduced this WMI methods after the usual WMAX WMI methods.

Because of this i advise against splitting WMAX (LED, attributes, ...) and AWCC functionality
into separate files.

> Approach I took for the rework
> ==============================
>
> Parts 1-7 sort of containerize all AlienFX functionality under the
> "alienware-wmi" platform driver so WMI drivers can prepare and register
> a matching platform device from the probe.
>
> Parts 8-12 create and register two WMI drivers for the LEGACY and WMAX
> devices respectively. The code for these probes is VERY similar and
> all "differences" are passed to the platform device via platform
> specific data (platdata). Also AlienFX functionality is refactored to
> use non-deprecated WMI methods.
>
> Parts 13-17 migrate all AWCC methods to use non-deprecated WMI methods
> and the state container driver model.
>
> Parts 18-21 I splitted the alienware-wmi.c module into the different
> features this driver manages.
>
> alienware-wmi-base.c is in charge of initializing WMI drivers and
> define some platform specific data, like operations (Part 10 for more
> info). alienware-wmi-alienfx.c has all AlienFX functionality and
> alienware-wmi-awcc.c has all AWCC functionality.

I would rather split the drivers into:

- alienware-wmi-legacy, which handles the LEGACY WMI device and registers a alienware-wmi platform device

- alienware-wmi-wmax, which handles the modern WMAX WMI device and also registers a alienware-wmi platform device

- alienware-wmi-base, which provides a driver for the alienware-wmi platform device

This of course only works if the LEGACY WMI device and the WMAX WMI device are newer both present at the same time,
in this case alienware-wmi-legacy could use wmi_has_guid() as a band aid check to avoid probing if a WMAX WMI device
is present.

Using the platform_data mechanism to decouple the alienware-wmi device driver from the underlying hardware implementation
should be fine IMHO.

> Coments
> =======
>
> This is still kind of a draft, but I did some testing and it works!
>
> Of course I will do thorough testing and cleanup when I send the
> non-RFC version. I just want to get some comments on the general
> approach before proceeding further.
>
> I think this is quite messy in it's current state so I apollogize.
>
> @Mario Limonciello: I included the reviews you gave me on [2]. I
> included some of those patches here, and dropped the ones that did not
> make sense with this design. As this is another series let me know if
> you want me to drop the tags!
>
> @Armin Wolf: I don't like the amount of files I made. As the maintainer
> of the wmi module, what do you think about making two independent
> modules, one for AlienFX and one for AWCC. In order to not register two
> drivers for the WMAX device the module init would check if the "AWCC"
> UID is present.

I know of at least one device which support both AWCC thermal control and
WMAX LED control, so splitting the WMAX device driver like this could cause
problems.

Like i said before, you should view the WMAX WMI device as having different
capabilities (= WMI methods) depending of the machine the kernel is running on.

If a capability is available (currently determined via quirks), the driver should
do the necessary things to handle it.

As a side note: i am currently exploring if we can decode the WMI BMOF buffers inside
the kernel, so that in the far future we can remove those quirks and automatically detect
which methods are available. But this will take a long time, so it has nothing to do with
this patch series.

I will take a look at the other patches tomorrow.

Thanks,
Armin Wolf

> The approach for that would be basically the same, and I think the
> series would change very little.
>
> I would like this a lot because I still think old and new WMAX devices
> are different, but I couldn't find another example of where an OEM
> repurposed a WMI device.
>
> @Everyone: I know this is VERY long. Thank you so much for your time in
> advance!
>
> This series were made on top of the 'for-next' branch:
>
> Commit c712e8fd9bf4 ("MAINTAINERS: Change AMD PMC driver status to "Supported"")
>
> ~ Kurt
>
> [1] https://lore.kernel.org/platform-driver-x86/6m66cuivkzhcsvpjv4nunjyddqhr42bmjdhptu4bqm6rm7fvxf@qjwove4hg6gb/T/#u
> [2] https://lore.kernel.org/platform-driver-x86/20241120163834.6446-3-kuurtb@xxxxxxxxx/
>
> Kurt Borja (21):
>    alienware-wmi: Modify parse_rgb() signature
>    alienware-wmi: Move Lighting Control State
>    alienware-wmi: Remove unnecessary check at module exit
>    alienware-wmi: Improve sysfs groups creation
>    alienware-wmi: Refactor rgb-zones sysfs group creation
>    alienware-wmi: Add state container and alienfx_probe()
>    alienware-wmi: Migrate to state container pattern
>    alienware-wmi: Add WMI Drivers
>    alienware-wmi: Initialize WMI drivers
>    alienware-wmi: Add alienfx OPs to platdata
>    alienware-wmi: Refactor LED control methods
>    alienware-wmi: Refactor hdmi, amplifier, deepslp
>    alienware-wmi: Add a state container for AWCC
>    alienware-wmi: Migrate thermal methods to wmidev
>    alienware-wmi: Refactor sysfs visibility methods
>    alienware-wmi: Make running control state part of platdata
>    alienware-wmi: Drop thermal methods dependency on quirks
>    platform-x86: Add header file for alienware-wmi
>    platform-x86: Rename alienare-wmi
>    platform-x86: Split the alienware-wmi module
>    platform-x86: Add config entries to alienware-wmi
>
>   MAINTAINERS                                   |    3 +-
>   drivers/platform/x86/dell/Kconfig             |   25 +-
>   drivers/platform/x86/dell/Makefile            |    5 +-
>   .../platform/x86/dell/alienware-wmi-alienfx.c |  531 +++++++
>   .../platform/x86/dell/alienware-wmi-awcc.c    |  282 ++++
>   .../platform/x86/dell/alienware-wmi-base.c    |  525 +++++++
>   drivers/platform/x86/dell/alienware-wmi.c     | 1267 -----------------
>   drivers/platform/x86/dell/alienware-wmi.h     |  141 ++
>   8 files changed, 1505 insertions(+), 1274 deletions(-)
>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-alienfx.c
>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-awcc.c
>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
>   delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
>   create mode 100644 drivers/platform/x86/dell/alienware-wmi.h