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

[Armin Wolf]

Am 07.12.24 um 02:59 schrieb Kurt Borja:

> On Sat, Dec 07, 2024 at 12:26:20AM +0100, Armin Wolf wrote:
> > 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.
> By examining the ACPI tables of the devices that support the AWCC
> functionality, I realized none of the newer devices support the LED
> interface.

You are right, i misread the decoded bmof buffer xd.

> At the time I added quirks for those devices I assigned `num_zones = 2`,
> because I mimicked the default behavior of the driver, which was
> assigning quirk_unknown to devices not listed on the DMI table.
>
> This is of course my bad, but fortunately in all these cases the WMAX
> device returned an error code safely.
>
> I can send a fix for this, but it would require a bit of refactoring of
> the init function, which I think would cause merge conflicts if we end
> up reworking this driver. Also we don't know "FOR SURE" which devices
> don't support the LED interface, although I'm pretty sure it comes down
> to the UID of the device, but it's just a guess in the end.

If you do not know for sure which devices _you_ added support the LED interface, then
i would prefer to remove the "num_zones = 2" quirk from those devices for now.

> Thoughts on sending a fix? I believe leaving zones is pretty harmless in
> the end.

Please send a fix for your quirk entries, so we can avoid forgetting this little detail.

> I would love to have advice from Dell on this too. Hopefully they'll
> get back at us at some point. Any time now...
>
> > > 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
> If you don't change your mind with the information given above, I'm ok
> with this. That's why I splitted the driver at the end of the series :p

I did not change my mind.

I can understand that most devices either support the original WMAX WMI methods or the AWCC WMI methods,
but from a technical point of view it is still the same device. Also Dell could combine both sets of WMI methods
in a future device, and i would prefer being prepared for that.

We can still split alienware-wmi-wmax into multiple files (which get linked together) later should the source code
of it get too big in the future.

Also having a separate alienware-wmi-legacy would allow users to disable this driver when building the kernel.

> > 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.
> This is good to know!
>
> > > 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.
> Yes, it's really unfortunate Dell didn't make a new device for the
> thermal methods.

I agree, sadly this god object architecture is very common with some hardware manufactures :(

> > 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.
> This would be an awesome feature! Will you implement Pali Rohar's decoder?
> I'll be sure to make the necessary improvements once is done.

I will base my work on Pali Rohars decoder, but sadly the source code is quite convoluted, so i will
need to do some reverse-engineering.

The decompression alogrithm is already finished:

https://github.com/Wer-Wolf/libdeds

Thanks,
Armin Wolf

> > I will take a look at the other patches tomorrow.
> Thank you very much!
>
> ~ Kurt
>
> > 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