From: James Seo <james@equiv.tech>
To: Armin Wolf <W_Armin@gmx.de>
Cc: jdelvare@suse.com, linux@roeck-us.net,
linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC 0/1] hwmon: (hp-wmi-sensors) Support autoloading
Date: Tue, 19 Mar 2024 17:40:36 -0700 [thread overview]
Message-ID: <ZfowhGaCWffQ2N1K@equiv.tech> (raw)
In-Reply-To: <a2c2ef97-3830-4277-8560-b97cfb8eb78e@gmx.de>
On Tue, Mar 19, 2024 at 02:00:06PM +0100, Armin Wolf wrote:
> Am 19.03.24 um 06:47 schrieb James Seo:
>
>> On Mon, Mar 18, 2024 at 10:57:31PM +0100, Armin Wolf wrote:
>>> Currently, the hp-wmi-sensors driver needs to be loaded manually
>>> on supported machines. This however is unnecessary since the WMI
>>> id table can be used to support autoloading.
>>>
>>> However the driver might conflict with the hp-wmi driver since both
>>> seem to use the same WMI GUID for registering notify handler.
>>>
>>> I am thus submitting this patch as an RFC for now.
>>>
>>> Armin Wolf (1):
>>> hwmon: (hp-wmi-sensors) Add missing MODULE_DEVICE_TABLE()
>>>
>>> drivers/hwmon/hp-wmi-sensors.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> --
>>> 2.39.2
>>>
>> Autoloading was deliberately left out for now because of the GUID
>> conflict with hp-wmi's WMI notify handler.
>>
>> HP's GUID reuse across product lines for different types of WMI
>> objects with different names and shapes means that with a patch like
>> this, many systems that should only load hp-wmi-sensors but not
>> hp-wmi will try to autoload both. (Perhaps all of them; I want to say
>> that the GUID 5FB7F034-2C63-45e9-BE91-3D44E2C707E4, which is the
>> second of the two GUIDs that hp-wmi uses to autoload, exists on every
>> HP system I've examined.)
>>
>> Meanwhile, hp-wmi does various other platform things, and there's so
>> much hardware out there that who knows, maybe there are some systems
>> that really should load both. I don't think so but I can't rule it
>> out.
>>
>> Unlike hp-wmi-sensors, hp-wmi doesn't survive failure to install its
>> notify handler, which sets up a potential race condition depending on
>> when hp-wmi and hp-wmi-sensors loads on a given system.
>>
>> Therefore, I intended to add autoloading at the same time as
>> converting hp-wmi-sensors to use the bus-based WMI interface once
>> aggregate WMI devices are better supported.
>>
>> As you mentioned [1], I ran into issues when I tried to do the
>> conversion by simply adding the GUID to struct wmi_driver.id_table.
>> That resulted in two separate independent instances of hp_wmi_sensors
>> being loaded, which isn't what I wanted.
>
> After taking a look at a ACPI table dump of a HP machine, i noticed that
> HPBIOS_BIOSEvent has the GUID 2B814318-4BE8-4707-9D84-A190A859B5D0, which is
> different than the event GUID used by hp-wmi.
>
> According your comment in hp_wmi_notify(), i assume that some machines have
> mixed-up event GUIDs.
I investigated further. Every HP machine in the Linux Hardware Database that
has \\.\root\WMI\hpqBEvnt at 95F24279-4D7B-4334-9387-ACCDC67EF61C also has
\\.\root\WMI\HPBIOS_BIOSEvent at 2B814318-4BE8-4707-9D84-A190A859B5D0.
> I thing it would be best to create a separate WMI driver for the event and
> use a notifier chain (see include/linux/notifier.h) to distribute the event data.
>
> In case of event GUID 95F24279-4D7B-4334-9387-ACCDC67EF61C, both hp-wmi and
> hp-wmi-sensors can subscribe on this notifier and receive event data without
> stepping on each other's toes.
>
> The same can be done for the event GUID 2B814318-4BE8-4707-9D84-A190A859B5D0,
> with a separate notifier chain.
>
> This would decouple the event handling from the event data consumers, allowing
> both hp-wmi and hp-wmi-sensors to coexist.
No objections from me for this specific use case to work around the GUID conflict.
hp-wmi-sensors should indeed subscribe on 2B814318-4BE8-4707-9D84-A190A859B5D0
for some of those machines.
Any ideas for getting rid of wmi_query_block() for fetching
\\.\root\HP\InstrumentedBIOS\HPBIOS_PlatformEvents? I know other drivers are
also using it for getting blocks other than their "main" GUID.
> I can provide a prototype implementation, but unfortunately i have no HP machine
> myself for testing. But i might be able to find one to test my changes.
Happy to test. (Also happy to try it myself, but I'd need some help.)
> Thanks,
> Armin Wolf
>
>>
>> [1] https://lore.kernel.org/linux-hwmon/cd81a7d6-4b81-f074-1f28-6d1b5300b937@gmx.de/
>>
next prev parent reply other threads:[~2024-03-20 0:40 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-18 21:57 [RFC 0/1] hwmon: (hp-wmi-sensors) Support autoloading Armin Wolf
2024-03-18 21:57 ` [RFC PATCH 1/1] hwmon: (hp-wmi-sensors) Add missing MODULE_DEVICE_TABLE() Armin Wolf
2024-03-18 23:04 ` [RFC 0/1] hwmon: (hp-wmi-sensors) Support autoloading Guenter Roeck
2024-03-19 5:47 ` James Seo
2024-03-19 13:00 ` Armin Wolf
2024-03-20 0:40 ` James Seo [this message]
2024-03-20 15:13 ` Armin Wolf
2024-03-20 20:11 ` James Seo
2024-03-20 22:09 ` Armin Wolf
2024-03-21 6:33 ` James Seo
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=ZfowhGaCWffQ2N1K@equiv.tech \
--to=james@equiv.tech \
--cc=W_Armin@gmx.de \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).