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.
>>>>
>>>> What I think works best in cases like this is to have the wmi-driver
>>>> expose a function to retrieve the fan max value.
>>>>
>>>> This function can use a static global array of fan max values +
>>>> a global fan_max_values_initialized bool and it can return -EPROBE_DEFER
>>>> when the bool is not set yet.
>>>>
>>>> This will also require the ACPI driver to have a Kconfig "depends on"
>>>> the WMI driver but that should be fine.
>>>>
>>>> And then the ACPI driver can simply call the exported helper function
>>>> to get the max fan values.
>>>>
>>>> This sort of cross driver function calling is not ideal, but it is
>>>> better then poking at a struct device owned by another driver.
>>> Problem is that when we really bring over the ACPI interface to other
>>> ODMs the WMI driver will no longer be there, that's why I wanted to
>>> avoid a static dependency on the WMI module to be able to reuse
>>> the code.
>> I see. But won't we still need the WMI module for older models then ?
> But it wouldn't need to be loaded, with the dependency it will always need to be loaded.
Presumably it will be auto-loaded based on the WMI GUID, so even if
it has special code in probe() to exit the probe() with -ENODEV on
systems where it is not necessary due to the ACPI interface being
there, it will still get loaded.
To avoid auto-loading the WMI interface would need to be removed
from the ACPI tablets.
And probe() could still store the max fan speeds in a global +
set the boolean to indicate the max fan speeds are set before
exiting with -ENODEV in that case.
So I think you could still go this route, but simply hardcoding
for the 2 models this is necessary for seems more KISS, so IMHO
that is probably the way to go for now.
Regards,
Hans
