linux-kernel - Re: [PATCH 1/5] thermal/drivers/mediatek/lvts: Disable monitor mode during suspend

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: <e3e9020b-f3d5-42bb-bf1e-6aa8da2d1708@notapiano>
Date: Tue, 26 Nov 2024 08:19:11 -0500
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Alexandre Mergnat <amergnat@...libre.com>,
	Balsam CHIHI <bchihi@...libre.com>, kernel@...labora.com,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Hsin-Te Yuan <yuanhsinte@...omium.org>,
	Chen-Yu Tsai <wenst@...omium.org>,
	Bernhard Rosenkränzer <bero@...libre.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	stable@...r.kernel.org
Subject: Re: [PATCH 1/5] thermal/drivers/mediatek/lvts: Disable monitor mode
 during suspend

On Tue, Nov 26, 2024 at 10:43:55AM +0100, AngeloGioacchino Del Regno wrote:
> Il 25/11/24 22:20, Nícolas F. R. A. Prado ha scritto:
> > When configured in filtered mode, the LVTS thermal controller will
> > monitor the temperature from the sensors and trigger an interrupt once a
> > thermal threshold is crossed.
> > 
> > Currently this is true even during suspend and resume. The problem with
> > that is that when enabling the internal clock of the LVTS controller in
> > lvts_ctrl_set_enable() during resume, the temperature reading can glitch
> > and appear much higher than the real one, resulting in a spurious
> > interrupt getting generated.
> > 
> > Disable the temperature monitoring and give some time for the signals to
> > stabilize during suspend in order to prevent such spurious interrupts.
> > 
> > Cc: stable@...r.kernel.org
> > Reported-by: Hsin-Te Yuan <yuanhsinte@...omium.org>
> > Closes: https://lore.kernel.org/all/20241108-lvts-v1-1-eee339c6ca20@chromium.org/
> > Fixes: 8137bb90600d ("thermal/drivers/mediatek/lvts_thermal: Add suspend and resume")
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > ---
> >   drivers/thermal/mediatek/lvts_thermal.c | 36 +++++++++++++++++++++++++++++++--
> >   1 file changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> > index 1997e91bb3be94a3059db619238aa5787edc7675..a92ff2325c40704adc537af6995b34f93c3b0650 100644
> > --- a/drivers/thermal/mediatek/lvts_thermal.c
> > +++ b/drivers/thermal/mediatek/lvts_thermal.c
> > @@ -860,6 +860,32 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
> >   	return 0;
> >   }
> > +static void lvts_ctrl_monitor_enable(struct device *dev, struct lvts_ctrl *lvts_ctrl, bool enable)
> > +{
> > +	/*
> > +	 * Bitmaps to enable each sensor on filtered mode in the MONCTL0
> > +	 * register.
> > +	 */
> > +	u32 sensor_filt_bitmap[] = { BIT(0), BIT(1), BIT(2), BIT(3) };
> > +	u32 sensor_map = 0;
> > +	int i;
> > +
> > +	if (lvts_ctrl->mode != LVTS_MSR_FILTERED_MODE)
> > +		return;
> > +
> 
> That's easier and shorter:
> 
> static void lvts_ctrl_monitor_enable( .... )
> {
> 	/* Bitmap to enable each sensor on filtered mode in the MONCTL0 register */
> 	const u32 sensor_map = GENMASK(3, 0);
> 
> 	if (lvts_ctrl->mode != LVTS_MSR_FILTERED_MODE)
> 		return;
> 
> 	/* Bits 0-3: Sensing points - Bit 9: Single point access flow */
> 	if (enable)
> 		writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));

Wait, no, here you're enabling all the sensors in the controller. We only want
to enable ones that are valid, otherwise we might get garbage data and irqs from
sensors that aren't actually there. That's why I use the
lvts_for_each_valid_sensor() helper in this patch.

Thanks,
Nícolas