Hi,
On 18-Nov-24 8:57 PM, Armin Wolf wrote:
> Am 18.11.24 um 20:32 schrieb Werner Sembach:
>
>>
>> Am 18.11.24 um 19:25 schrieb Armin Wolf:
>>> Am 18.11.24 um 14:11 schrieb Werner Sembach:
>>>
>>>>
>>>> Am 18.11.24 um 12:20 schrieb Hans de Goede:
>>>>> Hi,
>>>>>
>>>>> On 18-Nov-24 12:16 PM, Werner Sembach wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Am 18.11.24 um 12:09 schrieb Hans de Goede:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 13-Nov-24 11:04 PM, Armin Wolf wrote:
>>>>>>>> Am 13.11.24 um 13:12 schrieb Hans de Goede:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 13-Nov-24 12:47 PM, Werner Sembach wrote:
>>>>>>>>>> Am 13.11.24 um 12:19 schrieb Hans de Goede:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 13-Nov-24 12:11 PM, Werner Sembach wrote:
>>>>>>>>>>>> Am 13.11.24 um 12:05 schrieb Hans de Goede:
>>>>>>>>>>>>> Hi All,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 12-Nov-24 5:31 PM, Armin Wolf wrote:
>>>>>>>>>>>>>> Am 12.11.24 um 16:10 schrieb Werner Sembach:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Am 12.11.24 um 13:51 schrieb Armin Wolf:
>>>>>>>>>>>>>>>> Am 12.11.24 um 13:42 schrieb Werner Sembach:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Am 12.11.24 um 13:01 schrieb Armin Wolf:
>>>>>>>>>>>>>>>>>> Am 12.11.24 um 12:52 schrieb Werner Sembach:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> quick learning question: Why is wmi_bus_type not
>>>>>>>>>>>>>>>>>>> exported unlike, for
>>>>>>>>>>>>>>>>>>> example, acpi_bus_type, and platform_bus_type?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Wanted to use bus_find_device_by_name in an acpi driver
>>>>>>>>>>>>>>>>>>> that might
>>>>>>>>>>>>>>>>>>> need additional infos from a wmi interface that might
>>>>>>>>>>>>>>>>>>> or might not be
>>>>>>>>>>>>>>>>>>> present.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Kind regards,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Werner Sembach
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> What kind of information do you have in mind?
>>>>>>>>>>>>>>>>>> wmi_bus_type is not
>>>>>>>>>>>>>>>>>> being exported for historic reasons, i can change that
>>>>>>>>>>>>>>>>>> if necessary.
>>>>>>>>>>>>>>>>> It's for the tuxedo-drivers part for the Sirius 16 Gen 1
>>>>>>>>>>>>>>>>> & 2 which has
>>>>>>>>>>>>>>>>> a slow wmi and a quick acpi interface, however the quick
>>>>>>>>>>>>>>>>> acpi
>>>>>>>>>>>>>>>>> interface can not get the max rpm of the cooling fans,
>>>>>>>>>>>>>>>>> but the wmi
>>>>>>>>>>>>>>>>> interface can.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thing is for the acpi driver we might plan an earlier
>>>>>>>>>>>>>>>>> upstream date
>>>>>>>>>>>>>>>>> and it might get multi-odm support, while the wmi
>>>>>>>>>>>>>>>>> interface is and
>>>>>>>>>>>>>>>>> stays odm specific. So my idea was to only couple both
>>>>>>>>>>>>>>>>> drivers in a
>>>>>>>>>>>>>>>>> dynamic way using bus_find_device_by_name.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Interesting, how is the ACPI interface not ODM specific?
>>>>>>>>>>>>>>>> Can you
>>>>>>>>>>>>>>>> elaborate a bit on how the ACPI and the WMI interfaces
>>>>>>>>>>>>>>>> work?
>>>>>>>>>>>>>>> We have an ODM that was willing to include ACPI code by us
>>>>>>>>>>>>>>> in their
>>>>>>>>>>>>>>> BIOS blob and we hope that in the future we can carry that
>>>>>>>>>>>>>>> API over to
>>>>>>>>>>>>>>> other ODMs for future TUXEDO devices.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In pseudocode that API looks like this:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> v1:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> void SMOD(bool mode): Toggle firmware controlled fans vs
>>>>>>>>>>>>>>> manually (aka
>>>>>>>>>>>>>>> via the commands below) controlled fans
>>>>>>>>>>>>>>> bool GMOD(): Get current SMOD setting
>>>>>>>>>>>>>>> int GCNT(): Get number of fans
>>>>>>>>>>>>>>> enum GTYP(int index): Returns "CPU-fan" or "GPU-fan"
>>>>>>>>>>>>>>> void SSPD(int index, int value): Set fan speed target as a
>>>>>>>>>>>>>>> fraction of
>>>>>>>>>>>>>>> max speed
>>>>>>>>>>>>>>> int GSPD(int index): Get current fan speed target as a
>>>>>>>>>>>>>>> fraction of max
>>>>>>>>>>>>>>> speed
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> v2 same as v1 but with added:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> int GRPM(int index): Get current actual fan speed in
>>>>>>>>>>>>>>> revolutions per
>>>>>>>>>>>>>>> minute
>>>>>>>>>>>>>>> int GTMP(int index): Get temperature of thing fan with
>>>>>>>>>>>>>>> respective
>>>>>>>>>>>>>>> index is pointed at (CPU or GPU die, see GTYP)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Like I said, what is missing is a "Get Max RPM" function
>>>>>>>>>>>>>>> even in v2,
>>>>>>>>>>>>>>> which we might add a future iteration, but, well this bios
>>>>>>>>>>>>>>> is now out
>>>>>>>>>>>>>>> in the wild. However these released devices have a "get
>>>>>>>>>>>>>>> info" function
>>>>>>>>>>>>>>> in the wmi code which returns the v2 infos and the max rpm.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I want to write the code in a way that it probes the acpi
>>>>>>>>>>>>>>> interface
>>>>>>>>>>>>>>> for function existence and wherever something is missing
>>>>>>>>>>>>>>> tries to fall
>>>>>>>>>>>>>>> back to infos gathered from the wmi interface, but that
>>>>>>>>>>>>>>> one is
>>>>>>>>>>>>>>> implemented in a stand alone module (the tuxedo_nb04_*
>>>>>>>>>>>>>>> stuff in
>>>>>>>>>>>>>>> tuxedo-drivers) and I would like to keep it that way in
>>>>>>>>>>>>>>> honor of KISS.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> My plan is that the first time max rpm is pulled the acpi
>>>>>>>>>>>>>>> driver uses
>>>>>>>>>>>>>>> bus_find_device_* to get the wmi device, if present, and
>>>>>>>>>>>>>>> pulls max rpm
>>>>>>>>>>>>>>> from the driver data there and copies it over to it's own
>>>>>>>>>>>>>>> driver data.
>>>>>>>>>>>>>>> If not possible it returns a dummy value or falls back to
>>>>>>>>>>>>>>> another
>>>>>>>>>>>>>>> method. Maybe a hard coded list of max rpm values,
>>>>>>>>>>>>>>> currently only 2
>>>>>>>>>>>>>>> devices have the new interface, so it wouldn't be a long
>>>>>>>>>>>>>>> list.
>>>>>>>>>>>>>>> Directly going to the hard coded list is our current
>>>>>>>>>>>>>>> fallback plan,
>>>>>>>>>>>>>>> but it is not an elegant solution as the info is actually
>>>>>>>>>>>>>>> there, if
>>>>>>>>>>>>>>> you know what i mean?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Kind regards,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Werner
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I see, we once had a similar case with the dell-wmi driver,
>>>>>>>>>>>>>> see commit f97e058cfe80 ("platform/x86: wmi: Don't allow
>>>>>>>>>>>>>> drivers to get each other's GUIDs"):
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The only driver using this was dell-wmi, and it
>>>>>>>>>>>>>> really was a hack.
>>>>>>>>>>>>>> The driver was getting a data attribute from
>>>>>>>>>>>>>> another driver and this
>>>>>>>>>>>>>> type of action should not be encouraged.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Rather drivers that need to interact with one
>>>>>>>>>>>>>> another should pass
>>>>>>>>>>>>>> data back and forth via exported functions.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I would be quite unhappy with drivers interacting with WMI
>>>>>>>>>>>>>> devices without a proper WMI driver, but i can see your
>>>>>>>>>>>>>> point here.
>>>>>>>>>>>>> Agreed on that 1 driver should not be poking the [wmi_]dev of
>>>>>>>>>>>>> another
>>>>>>>>>>>>> driver. This usually works until it doesn't for some reason
>>>>>>>>>>>>> so it
>>>>>>>>>>>>> should just be avoided.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Maybe we can keep the retrieval of the fanX_max values out
>>>>>>>>>>>>>> of the kernel? I propose the following:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - have a driver for your generic ACPI interface
>>>>>>>>>>>>>> - have a driver for the WMI interface (with fanX_max hwmon
>>>>>>>>>>>>>> attributes)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The driver for the generic ACPI interface exposes the fan
>>>>>>>>>>>>>> speed controls as pwmX attributes if the interface does not
>>>>>>>>>>>>>> support
>>>>>>>>>>>>>> the "Get Max RPM" function. The userspace application in
>>>>>>>>>>>>>> this case searches for the hwmon chip exposed by the WMI
>>>>>>>>>>>>>> driver and
>>>>>>>>>>>>>> reads the fanX_max attributes there. Then the application
>>>>>>>>>>>>>> can convert the target fan speed into values for the pwmX
>>>>>>>>>>>>>> attributes
>>>>>>>>>>>>>> itself.
>>>>>>>>>>>>>> If the ACPI interface however supports the "Get Max RPM"
>>>>>>>>>>>>>> function, then it exposes fanX_max and fanX_target hwmon
>>>>>>>>>>>>>> attributes
>>>>>>>>>>>>>> themself and the userspace application uses them directly.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This would keep the kernel drivers simple.
>>>>>>>>>>>>> That would indeed keep the kernel drivers simple, but at the
>>>>>>>>>>>>> cost of
>>>>>>>>>>>>> providing a non standard hwmon interface.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Whatever implementation is written it really MUST follow the
>>>>>>>>>>>>> standard
>>>>>>>>>>>>> hwmon API so that any hwmon tools like the lm_sensors
>>>>>>>>>>>>> fancontrol script
>>>>>>>>>>>>> will work properly.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So NACK from me for exposing fanX_max on a separate hwmon
>>>>>>>>>>>>> device.
>>>>>>>> I think there is a misunderstanding here: the hwmon device
>>>>>>>> exported by the WMI interface
>>>>>>>> will also contain temperature and fan speed sensors (provided by
>>>>>>>> the WMI interface), so
>>>>>>>> this would be a standard hwmon device.
>>>>>>>>
>>>>>>>> The only difference between this hwmon device and the hwmon device
>>>>>>>> exposed by the ACPI interface
>>>>>>>> would be that:
>>>>>>>>
>>>>>>>> - the hwmon interface exposed by the ACPI interface also allows
>>>>>>>> for manual fan control
>>>>>>>> - the hwmon interface exposed by the ACPI interface does not
>>>>>>>> support the fanx_max attribute
>>>>>>>>
>>>>>>>> So i think there should be no problem with having two hwmon
>>>>>>>> devices on some models, since both are
>>>>>>>> usable independently from each other.
>>>>>>> Ah I see I thought the suggestion was to have the WMI hwmon
>>>>>>> interface only export
>>>>>>> the fanx_max attribute attribute and then require userspace to
>>>>>>> treat the 2 hwmon
>>>>>>> interfaces as one.
>>>>>>>
>>>>>>> If you duplicate all the functionality of the ACPI hwmon device on
>>>>>>> the WMI hwmon device,
>>>>>>> then the question becomes why have the ACPI hwmon interface there
>>>>>>> at all ? If it is less
>>>>>>> functional and duplicate IMHO we should just leave it out ?
>>>>>> Without knowing the details because a collegue write the wmi code:
>>>>>> The WMI fucntions are signigifanctly less performant then the acpi
>>>>>> ones where it has measurable performance impact to the whole system
>>>>>> when you continuously poll them.
>>>>>>
>>>>>> Luckily the only wmi exclusive information (max rpm) doesn't change
>>>>>> and only needs to be polled once during initialization).
>>>>> Right, but both offer the same information / functionality right
>>>>> (except for the missing max rpm).
>>>>>
>>> The ACPI interface allows for manual fan control, something the WMI
>>> interface does not support.
>> oh you got me wrong here: the wmi interface also supports fan control
>> (albeit I'm not sure if it is implemented in tuxedo-drivers)
>
> I see, in this case hiding the WMI-based hwmon device would indeed be beneficial to avoid problems should different userspace applications
> use different hwmon devices for fan control.
>
> Ideally this should happen inside the ACPI BIOS, but we can implement a blacklist if only a limited number of device are affected.
I don't think we need a deny-list, instead the WMI driver can just
check for presence of the new ACPI interface and if that is there
exit its probe() with -ENODEV.
This avoids the need to maintain a list.
Regards,
Hans
