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 ?
I thought the whole idea was to prefer the ACPI interface over the WMI one and only
expose a hwmon device backed by the ACPI interface if both are present ?
Regards,
Hans
