From: "Thomas Weißschuh" <linux@weissschuh.net>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Jean Delvare" <jdelvare@suse.com>,
"Guenter Roeck" <linux@roeck-us.net>,
platform-driver-x86@vger.kernel.org,
"Mark Gross" <mgross@linux.intel.com>,
linux-kernel@vger.kernel.org,
"Barnabás Pőcze" <pobrn@protonmail.com>,
"Matthew Garrett" <mjg59@srcf.ucam.org>
Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver
Date: Thu, 8 Apr 2021 16:54:51 +0200 [thread overview]
Message-ID: <37a67493-c489-477c-8133-16d5dbd494f2@t-8ch.de> (raw)
In-Reply-To: <6993d257-fdc1-2be6-555d-86c6b8c9d18d@redhat.com>
Hi Hans,
On Do, 2021-04-08T11:36+0200, Hans de Goede wrote:
> On 4/7/21 9:43 PM, Thomas Weißschuh wrote:
> > 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?
>
> I think that failing the probe with -ENODEV, with a dev_info() explaining why when
> all six sensors return 0 would be good yes, that should also fix those 2
> issues on https://github.com/t-8ch/linux-gigabyte-wmi-driver/.
I added such a validation step.
> >> 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.
>
> Since you alreayd have bugreports of boards where this does not work,
> please don't spend too much time on this. I guess those older DSDT-s will
> also just return an integer with value 0.
Ok.
> >> 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).
>
> Ok.
>
> >> Perhaps the WMI interface returns an error when you query an out-of-range
> >> temperature channel?
> >
> > Also "0" as mentioned above.
>
> Hmm, so maybe this can be used to limit the amount of reported temperature
> sensors, IOW if sensors 5 and 6 report 0, only register 4 sensors ?
So far the 0-returning sensors have not been at the end of the list but in the
middle. Is it worth building logic to properly probe a bitmask of useful
sensors?
> >> 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 .
>
> Good, you should probably ask reporters there to provide the output of:
>
> grep . /sys/class/dmi/id/* 2> /dev/null
I added a DMI-based whitelist and asked users to submit their DMI information.
> Ran as a normal user (so that the serial-numbers will be skipped) so that
> you will have DMI strings to match on if you decide to go that route.
The serials seem not to be too critical on these boards:
/sys/class/dmi/id/board_serial:Default string
/sys/class/dmi/id/chassis_serial:Default string
/sys/class/dmi/id/product_serial:Default string
> > 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.
>
> Actually the "Directly access the it87 registers" option is potentially interesting
> since it will allow using the it87 driver which gives a lot more features.
>
> I actually wrote a rough outline of how something like this could work here:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47
I must have overread this one, but yes that's also what I had in mind.
> Note I'm not sure if that is the right approach, but it definitely is an
> option. It seems that this one might also solve the X470-AORUS-ULTRA-GAMING
> case (https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)
>
> Hopefully the direct-register ACPI/WMI access methods will also allow
> reading the super-io vendor and product ids so that we can be reasonably
> sure that we are not loading the wrong driver on a board.
>
> OTOH the WMI-temp method approach may also work on boards where the sensors
> (or some of the sensors) are not supported.
>
> I'm afraid there is no obviously correct answer here. If you feel like it
> experimenting with the "Directly access the it87 registers" option would certainly
> be interesting IMHO.
>
> It might be good to get hwmon subsystems maintainer's opinion on this
> before sinking a lot of time into this though (added to the Cc).
Sounds good.
next prev parent reply other threads:[~2021-04-08 14:54 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-05 13:20 [PATCH] platform/x86: add Gigabyte WMI temperature driver Thomas Weißschuh
2021-04-05 17:13 ` Barnabás Pőcze
2021-04-05 20:48 ` [PATCH v2] " Thomas Weißschuh
2021-04-07 15:54 ` Hans de Goede
2021-04-07 19:43 ` Thomas Weißschuh
2021-04-08 9:36 ` Hans de Goede
2021-04-08 14:54 ` Thomas Weißschuh [this message]
2021-04-08 15:00 ` Guenter Roeck
2021-04-09 6:02 ` Thomas Weißschuh
2021-04-10 6:56 ` Guenter Roeck
2021-04-10 14:21 ` Hans de Goede
2021-04-10 14:40 ` [PATCH v3] " Thomas Weißschuh
2021-04-10 14:57 ` Hans de Goede
2021-04-10 15:21 ` Guenter Roeck
2021-04-10 15:15 ` Guenter Roeck
2021-04-10 15:23 ` Hans de Goede
2021-04-10 18:18 ` [PATCH v4] " Thomas Weißschuh
2021-04-11 0:58 ` Guenter Roeck
2021-04-11 14:05 ` Barnabás Pőcze
2021-04-12 12:35 ` [PATCH v5] " Thomas Weißschuh
2021-04-13 8:14 ` Hans de Goede
2021-04-07 18:27 ` [PATCH v2] " Barnabás Pőcze
2021-04-08 14:39 ` Thomas Weißschuh
2021-04-08 15:08 ` Guenter Roeck
2021-04-08 16:07 ` Hans de Goede
2021-04-10 6:40 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=37a67493-c489-477c-8133-16d5dbd494f2@t-8ch.de \
--to=linux@weissschuh.net \
--cc=hdegoede@redhat.com \
--cc=jdelvare@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mgross@linux.intel.com \
--cc=mjg59@srcf.ucam.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pobrn@protonmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).