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).
So unless I am missing something if both drivers are loaded then there will be 2 hwmon devices registered with duplicate functionality?
If I have understood that correctly then the WMI interface really should be hidden (the WMI drivers' probe() can simply return -ENODEV without doing anything) when the ACPI interface is there.
We do NOT want 2 hwmon devices with duplicate functionality.
Regards,
Hans
