Message-ID: <w3sxt2kt64g2b6gjucnsmnydfm4r74pzf2sl6fcjmjwvk3jyd5@2miohw3pslwk>
Date: Fri, 6 Dec 2024 22:59:35 -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 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.

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.

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

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

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

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