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.
Thanks,
Armin Wolf
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.
AFAIK i think this should normally be done at the BIOS level, but if
only a limited number of devices are affected then this should be fine.
Thanks,
Armin Wolf
ack
Regards,
Hans
