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