Message-ID: <123d021b-b86b-4356-b234-fb46fa260193@t-8ch.de>
Date:   Wed, 7 Apr 2021 21:43:06 +0200
From:   Thomas Weißschuh <linux@...ssschuh.net>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     platform-driver-x86@...r.kernel.org,
        Mark Gross <mgross@...ux.intel.com>,
        linux-kernel@...r.kernel.org,
        Barnabás Pőcze <pobrn@...tonmail.com>,
        Matthew Garrett <mjg59@...f.ucam.org>
Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi Hans,

On Mi, 2021-04-07T17:54+0200, Hans de Goede wrote:
> Thank you for your new driver and thank you for the quick respin
> addressing Barnabás' request to make it a WMI driver.
> 
> The code looks good, so merging this should be a no-brainer,
> yet I'm not sure if I should merge this driver as-is, let me
> explain.

thanks for the encouraging words.

> The problem is that I assume that this is based on reverse-engineering?

Yes, it is completely reverse-engineered.
Essentially I stumbled upon Matthews comment at
https://bugzilla.kernel.org/show_bug.cgi?id=204807#c37 and went from there.

> We have some mixes experiences with reverse-engineered WMI drivers,
> sometimes a feature is not supported yet the wmi_evaluate_method()
> call happily succeeds. One example of this causing trouble is:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1797d588af15174d4a4e7159dac8c800538e4f8c

There actually are reports of recent, similar mainboards with recent firmware and
similar sensor chips that do not support the temperature query.
(https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/3 and
https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)

Unfortunately for unknown WMI queries the firmware does not return any value.
This ends up as an ACPI integer with value 0 on the driver side.
(Which I could not yet find documentation for if that is expected)
In the current version of the driver EIO is returned for 0 values which
get translated to N/A by lm-sensors.

> At a minimum I think your driver should check in its
> probe function that
> 
> gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, ...)
> 
> actually succeeds on the machine the driver is running on chances
> are that Gigabyte has been using the DEADBEEF-2001-0000-00A0-C90629100000
> GUID for ages, where as the 0x125 value for GIGABYTE_WMI_TEMPERATURE_QUERY
> suggests that this is a pretty new API.

Would it be enough to probe all six sensors and check if all return 0?

> It would be good if you can see if you can find some DSDT-s for older
> gigabyte motherboards attached to various distro's bug-tracking systems
> or forum posts and see how those respond to an unknown gigabyte_wmi_commandtype.

Will do.

> Another open question to make sure this driver is suitable
> as a generic driver (and does not misbehave) is how to figure out
> how many temperature sensors there actually are.

So far I could not find out how to query this from the firmware.
The IT8688 chip can report the state of each sensor but that is not exposed by
the firmware.
But even the state information from the IT8688 is not accurate as is.
One of the sensors that is reported as being active (directly via it87) on my
machine always reports -55°C (yes, negative).

> Perhaps the WMI interface returns an error when you query an out-of-range
> temperature channel?

Also "0" as mentioned above.

> One option here might be to add a DMI matching table and only load on
> systems on that table for now. That table could then perhaps also provide
> labels for each of the temperature channels, which is something which
> would be nice to have regardless of my worries about how well this driver
> will work on motherboards on which it has not been tested.

I am collecting reports for working motherboards at
https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/1 .

> You could combine this DMI matching table with a "force" module option to
> continue with probing on boards which are not on the table to allow users
> to test and report their results to you.
>
> And hopefully after a while, when we're confident that the code works
> well on most gigabyte boards we can drop the DMI table, or at least
> only use it for the channel labels.

That sounds good.

> Please don't take this the wrong way; I think it is great that you are
> working on this. And the quick turnaround of the v2 of this drivers makes
> me pretty certain that we can figure something out and get this merged.

Thank you for the feedback!

> Have you tried contacting Gigabyte about this? I don't think the WMI
> interface is something which they need to keep secret for competitive
> reasons, so maybe they can help? Note if they want you to sign a NDA
> of sorts to view docs, then make sure that it contains some language
> about them allowing you to release an opensource driver for their
> hardware based on the "protected" information.

I have not contacted them yet, will do.

As mentioned in the initial patch submission there would be different ways to
access this information firmware:

* Directly call the underlying ACPI methods (these are present in all so far
  observed firmwares, even if not exposed via WMI).
* Directly access the ACPI IndexField representing the it87 chip.
* Directly access the it87 registers while holding the relevant locks via ACPI.

I assume all of those mechanisms have no place in a proper kernel driver but
would like to get your opinion on it.

Thomas

Powered by blists - more mailing lists