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

Message-ID: <142b0767-6033-4cf4-9a90-11592c8786df@collabora.com>
Date: Tue, 26 Nov 2024 15:38:19 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Nícolas F. R. A. Prado <nfraprado@...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

Il 26/11/24 14:19, Nícolas F. R. A. Prado ha scritto:
> 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.
> 

Whoa, my brain actually missed the lvts_for_each_valid_sensor()!

Okay no, then you're right - sorry for the bad example! In that case, though, I
still have one more comment.

You can constify sensor_filt_bitmap, and since the values never go higher than
BIT(3), you should also be able to spare some memory by turning that into a u8:

	const u8 sensor_filt_bitmap[] = { BIT(0), BIT(1), BIT(2), BIT(3) };

...and then I assume that there's no way valid sensors could ever read from an
index that is more than 4 (so, I assume that there's no way the loop tries to
read out of the array upper boundary).

In which case - after at least constifying the sensor_filt_bitmap array, for v2
feel free to add my

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>

...and sorry again for the initial miss :-)

Cheers,
Angelo