Hi,
W dniu 27.04.2020 o 16:20, Zhang, Rui pisze:
-----Original Message-----
From: Zhang, Rui
Sent: Friday, April 24, 2020 5:03 PM
To: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>; linux-
pm@xxxxxxxxxxxxxxx
Cc: Rafael J . Wysocki <rjw@xxxxxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>;
Jiri Pirko <jiri@xxxxxxxxxxxx>; Ido Schimmel <idosch@xxxxxxxxxxxx>; David
S . Miller <davem@xxxxxxxxxxxxx>; Peter Kaestle <peter@xxxxxxxx>; Darren
Hart <dvhart@xxxxxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxxxxx>;
Support Opensource <support.opensource@xxxxxxxxxxx>; Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx>; Amit Kucheria
<amit.kucheria@xxxxxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>;
Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
<kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; NXP
Linux Team <linux-imx@xxxxxxx>; Heiko Stuebner <heiko@xxxxxxxxx>;
Orson Zhai <orsonzhai@xxxxxxxxx>; Baolin Wang
<baolin.wang7@xxxxxxxxx>; Chunyan Zhang <zhang.lyra@xxxxxxxxx>; linux-
acpi@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; platform-driver-
x86@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
kernel@xxxxxxxxxxxxx; Barlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
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.
-----Original Message-----
From: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>
Sent: Friday, April 24, 2020 12:57 AM
To: linux-pm@xxxxxxxxxxxxxxx
Cc: Zhang, Rui <rui.zhang@xxxxxxxxx>; Rafael J . Wysocki
<rjw@xxxxxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>; Jiri Pirko
<jiri@xxxxxxxxxxxx>; Ido Schimmel <idosch@xxxxxxxxxxxx>; David S .
Miller <davem@xxxxxxxxxxxxx>; Peter Kaestle <peter@xxxxxxxx>; Darren
Hart <dvhart@xxxxxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxxxxx>;
Support Opensource <support.opensource@xxxxxxxxxxx>; Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx>; Amit Kucheria
<amit.kucheria@xxxxxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>;
Sascha
Hauer <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
<kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; NXP
Linux
Team <linux-imx@xxxxxxx>; Heiko Stuebner <heiko@xxxxxxxxx>; Orson
Zhai
<orsonzhai@xxxxxxxxx>; Baolin Wang <baolin.wang7@xxxxxxxxx>;
Chunyan
Zhang <zhang.lyra@xxxxxxxxx>; linux- acpi@xxxxxxxxxxxxxxx;
netdev@xxxxxxxxxxxxxxx; platform-driver- x86@xxxxxxxxxxxxxxx;
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
kernel@xxxxxxxxxxxxx; Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>;
Barlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
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.
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.
I overlooked the last line of the patch. So thermal_zone_device_update() returns
immediately if the thermal zone is disabled, right?
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.
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.
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
