Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <e256d281-49a5-2d9a-7def-fd68e177e926@roeck-us.net>
Date:   Fri, 7 Apr 2023 05:54:48 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Content-Language: en-US
To:     James Seo <james@equiv.tech>, Armin Wolf <W_Armin@gmx.de>
Cc:     Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230406152321.42010-1-james@equiv.tech>
 <2257deba-187b-82d2-181c-f1fed08a2ff7@gmx.de> <ZC+sgnuy5bssD1DN@vb-22lts>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v2] hwmon: add HP WMI Sensors driver
In-Reply-To: <ZC+sgnuy5bssD1DN@vb-22lts>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/6/23 22:39, James Seo wrote:
> Hi,
> 
>> is it guaranteed that faulty sensors wont become operational later?
>> Also filtering out such sensors would make the support for the hwmon_temp_fault and
>> hwmon_fan_fault attributes meaningless.
> 
> Good point. I can't be certain, but the MOF does seem to imply that
> sensors can indeed be faulty on just a temporary basis.
> 

Your current code would explicitly exclude faulty fans from being listed,
which does not exactly sound like a good idea.

> I'll filter out only the sensors that are "Not Connected" at probe
> time. My thinking is, even if these might turn into connected sensors
> later, that would mean the user is e.g. hot-plugging a fan (!), and
> keeping them could result in a large number (~10 on my Z420) of
> pointless extra channels. And this would also match the behavior of
> HP's official utility.
> 
Ultimately that is an implementation decision. Are the sensors hot-pluggable ?
If so, how does HP's utility handle the insertion or removal of a sensor (fan) ?

Either case, it is ok with me if disconnected sensors are not listed.
Not listing faulty sensors seems like a bad idea, though.

Guenter

> Does that seem reasonable? Or did you mean that I shouldn't filter,
> and leave disconnected sensors in like some other hwmon drivers do?
> 
>> The sanity check for HP_WMI_NUMERIC_SENSOR_GUID is unnecessary, the WMI driver core already makes sure that your driver
>> is only matched with WMI devices containing HP_WMI_NUMERIC_SENSOR_GUID.
>> As for the sanity check regarding HP_WMI_BIOS_GUID: this WMI GUID is not used inside the driver. Since WMI GUIDs are expected
>> to be unique, checking for HP_WMI_BIOS_GUID (which AFAIK is used by the HP-BIOSCFG driver) without intending to use it is
>> meaningless.
> 
> In that case, I'll gladly remove the checks. I was following the
> example of the platform/x86/hp-wmi driver, which checks for that GUID
> and another at module load.
> 
> Thanks for reviewing.
> 
> James