Message-ID: <hw6bhq4jhb5unzhpojoifufotqsbwik5p2mkwg4rvpqsrianxw@oebpysg3yswp>
Date: Sat, 7 Dec 2024 00:47:50 -0300
From: Kurt Borja <kuurtb@...il.com>
To: Armin Wolf <W_Armin@....de>
Cc: platform-driver-x86@...r.kernel.org, Dell.Client.Kernel@...l.com, 
	hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com, linux-kernel@...r.kernel.org, 
	mario.limonciello@....com
Subject: Re: [RFC PATCH 00/21] alienware-wmi driver rework

On Sat, Dec 07, 2024 at 04:20:09AM +0100, Armin Wolf wrote:
> 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.

Sure! I'll fix this weekend.

> 
> > 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.

Makes sense, I'll split it that way in the next iteration.

> 
> > > 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

Wow this is MUCH more readable now, nice work! I remember pulling gdb
just to get a general idea of what was going on. The parsing code should
be able to be simplified, but it is indeed very convoluted.

> 
> 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@gmail.com/
> > > > 
> > > > 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
> > > > 

Powered by blists - more mailing lists