Upstreaming

[Wer-Wolf]

Waiting for feedback from @tuxedo-wse before upstreaming both drivers.

Currently not all features from the tuxedo-drivers package are implemented, but those features which are implemented seem to work. Did i miss something obvious?

[Wer-Wolf]

Note: the up-to-date prototype can be found on the testing branch.

[tuxedo-wse]

Had an initial look at it:

• Not to miss anything: This currently is only key events and fan control (via hwmon)?
• For reference: What model is your device?
• If i see that correctly, you are somewhat keeping the semantical split from tuxedo-drivers with uniwill-wmi.c ~= uniwill_wmi.c, uniwill-wmi.h ~= uniwill_interfaces.h, and uniwill-laptop.c ~= uniwill_keyboard.h
• https://github.com/Wer-Wolf/uniwill-laptop/blob/af1413a75c200eed429284f72ab4a2c6039129c8/uniwill-laptop.c#L759 but currently it is used for autoloading? I would suggest a whitelist approach here like I did for the upstreaming of the NB04 keyboard backlight driver. For uniwill you have the choice of doing it via DMI strings or by reading a specific ec register (see tuxedo-drivers for reference), but which value is mapping to which model is sadly only obtainable by reversing.
• Does the hwmon approach enforce a minimum fan speed at a certain temprature level?

@tuxedoxt please also have a look at this

[Wer-Wolf]

Had an initial look at it:

* Not to miss anything: This currently is only key events and fan control (via hwmon)?

Yes, and universal fan control is currently not implemented.

* For reference: What model is your device?

I tested it on a Intel NUC x15 notebook.

* If i see that correctly, you are somewhat keeping the semantical split from tuxedo-drivers with uniwill-wmi.c ~= uniwill_wmi.c, uniwill-wmi.h ~= uniwill_interfaces.h, and uniwill-laptop.c ~= uniwill_keyboard.h

Not really, the uniwill-wmi and uniwill-laptop drivers are not directly related to those files. The uniwill-wmi driver solely handles WMI events while the uniwill-laptop driver handles everything associated with the EC.

* https://github.com/Wer-Wolf/uniwill-laptop/blob/af1413a75c200eed429284f72ab4a2c6039129c8/uniwill-laptop.c#L759
   but currently it is used for autoloading? I would suggest a whitelist approach here like I did for the upstreaming of the NB04 keyboard backlight driver. For uniwill you have the choice of doing it via DMI strings or by reading a specific ec register (see tuxedo-drivers for reference), but which value is mapping to which model is sadly only obtainable by reversing.

Since i omitted the MODULE_DEVICE_TABLE macro, those WMI GUIDs are not used for autoloading by udev. The user has to load both modules manually. Sadly i cannot call any WMI method to identify the device since Uniwill reused the GUID from the Windows driver samples for their WMI interface, so this WMI GUID is not unique.

* Does the hwmon approach enforce a minimum fan speed at a certain temprature level?

This would be a policy decision and should thus be enforced by userspace. The driver exposes a standard hwmon interface for sensor access and fan control, everything else needs to be done by userspace programs.

@tuxedoxt please also have a look at this

[Wer-Wolf]

Regarding the minimum fan speed: AFAIK this would only be possible by using the universal fan control interface.

[Wer-Wolf]

Any updates on this?

[tuxedo-wse]

Sorry for the delay, both christoffer and I are busy atm.

Since I omitted the MODULE_DEVICE_TABLE macro, those WMI GUIDs are not used for autoloading by udev. The user has to load both modules manually.

Ok, this is good so we have no external deadline adopting tuxedo-drivers.

Sadly i cannot call any WMI method to identify the device since Uniwill reused the GUID from the Windows driver samples for their WMI interface, so this WMI GUID is not unique.

Yeah, you are right you need to first know if the wmi mathod for the device id is there. So this basically only leaved the option of DMI string checking. Can this be done by userspace udev rules?

Regarding the minimum fan speed: AFAIK this would only be possible by using the universal fan control interface.

I was not aware of this until now, do you have a link? If this interface allows driver enforced min speeds a cetain temperatures I would highly prefer it.

[tuxedo-wse]

In fact, the lack of min fanspeed is why we didn't already adopt hwmon in tuxedo-drivers. Christoffer looked into it and found no obvious way of doing it in a clean way. That's why I asked above.

[Wer-Wolf]

Then maybe i misunderstood the universal fan control interface, sorry.

However enforcing a minimum fan speed is not necessary for hwmon drivers, it is the responsibility of userspace to update the fan speed based on the temperature. The userspace can still just use the automatic mode setting.

[Wer-Wolf]

Sorry for the delay, both christoffer and I are busy atm.

Since I omitted the MODULE_DEVICE_TABLE macro, those WMI GUIDs are not used for autoloading by udev. The user has to load both modules manually.

Ok, this is good so we have no external deadline adopting tuxedo-drivers.

Sadly i cannot call any WMI method to identify the device since Uniwill reused the GUID from the Windows driver samples for their WMI interface, so this WMI GUID is not unique.

Yeah, you are right you need to first know if the wmi mathod for the device id is there. So this basically only leaved the option of DMI string checking. Can this be done by userspace udev rules?

I think so, just create a udev rule which calls modprobe.

Regarding the minimum fan speed: AFAIK this would only be possible by using the universal fan control interface.

I was not aware of this until now, do you have a link? If this interface allows driver enforced min speeds a cetain temperatures I would highly prefer it.

[tuxedo-wse]

Then maybe i misunderstood the universal fan control interface, sorry.

Sorry i'm confused now,you brought up the universal fan control interface. I figured it is a kernel uapi like hwmon or something?

However enforcing a minimum fan speed is not necessary for hwmon drivers, it is the responsibility of userspace to update the fan speed based on the temperature. The userspace can still just use the automatic mode setting.

The problem is: If it is possible to turn of the fan even with the cpu at 100°C via a config file, we know that there will be users out there doing it and then complain that their notebook died after 2 years.

[Wer-Wolf]

Then maybe i misunderstood the universal fan control interface, sorry.

Sorry i'm confused now,you brought up the universal fan control interface. I figured it is a kernel uapi like hwmon or something?

I meant the hardware interface.

However enforcing a minimum fan speed is not necessary for hwmon drivers, it is the responsibility of userspace to update the fan speed based on the temperature. The userspace can still just use the automatic mode setting.

The problem is: If it is possible to turn of the fan even with the cpu at 100°C via a config file, we know that there will be users out there doing it and then complain that their notebook died after 2 years. I see.

It would be difficult to provide such guarantees through the kernel driver, as the kernel driver can potentially be patched. So either you:

• enforce this limit through your GUI application and assume that the average user does not want to deal with low-level fan control via sysfs
• create your own ACPI interface for managing the fan speed profile. In this case the firmware would enforce a safe limit

Since the OEM software under Windows seems to work just fine with directly using the WMI interface, i think enforcing this limit through your OEM software will be enough.

Other question: does the tuxedo driver enforce such a minimum fan speed?

[tuxedo-wse]

Other question: does the tuxedo driver enforce such a minimum fan speed?

Also only in the userspace part, but afaik there is no general linux fan speed tool supporting our tuxedo_io fan control uapi. I don't know how that is with hwmon, so the idea was, that when we move to a standar dinterface we implement the min fan speed on a lower level.

Yes, the driver can always be recompiled without the restriction, but the idea is that this is enough of a hassle that people doing it know a little bit about computers and that such a enforcement is probably there for a reason.

[tuxedo-wse]

I meant the hardware interface.

Sorry forgot that the stuff in tuxedo-drivers->uniwill is named universal_ec_fan_control

[Wer-Wolf]

Other question: does the tuxedo driver enforce such a minimum fan speed?

Also only in the userspace part, but afaik there is no general linux fan speed tool supporting our tuxedo_io fan control uapi. I don't know how that is with hwmon, so the idea was, that when we move to a standar dinterface we implement the min fan speed on a lower level.

Yes, the driver can always be recompiled without the restriction, but the idea is that this is enough of a hassle that people doing it know a little bit about computers and that such a enforcement is probably there for a reason.

I think people who use utilities like pwmconfig will know a bit about computers, as using those tools requires usage of the command line.

Also i believe most people will use your OEM software if its easier to use than pwmconfig, especially if it has a GUI.

[tuxedo-wse]

I think people who use utilities like pwmconfig will know a bit about computers, as using those tools requires usage of the command line.

I was more thinking about things like corectrl https://gitlab.com/corectrl/corectrl

Don't know if it has hwmon support, but I supose so. Can you test that for me? Don't have a device at hand with a hwmon fan atm and you have the intel nuc x15 with this driver ^^.

Can you fix the fan to 0% with it for all temperatures?

[tuxedo-wse]

So I talked with Christoffer and a quick sumup:

• Upstreaming this should be fine from his pov as it is not loaded automatically anyway.
• He reminded me that the fanboost mode, that we used to hack in our custom fan control, gets deactivated automatically after ~1 hour and needs to be set again (I think we implemented that in tccd that once ever 30 minutes it quickly deactivates and reactivates the fanboost mode)
• He emphasized that different uniwill devices have different EC registers. And well this driver is allowed to be loaded on all devices which can cause undefined behaviour. Maybe if not enforced by a whitelist approach can users somewhere be warned to not load this module on unsupported devices?

[Wer-Wolf]

I think people who use utilities like pwmconfig will know a bit about computers, as using those tools requires usage of the command line.

I was more thinking about things like corectrl https://gitlab.com/corectrl/corectrl

Don't know if it has hwmon support, but I supose so. Can you test that for me? Don't have a device at hand with a hwmon fan atm and you have the intel nuc x15 with this driver ^^.

Can you fix the fan to 0% with it for all temperatures?

I will ask the person who own the Intel Nuc x15 to test this.

[Wer-Wolf]

So I talked with Christoffer and a quick sumup:

* Upstreaming this should be fine from his pov as it is not loaded automatically anyway.

* He reminded me that the fanboost mode, that we used to hack in our custom fan control, gets deactivated automatically after ~1 hour and needs to be set again (I think we implemented that in tccd that once ever 30 minutes it quickly deactivates and reactivates the fanboost mode)

I did not know that, is it necessary to disable fan boost mode first or is it sufficient to write the enable bit inside the register again?

* He emphasized that different uniwill devices have different EC registers. And well this driver is allowed to be loaded on all devices which can cause undefined behaviour. Maybe if not enforced by a whitelist approach can users somewhere be warned to not load this module on unsupported devices?

I can write a admin guide which contains this warning.

[tuxedo-wse]

I did not know that, is it necessary to disable fan boost mode first or is it sufficient to write the enable bit inside the register again?

Looking at our code again, I think we just let the timeout happen because we refresh the value every secound anyway. But my guess is that you have to write 0 first before writing 1 to reset the timer in the EC.

Did you check if you device has the universal_ec_fan_control bit?

    ret = uniwill_read_ec_ram(0x078e, &data);
    if (ret < 0) {
        return ret;
    }
    return (data >> 6) & 1;

That method of directly controlling the fan is way less hacky.

I can write a admin guide which contains this warning.

That would be nice.

But is there any particular reason you oppose the whitelist approach?

[Wer-Wolf]

The device seems to support universal fan control, and the fan boost mode also seems to work for several hours.

My reason for opposing a whitelist approach is that such an increasingly long list would be hard to maintain.

[Wer-Wolf]

I will try to add support for universal fan control.

[tuxedo-wse]

I will try to add support for universal fan control.

All out of my head but maybe it helps you:

For that you can in theory set a fancurve in firmware: you have 15 or 16 steps (would need to check) and can define different temp values for stepping up and down (to have some overlap and no fluctuating fan) (don't know how much I documented that in the code).

However we only use the first 2 steps set to 0-120°C and 120°-255°C (don't know if we set an overlap out of my head). The first steps fan speed is controlled by tccd, the secound one is set to 100% and is mainly just there to prevent buggy behaviour (christoffer has more infos there, he debugged that).

So maybe for this driver use 4 zones: 0-80°C controlled by hwmon and can go down to 0%, 80-90 also controlled by the hwmon input but at least 30%, 90-120°C also controlled by the hwmon input but at least 40%, 120-255°C 100% to prevent the bug mentioned above (the cpu should never rech that temp anyway, just needs to be there).

Additional notes:

• Using this method the ec has an automatic slow ramp up and ramp down for the fan, so don't be confused when the fan needs some time to rech the target speed
• this ramp up can the skipped by writing the target speed also directly to 0x1804 and 0x1809
• Writing 0% speed on some devices causes the fan to spin with 30% for 2 or 3 minuted before finally shutting off, to avoid that you can write 0.5% (-> value 1 because it goes from 0-200) on most devices which also means that the fan does not spin (however check if you specific model actually does not try to spin up with such a low value, some TF devices still try causing the fan to "click" or twitch just ever so slightly and it can't be healthy for it in the long run)
• besides the wmi interface there is also a acpi write method, doing basically the same but more perfomant, however that one can't be used to write the 0x18xx registers

[tuxedo-wse]

For auto loading: what about having a whitelist of auto loaded devices (dmi strings checked in an init function) and in additon have a module parameter "force_skip_supported_check", so new devices can be added both directly in kernel or via a udev rule

[Wer-Wolf]

I am currently a bit preoccupied, but i will add a dmi list.