Subject: Re: [PATCH v0 03/12] mlxsw: core: Add core environment module for port temperature reading

From: Guenter Roeck <[email-masked]>
Date: Thu, 21 Jun 2018 15:06:44 -0700

On Thu, Jun 21, 2018 at 08:34:40PM +0200, Andrew Lunn wrote:
> > Hi Andrew,
> 
> Adding Guenter Roeck, the HWMON maintainer.
>  
> > The temperature of each individual module can be obtained
> > through ethtool.
> 
> You mean via --module-info?
> 
> FYI: I plan to add hwmon support to the kernel SFP code. So if you
> ever decide to swap to the kernel SFP code, not your own, the raw
> temperatures will be exported.
> 
As should be. Unless adjustments are made by the hardware (eg a thermal
diode temperature offset register), all adjustments should be made in
userspace.

> > The worst temperature is necessary for the system cooling
> > control decision.
> 
> I would expect the system cooling would understand that.
> 
> > Up to 64 SFP/QSFP modules could be connected to the system.
> > Some of them could cooper modules, which doesn't provide
> > temperature measurement.
> 
> SFP modules are hot-plugable. So i would also expect the hwmon devices
> to hotplug. If there is no sensor, then there is no hwmon device... If
> there is no hwmon device, it plays no part in the thermal control
> loop.
> 
One hardware monitoring device per SFP, and I would assume that the
hwmon device for an SFP is only instantiated if a thermal sensor
is present.

> > Some of them could be optical modules, providing untrusted
> > temperature measurement, which could impact thermal
> > control of the system.
> 
> Why would you not trust it? Are you saying some modules simply have
> broken temperature sensors? Do you have a whitelist/blacklist of
> modules?
> 
> > Also optical modules could be from the different vendors,  and
> > this is real situation, when, f.e. one module has the warning and
> > critical thresholds 75C and 85C, while another 70C and 80C.
> 
> But hwmon exports both the actual temperature and the alarm
> temperatures. I would expect the thermal control code to use all this
> information when making its decisions, not just the current
> temperature.
> 
The respective information would either be provided by hardware
and reported to userspace, or userspace needs to determine the limits
and write them into the hardware. Either case, that is only relevant
if the hardware has limit registers. Otherwise all limits can and
should be handled in the thermal subsystem.

> > So, nominal temperature is not the case here, we should know the
> > "worst" value for the thermal control decision.
> 
> What it sounds like to me is you are working around problems in the
> thermal control by fudging the raw temperatures. That is the wrong
> thing to do. hwmon should export the raw data, and you should fix the
> thermal control code to use it correctly.
> 
Agreed. From the context it sounds like there should be some kind of
temperature aggregator which would probably reside in the thermal
subsystem (definitely not in hwmon).

I have not seen any hwmon specific patches. For new drivers,
please use [devm_]hwmon_device_register_with_info().

Guenter