RE: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal devices

The patch is on top of this patch set.
Run into an issue during test today, will send out after the issue resolved.

Thanks,
rui

-----Original Message-----
From: linux-acpi-owner@vger.kernel.org <redacted>
On Behalf Of Andrzej Pietrasiewicz
Sent: Tuesday, April 28, 2020 2:35 AM
To: Zhang, Rui <rui.zhang@intel.com>; 'linux-pm@vger.kernel.org' <linux-
pm@vger.kernel.org>
Cc: 'Rafael J . Wysocki' <redacted>; 'Len Brown' <lenb@kernel.org>;
'Jiri Pirko' [off-list ref]; 'Ido Schimmel' [off-list ref];
'David S . Miller' [off-list ref]; 'Peter Kaestle' [off-list ref];
'Darren Hart' [off-list ref]; 'Andy Shevchenko'
[off-list ref]; 'Support Opensource'
[off-list ref]; 'Daniel Lezcano'
[off-list ref]; 'Amit Kucheria'
[off-list ref]; 'Shawn Guo' [off-list ref];
'Sascha Hauer' [off-list ref]; 'Pengutronix Kernel Team'
[off-list ref]; 'Fabio Estevam' [off-list ref]; 'NXP
Linux Team' [off-list ref]; 'Heiko Stuebner' [off-list ref];
'Orson Zhai' [off-list ref]; 'Baolin Wang'
[off-list ref]; 'Chunyan Zhang' [off-list ref];
'linux-acpi@vger.kernel.org' [off-list ref];
'netdev@vger.kernel.org' [off-list ref]; 'platform-driver-
x86@vger.kernel.org' [off-list ref]; 'linux-arm-
kernel@lists.infradead.org' [off-list ref];
'kernel@collabora.com' [off-list ref]; 'Barlomiej Zolnierkiewicz'
[off-list ref]
Subject: Re: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal
devices
Importance: High

Hi,

W dniu 27.04.2020 o 16:20, Zhang, Rui pisze:

quoted

quoted

-----Original Message-----
From: Zhang, Rui
Sent: Friday, April 24, 2020 5:03 PM
To: Andrzej Pietrasiewicz <redacted>; linux-
pm@vger.kernel.org
Cc: Rafael J . Wysocki <redacted>; Len Brown
[off-list ref]; Jiri Pirko [off-list ref]; Ido Schimmel
[off-list ref]; David S . Miller [off-list ref];

Peter

quoted

quoted

Kaestle [off-list ref]; Darren Hart [off-list ref]; Andy
Shevchenko [off-list ref]; Support Opensource
[off-list ref]; Daniel Lezcano
[off-list ref]; Amit Kucheria
[off-list ref]; Shawn Guo [off-list ref];
Sascha Hauer [off-list ref]; Pengutronix Kernel Team
[off-list ref]; Fabio Estevam [off-list ref]; NXP
Linux Team [off-list ref]; Heiko Stuebner [off-list ref];
Orson Zhai [off-list ref]; Baolin Wang
[off-list ref]; Chunyan Zhang [off-list ref];
linux- acpi@vger.kernel.org; netdev@vger.kernel.org; platform-driver-
x86@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
kernel@collabora.com; Barlomiej Zolnierkiewicz
[off-list ref]
Subject: RE: [PATCH v3 2/2] thermal: core: Stop polling DISABLED
thermal devices

Hi, Andrzej,

Thanks for the patches. My Linux laptop was broken and won't get
fixed till next week, so I may lost some of the discussions previously.

quoted

-----Original Message-----
From: Andrzej Pietrasiewicz <redacted>
Sent: Friday, April 24, 2020 12:57 AM
To: linux-pm@vger.kernel.org
Cc: Zhang, Rui <rui.zhang@intel.com>; Rafael J . Wysocki
[off-list ref]; Len Brown [off-list ref]; Jiri Pirko
[off-list ref]; Ido Schimmel [off-list ref]; David S .
Miller [off-list ref]; Peter Kaestle [off-list ref];

Darren

quoted

quoted

quoted

Hart [off-list ref]; Andy Shevchenko [off-list ref];
Support Opensource [off-list ref]; Daniel

Lezcano

quoted

quoted

quoted

[off-list ref]; Amit Kucheria
[off-list ref]; Shawn Guo [off-list ref];

Sascha

quoted

Hauer [off-list ref]; Pengutronix Kernel Team
[off-list ref]; Fabio Estevam [off-list ref]; NXP

Linux

quoted

Team [off-list ref]; Heiko Stuebner [off-list ref];

Orson

quoted

quoted

Zhai

quoted

[off-list ref]; Baolin Wang [off-list ref];

Chunyan

quoted

Zhang [off-list ref]; linux- acpi@vger.kernel.org;
netdev@vger.kernel.org; platform-driver- x86@vger.kernel.org;
linux-arm-kernel@lists.infradead.org;
kernel@collabora.com; Andrzej Pietrasiewicz
[off-list ref]; Barlomiej Zolnierkiewicz
[off-list ref]
Subject: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal
devices
Importance: High

Polling DISABLED devices is not desired, as all such "disabled"
devices are meant to be handled by userspace. This patch introduces
and uses
should_stop_polling() to decide whether the device should be polled
or

not.

quoted

Thanks for the fix, and IMO, this reveal some more problems.
Say, we need to define "DISABLED" thermal zone.
Can we read the temperature? Can we trust the trip point value?

IMO, a disabled thermal zone does not mean it is handled by
userspace, because that is what the userspace governor designed for.
Instead, if a thermal zone is disabled, in
thermal_zone_device_update(), we should basically skip all the other

operations as well.

quoted

quoted

I overlooked the last line of the patch. So
thermal_zone_device_update() returns immediately if the thermal zone is

disabled, right?

quoted

But how can we stop polling in this case?

It does stop. However, I indeed observe an extra call to
thermal_zone_device_update() before it fully stops.
I think what happens is this:

- storing "disabled" in mode ends up in thermal_zone_device_set_mode(),
which calls driver's ->set_mode() and then calls
thermal_zone_device_update(), which returns immediately and does not
touch the tz->poll_queue delayed work

- thermal_zone_device_update() is called from the delayed work when its
time comes and this time it also returns immediately, not modifying the said
delayed work, so polling effectively stops now.

quoted

There is no chance to call into monitor_thermal_zone() in
thermal_zone_device_update(), or do I miss something?

Without the last "if" statement in this patch polling stops with the first call to
thermal_zone_device_update() because it indeed disables the delayed work.

So you are probably right - that last "if" should not be introduced.

quoted

quoted

I'll try your patches and probably make an incremental patch.

I have finished a small patch set to improve this based on my
understanding, and will post it tomorrow after testing.

Is your small patchset based on top of this series or is it a completely
rewritten version?

Andrzej