Re: [RFC 0/1] hwmon: (hp-wmi-sensors) Support autoloading

[Armin Wolf]

Am 20.03.24 um 01:40 schrieb James Seo:

> 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.

Could it be that using 95F24279-4D7B-4334-9387-ACCDC67EF61C is a mistake?
Or do you know of a machine which indeed uses this GUID to deliver sensor events?
Because it not, then we can just avoid this GUID conflict entirely by using the
other GUID.

> > 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.

Good question, it seems that HPBIOS_PlatformEvents is optional, so using the component
framework will not work.

If those WMI data blocks are always associated with the same ACPI device used by the
sensors GUID, then maybe i could create some sort of API for checking if a given GUID
exists the ACPI device associated with a WMI device.

However i thing the event GUID issue is more important right now.

Thanks,
Armin Wolf

> > 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@xxxxxx/