Re: [PATCH net-next 09/12] mlxsw: core: Extend hwmon interface with fan fault attribute

[Guenter Roeck <linux@roeck-us.net>]

On 2/13/19 11:06 PM, Vadim Pasternak wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Wednesday, February 13, 2019 5:03 PM
>> To: Andrew Lunn <andrew@lunn.ch>; Ido Schimmel <idosch@mellanox.com>
>> Cc: netdev@vger.kernel.org; davem@davemloft.net; Jiri Pirko
>> <jiri@mellanox.com>; mlxsw <mlxsw@mellanox.com>; Vadim Pasternak
>> <vadimp@mellanox.com>
>> Subject: Re: [PATCH net-next 09/12] mlxsw: core: Extend hwmon interface with
>> fan fault attribute
>>
>> On 2/13/19 5:53 AM, Andrew Lunn wrote:
>>> On Wed, Feb 13, 2019 at 11:28:53AM +0000, Ido Schimmel wrote:
>>>> From: Vadim Pasternak <vadimp@mellanox.com>
>>>>
>>>> Add new fan hwmon attribute for exposing fan faults (fault indication
>>>> is read from Fan Out of Range Event Register).
>>>>
>>>> Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
>>>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>>>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>>>
>>> Hi Ido
>>>
>>> You should include the HWMON maintainer in the Cc: list.
>>>
>>> I would not be too surprised if he says to use
>>> hwmon_device_register_with_info().
>>>
>>
>> I would ask to do that for new drivers, but this is is not a new driver.
>> On top of that, I wasn't included in its initial review. Since I wasn't involved, I
>> have no idea what shape the driver is in, and for sure won't review it now (to
>> retain my sanity).
>>
>> Only comment I have is that using the _with_info API and using devm_ functions
>> might simplify the driver a lot. I'll be happy to do a review if/when that is done.
>>
>> Guenter
> 
> Hi Guenter,
> 
> Thank you for your comments.
> 
> But, this patch adds one new FAN attribute to the existing infrastructure.
> At the time mlxsw core_hwmon module has been submitted, the API
> hwmon_device_register_with_info() was not available yet.
> Of course in a new modules we are using this API, like in
> our mlxreg-fan for example.
> 
> The same is relevant for the patch 10/12 from this series – it also extends
> the existing infrastructure.
> But there, even in case of a new code the config structure for
> hwmon_device_register_with_info()  would be look like:
> {
> 	/* 3 entries like below - for chip ambient temperatures (could be from 1 to 3. */
> 	HWMON_F_INPUT | HWMON_T_HIGHEST | HWMON_T_RESET_HISTORY,
> 	...
> 	/* 128 entries like below - for modules temperatures (could be from 16 to 128. */
> 	HWMON_F_INPUT | HWMON_T_CRIT | HWMON_T_EMERGENCY, HWMON_T_FAULT | HWMON_T_LABEL,
> 	...
> 	/* 64 entries like below - for interconnects temperatures (could be from 0 to 64). */
> 	HWMON_F_INPUT | HWMON_T_HIGHEST | HWMON_T_RESET_HISTORY | HWMON_T_LABEL,
> 	0
> };
> 
I would probably have created the above dynamically, just like the current code does,
except it now generates all the attributes. AFAICS the current code doesn't leave holes
in attribute numbering, so allocating everything statically doesn't really make much
sense. Note that you would need separate arrays, one per sensor type (temperature, fan,
pwm).

> Means we'll have 195 lines for configuration description and in case future systems will
> be equipped with the bigger number of port slots and/or with the bigger number of
> interconnects devices, the below structure will require modification.
> Modification will be not necessary, in case we'll keep configuration like it is now.
> 
> Regarding devm_ API - it has been used initially in core_hwmon module, but the in
> commit (9b3bc7db759e) it has been removed. And it was a good reason for that.
> Chip can be re-programmed after initialization in case driver determines it needs to
> flash a new firmware version. In such case after re-programming driver will make
> registration again and among the other stuff it will make new registration with
> hwmon subsystem. And in case it 's registered with
> hwmon subsystem using devm_hwmon_device_register_with_groups(), the old
> hwmon device (registered before the flashing) was never unregistered and was
> referencing stale data, resulting in a use-after free.
> 
Well, devm_ obviously doesn't work if the object lifetime doesn't match device
lifetime.

Guenter