Re: [PATCH v3] hwmon: add HP WMI Sensors driver

[Armin Wolf]

Am 26.04.23 um 15:16 schrieb James Seo:

> On Mon, Apr 24, 2023 at 11:13:36PM +0200, Armin Wolf wrote:
> > Am 24.04.23 um 12:05 schrieb James Seo:
> >
> > > +	for (i = 0; i < HP_WMI_MAX_INSTANCES; i++, pevents++) {
> > Hi,
> >
> > the WMI driver core already knows how many instances of a given WMI object are available.
> > Unfortunately, this information is currently unavailable to drivers. Would it be convenient
> > for you to access this information? I could try to implement such a function if needed.
> >
> > > +	for (i = 0; i < HP_WMI_MAX_INSTANCES; i++, info++) {
> > Same as above.
> Hello,
>
> Having the WMI object instance count wouldn't make much difference to
> me for now. The driver has to iterate through all instances during
> init anyway. If I were forced to accommodate 50+ sensors, I'd rewrite
> some things and I think I'd want such a function then, but I picked
> the current arbitrary limit of 32 because even that seems unlikely.
>
> So, maybe don't worry about it unless you want to. Or am I missing
> something?
Hi,

i already have a experimental patch available which adds such a function.
If you could test this patch to see if it works, then i could submit it upstream
where other drivers could profit from being able to know the number of
WMI object instances.

Your driver could also profit from such a function, as it could optimize the amount
of memory allocated to store WMI object data. The current instance discovery algorithm
(using a for-loop and break on error) also has a potential issue: when a single WMI call
fails for some reason (ACPI error, ...), all following WMI instances are being ignored.

I will post a RFC patch implementing such a function and CC you and a couple of people
who might be interested. If you want to test this function, then you can use this patch.
If you find out that the function does not work for you, then you can just continue with
your current approach.

Armin Wolf

> > > +	err = wmi_install_notify_handler(HP_WMI_EVENT_GUID,
> > > +					 hp_wmi_notify, state);
> > As a side note: the GUID-based interface for accessing WMI devices is deprecated.
> > It has known problems handling WMI devices sharing GUIDs and/or notification IDs. However,
> > the modern bus-based WMI interface (currently) does not support such aggregate devices well,
> > so i think using wmi_install_notify_handler() is still the best thing you can currently do.
> Interesting. Of course I had no idea. Though, for some strange
> reason, it does look like some documentation to that effect has
> emerged on the topic since the last time I checked ;)
>
> > > +	if (err) {
> > > +		dev_info(dev, "Failed to subscribe to WMI event\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	err = devm_add_action(dev, hp_wmi_devm_notify_remove, NULL);
> > > +	if (err) {
> > > +		wmi_remove_notify_handler(HP_WMI_EVENT_GUID);
> > > +		return false;
> > > +	}
> > Maybe use devm_add_action_or_reset() here?
> Will do.
>
> Thanks for reviewing/writing.
>
> James