linux-kernel - Re: [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache

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] [day] [month] [year] [list]

Date:	Sun, 26 Jun 2016 07:17:34 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Nishanth Menon <nm@...com>, Jean Delvare <jdelvare@...e.com>
Cc:	linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
	Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop
 local cache

Hi Nishanth,

On 06/26/2016 06:27 AM, Nishanth Menon wrote:
>
> Note, at this point:
> We have set CR0=0, CR1=1 (4HZ conversion rate). we do indeed have a
> 26typical (worst case 35ms) conversion time, but if we read register
> before 250 ms, we are not getting a new data, instead, we are just
> reading the same old register data.
>
Yes, I know. As I said in the description of patch 5, there is a
trade-off between caching provided by regmap for non-volatile
registers and internally managed caching.

Question for me is the practical impact. It is quite uncommon for
applications to actually read the attribute that often.

> for lowering potential i2c ops, I suggest:
> if < conversion_rate + CONVERSION_TIME_MS, provide a cached data, if
> after that, do a read.
>
That would be a possibility (we could specifically cache only the
temperature register), but I am not sure if it is worth it.

Caching was more important when the driver kept reading all registers,
which was more time consuming and also happened if only a single attribute
was read. Since the non-volatile registers are now cached by regmap,
this won't happen anymore, and reading a single register is not as
expensive as reading four. In practice, that means the amount of i2c
reads is only larger than before if an application polls the temperature
more than 10 times per second.

Do you think there is an application which does poll the temperature
that often ?

> we could do a patch over this ofcourse -> best will  be to let us do a
> configurable conversion rate. We could get upto 8Hz conversion rate
> with this chip (CR0,1=1).
>
Adding such an attribute might be worthwhile, if for nothing else to
reduce power consumption if high speed conversions are not needed.
I actually thought about it, but my ultimate goal was elsewhere -
the series served as baseline to test the new registration API
(http://www.spinics.net/lists/kernel/msg2288309.html), and I didn't
want to get too distracted.

If we add that attribute, it might be easier to do it on top of the
driver converted to the new API, though.

Thanks,
Guenter