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