linux-kernel - Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 29 Nov 2016 13:57:45 -0800
From:   Brian Norris <briannorris@...omium.org>
To:     Eduardo Valentin <edubezval@...il.com>
Cc:     Caesar Wang <wxt@...k-chips.com>, rui.zhang@...el.com,
        heiko@...ech.de, smbarber@...omium.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case

Hi Eduardo,

I'm not sure I completely understand what you're asking, but I'll see
what I can answer.

On Mon, Nov 28, 2016 at 05:45:54PM -0800, Eduardo Valentin wrote:
> On Mon, Nov 28, 2016 at 07:12:02PM +0800, Caesar Wang wrote:
> > The temp_to_code function will return 0 when we set the temperature to a
> > invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch
> > will prevent this case happening. That will return the max analog value to
> > indicate the temperature is invalid or over table temperature range.
> 
> <cut>
> 
> >  
> >  	/* Make sure the value is valid */
> >  	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
> 
> dummy question here, looking at your tables, if I did not miss
> something, looks like we have an accuracy of 5C steps. Not only, that,
> we also support only multiples of 5C temperatures. If that observation

Currently, that's true I think. But patch 4 actually supports doing the
linear interpolation that is claimed (but not fully implemented) in the
comments today. So with that patch, we roughly support temperatures in
between the 5C intervals.

> is correct, would it make more sense to simply check for this property,

I'm not quite sure what you mean by "this property." Do you mean to just
assume that there will be 5C intervals, and jump ahead in the table
accordingly? Seems a bit fragile; nothing really guarantees that a
future ADC supported by this driver won't have 1, 2, 6, or 7C accuracy
(and therefore a different set of steps).

> and min and max temperature check, instead of going through the binary
> search to check for valid temperature? 

I was thinking while reviewing that the binary search serves more to
complicate things than to help -- it's much harder to read (and validate
that the loop termination logic is correct). And searching through a few
dozen table entries doesn't really get much benefit from a O(n) ->
O(log(n)) speed improvement.

Anyway, I'm not sure if you were thinking along the same lines as me.

Brian