linux-kernel - Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1496903262.23335.1.camel@aj.id.au>
Date:   Thu, 08 Jun 2017 15:57:42 +0930
From:   Andrew Jeffery <andrew@...id.au>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Joel Stanley <joel@....id.au>,
        Matthew Barth <msbarth@...ux.vnet.ibm.com>,
        linux-hwmon@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Timothy Pearson <tpearson@...torengineering.com>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>
Subject: Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan
 controller

On Wed, 2017-06-07 at 10:37 -0700, Guenter Roeck wrote:
> On Wed, Jun 07, 2017 at 04:15:06PM +0930, Andrew Jeffery wrote:
> > On Wed, 2017-06-07 at 12:18 +0930, Joel Stanley wrote:
> > > On Wed, Jun 7, 2017 at 1:50 AM, Matthew Barth
> > > > > > > > <msbarth@...ux.vnet.ibm.com> wrote:
> > > > 
> > > > On 06/06/17 8:33 AM, Guenter Roeck wrote:
> > > > > 
> > > > > On 06/06/2017 12:02 AM, Andrew Jeffery wrote:
> > > > > > Over and above the features of the original patch is support for a
> > > > > > secondary
> > > > > > rotor measurement value that is provided by MAX31785 chips with a revised
> > > > > > firmware. The feature(s) of the firmware are determined at probe time and
> > > > > > extra
> > > > > > attributes exposed accordingly. Specifically, the MFR_REVISION 0x3040 of
> > > > > > the
> > > > > > firmware supports 'slow' and 'fast' rotor reads. The feature is
> > > > > > implemented by
> > > > > > command 0x90 (READ_FAN_SPEED_1) providing a 4-byte response (with the
> > > > > > 'fast'
> > > > > > measurement in the second word) rather than the 2-bytes response in the
> > > > > > original firmware (MFR_REVISION 0x3030).
> > > > > > 
> > > > > 
> > > > > Taking the pmbus driver question out, why would this warrant another
> > > > > non-standard
> > > > > attribute outside the ABI ? I could see the desire to replace the 'slow'
> > > > > access
> > > > > with the 'fast' one, but provide two attributes ? No, I don't see the
> > > > > point, sorry,
> > > > > even more so without detailed explanation why the second attribute in
> > > > > addition
> > > > > to the first one would add any value.
> > > > 
> > > > In the case of counter-rotating(CR) fans which contain two rotors to provide
> > > > more airflow there are then two tach feedbacks. These CR fans take a single
> > > > target speed and provide individual feedbacks for each rotor contained
> > > > within the fan enclosure. Providing these individual feedbacks assists in
> > > > fan fault driven speed changes, improved thermal characterization among
> > > > other things.
> > > > 
> > > > Maxim provided this as a 'slow' / 'fast' set of bytes to be user
> > > > compatible(so the 'slow' rotor speed, regardless of which rotor, is in the
> > > > first 2 bytes with the 'slow' version of firmware as well). In some cases,
> > > > mfg systems could have a mix of these revisions.
> > > 
> > > Andrew, instead of creating the _fast sysfs nodes, I think you could
> > > expose the extra rotors are separate fan devices in sysfs. This would
> > > not introduce new ABI.
> > 
> > I considered this approach: I debated whether to consider the fan unit
> > as a single entity with two inputs, or just separate fans, and ended up
> > leaning towards the former. To go the latter path we need to consider
> > whether or not to expose the writeable properties for the second input
> > (given they also affect the first) and how to sensibly arrange the
> > pairs given the functionality is determined at probe-time. Not rocket
> > science but decisions we need to make.
> > 
> 
> There are many other examples with one writeable and multiple readable
> attributes. Temperature offset attributes are an excellent example.
> Next question would be what exactly would be writable. pwm attributes are
> commonly completely independent of fan attributes. pwm1 output doesn't
> mean that fan1 is the matching input; in fact, most of the time it isn't.
> The only question would be numbering (is the pair numbered fan1/2 or
> fan1/fan(1+X) ?) which is just a matter of personal preference. However,
> everything is better than coming up with non-standard attributes which
> can not be used with any standard application beyond the application of the
> person submitting the driver. It is bad enough if a non-standard attribute
> describes something really driver specific. But a non-standard attribute
> for a fan speed reading ? Please no. 

Yes, I've received loud and clear that I made the wrong choice :)
Apologies.

Thanks again for your feedback.

Andrew


Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)