On 2/13/19 11:06 PM, Vadim Pasternak wrote:
-----Original Message-----
From: Guenter Roeck <groe...@gmail.com> On Behalf Of Guenter Roeck
Sent: Wednesday, February 13, 2019 5:03 PM
To: Andrew Lunn <and...@lunn.ch>; Ido Schimmel <ido...@mellanox.com>
Cc: netdev@vger.kernel.org; da...@davemloft.net; Jiri Pirko
<j...@mellanox.com>; mlxsw <ml...@mellanox.com>; Vadim Pasternak
<vad...@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 <vad...@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 <vad...@mellanox.com>
Reviewed-by: Jiri Pirko <j...@mellanox.com>
Signed-off-by: Ido Schimmel <ido...@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
