Re: [patch net-next RFC 11/12] mlxsw: core: Extend hwmon interface with FAN fault attribute

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

On Tue, Jun 26, 2018 at 02:47:01PM +0000, Vadim Pasternak wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: Tuesday, June 26, 2018 5:29 PM
> > To: Vadim Pasternak <vadimp@mellanox.com>
> > Cc: davem@davemloft.net; netdev@vger.kernel.org; linux@roeck-us.net;
> > rui.zhang@intel.com; edubezval@gmail.com; jiri@resnulli.us; mlxsw
> > <mlxsw@mellanox.com>; Michael Shych <michaelsh@mellanox.com>
> > Subject: Re: [patch net-next RFC 11/12] mlxsw: core: Extend hwmon interface
> > with FAN fault attribute
> > 
> > > +static ssize_t mlxsw_hwmon_fan_fault_show(struct device *dev,
> > > +					  struct device_attribute *attr,
> > > +					  char *buf)
> > > +{
> > > +	struct mlxsw_hwmon_attr *mlwsw_hwmon_attr =
> > > +			container_of(attr, struct mlxsw_hwmon_attr,
> > dev_attr);
> > > +	struct mlxsw_hwmon *mlxsw_hwmon = mlwsw_hwmon_attr->hwmon;
> > > +	char mfsm_pl[MLXSW_REG_MFSM_LEN];
> > > +	u16 tach;
> > > +	int err;
> > > +
> > > +	mlxsw_reg_mfsm_pack(mfsm_pl, mlwsw_hwmon_attr->type_index);
> > > +	err = mlxsw_reg_query(mlxsw_hwmon->core, MLXSW_REG(mfsm),
> > mfsm_pl);
> > > +	if (err) {
> > > +		dev_err(mlxsw_hwmon->bus_info->dev, "Failed to query
> > fan\n");
> > > +		return err;
> > > +	}
> > > +	tach = mlxsw_reg_mfsm_rpm_get(mfsm_pl);
> > > +
> > > +	return sprintf(buf, "%u\n", (tach < mlxsw_hwmon->tach_min) ? 1 : 0);
> > > +}
> > 
> > Documentation/hwmon/sysfs-interface says:
> > 
> > Alarms are direct indications read from the chips. The drivers do NOT make
> > comparisons of readings to thresholds. This allows violations between readings
> > to be caught and alarmed. The exact definition of an alarm (for example,
> > whether a threshold must be met or must be exceeded to cause an alarm) is
> > chip-dependent.
> > 
> > Now, this is a fault, not an alarm. But does the same apply?
> 
Yes, it does. There are no "soft" alarms / faults.

> Hi Andrew,
> 
> Hardware provides minimum value for tachometer.
> Tachometer is considered as faulty in case it's below this
> value.

This is for user space to decide, not for the kernel.

> In case any tachometer is faulty, PWM according to the
> system requirements should be set to 100% until the fault

system requirements. Again, this is for user space to decide.

> is not recovered (f.e. by physical replacing of bad unit).
> This is the motivation to expose fan{x}_fault in the way
> it's exposed.
> 
> Thanks,
> Vadim.
> 
> > 
> >      Andrew