linux-kernel - Re: [PATCH v2 5/6] thermal/drivers/mediatek/lvts

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:   Thu, 29 Jun 2023 17:10:34 -0400
From:   Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     kernel@...labora.com, Alexandre Mergnat <amergnat@...libre.com>,
        Balsam CHIHI <bchihi@...libre.com>,
        Chen-Yu Tsai <wenst@...omium.org>,
        Alexandre Bailon <abailon@...libre.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Amit Kucheria <amitk@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 5/6] thermal/drivers/mediatek/lvts_thermal: Don't
 leave threshold zeroed

On Fri, Jun 02, 2023 at 11:17:27AM +0200, Daniel Lezcano wrote:
> On 04/05/2023 02:48, Nícolas F. R. A. Prado wrote:
> > The thermal framework might leave the low threshold unset if there
> > aren't any lower trip points. This leaves the register zeroed, which
> > translates to a very high temperature for the low threshold. The
> > interrupt for this threshold is then immediately triggered, and the
> > state machine gets stuck, preventing any other temperature monitoring
> > interrupts to ever trigger.
> > 
> > (The same happens by not setting the Cold or Hot to Normal thresholds
> > when using those)
> > 
> > Set the unused threshold to a valid low value. This value was chosen so
> > that for any valid golden temperature read from the efuse, when the
> > value is converted to raw and back again to milliCelsius, the result
> > doesn't underflow.
> > 
> > Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Added this commit
> > 
> >   drivers/thermal/mediatek/lvts_thermal.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> > index efd1e938e1c2..951a4cb75ef6 100644
> > --- a/drivers/thermal/mediatek/lvts_thermal.c
> > +++ b/drivers/thermal/mediatek/lvts_thermal.c
> > @@ -82,6 +82,8 @@
> >   #define LVTS_HW_SHUTDOWN_MT8195		105000
> >   #define LVTS_HW_SHUTDOWN_MT8192		105000
> > +#define LVTS_MINIUM_THRESHOLD		20000
> 
> MINIMUM
> 
> So if the thermal zone reaches 20°C, the interrupt fires, the set_trips sets
> again 20°C but the interrupt won't fire until the temperature goes above
> 20°C and then crosses the temperature low threshold the way down again?

Well, actually, set_trips won't even be called since from the thermal
framework's perspective we haven't crossed trip points, ie in
__thermal_zone_set_trips():

	/* No need to change trip points */
	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
		return;

But in any case, yes, the interrupt will fire, the temperature will get updated
in the framework, and that's it. It will only fire again when a threshold is
crossed again (either by the temperature rising and falling again below this
minimum, or rising beyond the high treshold).

So basically at most this will cause a spurious interrupt when the temperature
gets low enough. I do get 34-36C on all sensors when idling though, so I doubt
that temperature is even reachable. Besides, we don't really have another option
here if we want working interrupts, the threshold needs to be set to a valid
value, and this is the lowest I've found.

And thanks for all the feedback! I'll prepare a v3 based on your comments.

Thanks,
Nícolas