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
