Re: [PATCH 0/1] platform/x86: Add Lenovo Legion WMI Drivers

[Derek John Clark]

All.

Sorry for the spam. My mobile client switched to HTML mode and I
didn't catch it yesterday.
Resending my last response plain text so it gets posted to LKML properly.

>> Adds support for the Lenovo Legion series of laptop hardware to use WMI
>> interfaces that control various power settings.

>Note that there already is a driver for Lenovo Legion laptops that I
>wanted to merge upstream.

>https://github.com/johnfanv2/LenovoLegionLinux

Hi John.

I'm a fan of your work and did consider working on getting your driver
upstream-able before moving forward with my own. After reviewing it I
found there were too many changes needed which would have ultimately
required a full rewrite. For this initial driver MR I only intended to
add the features and models that Lenovo specifically asked me to
implement. I'm treating it as a platform baseline and I plan on adding
additional features in later patch series'. I fully encourage
community involvement in expanding this driver as I won't be able to
do it all on my own.

>Compared to the proposed patch, it has the following
>advantages:
>1. already popular and tested by thousands of users
>    - many stars and discussions on github
>    - patched into multiple kernels of gaming-related distros
>    - packaged as dkms module for almost all relevant Linux
>      distributions including Debian by other developers

Those DKMS packages will need to blacklist these drivers anyway, so
they should remain functional for anyone who prefers the out of tree
driver until there is feature parity.

>2. supports many different Lenovo Legion models starting from 2020/2021

I would gladly take any assistance with hardware I don't have access
to. I have structured the driver such that adding the additional
interfaces and features should be simple. Since a lot of the
historical data is available I expect a fairly rapid development
timeline going forward.

>3. supports a lot of more functions, including fan control, which is the
>  most requested feature

I'm aware of the user's need for fan control, this is planned as the
next feature once this patch series is accepted. Other things I have
planned are hwmon sensors and lighting control. I don't think feature
parity in the initial driver is a reasonable goal as that's
essentially a moving target that would take many months to write and
test. As I'm sure you're aware, maintaining a kernel patch through
multiple kernel versions takes a lot of effort if the subsystem is
active enough.

>4. supports the many changes between different in the WMI/ACPI method
>5. actually shares some credtis with persons who revere engineered it :)

I appreciate those efforts, many talented folks have done some amazing
work and I don't want to take anything away from them. I just want to
note this driver isn't reverse engineered as Lenovo have provided me
with the full WMI spec to create it.

>6. support by GUI tool to configure it all

>On the other hand, my driver has the following disadvantages:
>1. The version of master on github is the most recent one and contains
>   a lot of debug output that has to be removed (often indicated by TODO)
>2. It is all in one large c file instead of organizing it neatly into
>   multiple files.
>3. It was modeled after the ideapad driver instead of the newer ASUS
>   driver.

>A few notes regarding the many changes of the WMI methods that I tried
>to deal with in my driver: note that in almost every new model a new
>WMI method is used to control the same functionality (e.g. fan control
> or powermode). Additionally, often the constants or the unit changes
>: e.g. percent or rpm for fan speed.


>> The driver has been tested by me on the Lenovo Legion Go.

>The driver on github has been tested by thousands of users.

>I suggest that we maybe combine the two drivers before merging them,
>since Derek seems to have more kernel patching knowledge and I seem
>to have more worked on all the Legion laptops.

Based on the sheer size of your driver, I think a change as large as
that will be too much for a reasonable review on the LKML. Your driver
is over 6K lines and that will expand a bit when doing all the
necessary things to break it into multiple files and switch from
depreciated methods, add additional safety checks, etc. In my opinion
it would be better to add each interface as its own patch series (e.g.
custom mode and lighting, dmi specific functions, etc.) as that will
be much more manageable for review and easier to catch bugs.

All that being said, I'm certainly open to collaboration on the future
of this driver. If you want to contact me separately, we can discuss
it.

On Sun, Dec 22, 2024 at 2:09 AM Derek John Clark
<derekjohn.clark@xxxxxxxxx> wrote:
>
> >> Adds support for the Lenovo Legion series of laptop hardware to use WMI
> >> interfaces that control various power settings.
>
> >Note that there already is a driver for Lenovo Legion laptops that I
> >wanted to merge upstream.
>
> >https://github.com/johnfanv2/LenovoLegionLinux
>
> Hi John.
>
> I'm a fan of your work and did consider working on getting your driver upstreamable before moving forward with my own. After reviewing it I found there were too many changes needed which would have ultimately required a full rewrite. For this initial driver MR I only intended to add the features and models that Lenovo specifically asked me to implement. I'm treating it as a platform baseline and I do plan on adding additional features in later patch series'. I fully encourage community involvement in expanding this driver as I won't be able to do it all on my own.
>
> >Compared to the proposed patch, it has the following
> >advantages:
> >1. already popular and tested by thousands of users
> >    - many stars and discussions on github
> >    - patched into multiple kernels of gaming-related distros
> >    - packaged as dkms module for almost all relevant Linux
> >      distributions including Debian by other developers
>
> Those DKMS packages will need to blacklist these drivers anyway, so they should remain functional for anyone who prefers the out of tree driver until there is feature parity.
>
> >2. supports many different Lenovo Legion models starting from 2020/2021
>
> I would gladly take any assistance with hardware I don't have access to. I have structured the driver such that adding the additional interfaces and features should be simple. Since a lot of the historical data is available I expect a fairly rapid development timeline going forward.
>
> >3. supports a lot of more functions, including fan control, which is the
> >  most requested feature
>
> I'm aware of the user need for fan control, this is planned as the next feature once this patch series is accepted. Other things I have planned are hwmon sensors and lighting control. I don't think feature parity in the initial driver is a reasonable goal as that's essentially a moving target that would take many months to write and test. As I'm sure you're aware, maintaining a kernel patch through multiple kernel versions takes a lot of effort if the subsystem is active enough.
>
> >4. supports the many changes between different in the WMI/ACPI method
> >5. actually shares some credtis with persons who revere engineered it :)
>
> I appreciate those efforts, many talented folks have done some amazing work and I don't want to take anything away from them. I just want to note this driver isn't reverse engineered as Lenovo have provided me with the full WMI spec to create it.
>
> >6. support by GUI tool to configure it all
>
> >On the other hand, my driver has the following disadvantages:
> >1. The version of master on github is the most recent one and contains
> >   a lot of debug output that has to be removed (often indicated by TODO)
> >2. It is all in one large c file instead of organizing it neatly into
> >   multiple files.
> >3. It was modeled after the ideapad driver instead of the newer ASUS
> >   driver.
>
> >A few notes regarding the many changes of the WMI methods that I tried
> >to deal with in my driver: note that in almost every new model a new
> >WMI method is used to control the same functionality (e.g. fan control
> > or powermode). Additionally, often the constants or the unit changes
> >: e.g. percent or rpm for fan speed.
>
>
> >> The driver has been tested by me on the Lenovo Legion Go.
>
> >The driver on github has been tested by thousands of users.
>
> >I suggest that we maybe combine the two drivers before merging them,
> >since Derek seems to have more kernel patching knowledge and I seem
> >to have more worked on all the Legion laptops.
>
> Based on the sheer size of your driver, I think a change as large as that will be too much for a reasonable review on the LKML. Your driver is over 6K lines and that will expand a bit when doing all the necessary things to break it into multiple files and switch from depreciated methods, add additional safety checks, etc. In my opinion it would be better to add each interface as it's own patch series (e.g. custom mode and lighting, dmi specific functions, etc.) as that will be much more manageable for review and easier to catch bugs.
>
> All that being said, I'm certainly open to collaboration on the future of this driver. if you want to contact me separately we can discuss it.
>
> On Sun, Dec 22, 2024, 00:42 John Martens <johnfanv2@xxxxxxxxx> wrote:
>>
>> > Adds support for the Lenovo Legion series of laptop hardware to use WMI
>> > interfaces that control various power settings.
>>
>> Note that there already is a driver for Lenovo Legion laptops that I
>> wanted to merge upstream.
>>
>> https://github.com/johnfanv2/LenovoLegionLinux
>>
>> Compared to the proposed patch, it has the following
>> advantages:
>> 1. already popular and tested by thousands of users
>>     - many stars and discussions on github
>>     - patched into multiple kernels of gaming-related distros
>>     - packaged as dkms module for almost all relevant Linux
>>       distributions including Debian by other developers
>> 2. supports many different Lenovo Legion models starting from 2020/2021
>> 3. supports a lot of more functions, including fan control, which is the
>>   most requested feature
>> 4. supports the many changes between different in the WMI/ACPI method
>> 5. actually shares some credtis with persons who revere engineered it :)
>> 6. support by GUI tool to configure it all
>>
>> On the other hand, my driver has the following disadvantages:
>> 1. The version of master on github is the most recent one and contains
>>    a lot of debug output that has to be removed (often indicated by TODO)
>> 2. It is all in one large c file instead of organizing it neatly into
>>    multiple files.
>> 3. It was modeled after the ideapad driver instead of the newer ASUS
>>    driver.
>>
>> A few notes regarding the many changes of the WMI methods that I tried
>> to deal with in my driver: note that in almost every new model a new
>> WMI method is used to control the same functionality (e.g. fan control
>>  or powermode). Additionally, often the constants or the unit changes
>> : e.g. percent or rpm for fan speed.
>>
>> > The driver has been tested by me on the Lenovo Legion Go.
>>
>> The driver on github has been tested by thousands of users.
>>
>> I suggest that we maybe combine the two drivers before merging them,
>> since Derek seems to have more kernel patching knowledge and I seem
>> to have more worked on all the Legion laptops.
>>