linux-kernel - Re: [v2 3/3] hwmon: Add Aspeed ast2600 TACH support

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: <F1166366-99CC-4A36-A0A2-4965C787E60B@aspeedtech.com>
Date:   Thu, 3 Nov 2022 03:52:59 +0000
From:   Billy Tsai <billy_tsai@...eedtech.com>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     "jdelvare@...e.com" <jdelvare@...e.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "joel@....id.au" <joel@....id.au>,
        "andrew@...id.au" <andrew@...id.au>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        BMC-SW <BMC-SW@...eedtech.com>,
        "garnermic@...a.com" <garnermic@...a.com>
Subject: Re: [v2 3/3] hwmon: Add Aspeed ast2600 TACH support

On 2022/11/3, 1:01 AM, "Guenter Roeck" <groeck7@...il.com on behalf of linux@...ck-us.net> wrote:

    On Wed, Nov 02, 2022 at 06:54:43AM +0000, Billy Tsai wrote:
    >   > Hi Guenter,
    >   > 
    >   > On 2022/11/1, 9:15 PM, "Guenter Roeck" <groeck7@...il.com on behalf of linux@...ck-us.net> wrote:
    >   > 
    >   >     On Tue, Nov 01, 2022 at 05:51:56PM +0800, Billy Tsai wrote:
    >   >     > > +
    >   >     > > +	/* Restart the Tach channel to guarantee the value is fresh */
    >   >     > > +	aspeed_tach_ch_enable(priv, fan_tach_ch, false);
    >   >     > > +	aspeed_tach_ch_enable(priv, fan_tach_ch, true);
    >   > 
    >   >     > Is that really needed ? Doesn't the controller measure values continuously ?
    >   > 
    >   > Yes, the controller will measure values continuously by hardware. I will remove it. 
    >   > If the user want to get the fresh value, it should be done by the application layer
    >   > (e.g. read two times).
    >   > 
    >   >     > > +
    >   >     > > +	if (ret) {
    >   >     > > +		/* return 0 if we didn't get an answer because of timeout*/
    >   >     > > +		if (ret == -ETIMEDOUT)
    >   >     > > +			return 0;
    >   >     > > +		else
    >   >     > > +			return ret;
    >   > 
    >   >     > else after return is unnecessary, and why would a timeout be be ignored ?
    >   > 
    >   > When the user sets the correct fan information (i.e., min_rpm, max_rpm), the read
    >   > poll timeout will only occur if the tach pin does not get any signal (i.e. rpm=0).
    >   > 

    >   In that case it would be appropriate to return -ETIMEDOUT to the caller.

    >   Anyway, that should really not happen. Sysfs attributes such as minimum/maximum fan
    >   speed, the number of fan pulses per revolution, or a divider value should only exist
    >   if the chip needs that information, for example to report a fan error/alarm if the
    >   measured speed is out of range or if the chip actually calculates RPM and provides
    >   the result to the driver. Those values should not be necessary (and should not be
    >   used) to calculate some timeout.

Can't I use a min/max RPM to let the driver know a reasonable timeout/polling period when
the driver is trying to get RPM?
Beacause that our tach controller have the falg to indicates the hardware detected the change
in the input signal. I need the proper timout to rule out slow RPMs.

Thanks

Best Regards,
Billy Tsai