netdev - Re: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal devices

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: <da9f0547-226d-71cf-f508-f4669fb2f5c2@collabora.com>
Date:   Mon, 27 Apr 2020 20:34:35 +0200
From:   Andrzej Pietrasiewicz <andrzej.p@...labora.com>
To:     "Zhang, Rui" <rui.zhang@...el.com>,
        "'linux-pm@...r.kernel.org'" <linux-pm@...r.kernel.org>
Cc:     "'Rafael J . Wysocki'" <rjw@...ysocki.net>,
        'Len Brown' <lenb@...nel.org>,
        'Jiri Pirko' <jiri@...lanox.com>,
        'Ido Schimmel' <idosch@...lanox.com>,
        "'David S . Miller'" <davem@...emloft.net>,
        'Peter Kaestle' <peter@...e.net>,
        'Darren Hart' <dvhart@...radead.org>,
        'Andy Shevchenko' <andy@...radead.org>,
        'Support Opensource' <support.opensource@...semi.com>,
        'Daniel Lezcano' <daniel.lezcano@...aro.org>,
        'Amit Kucheria' <amit.kucheria@...durent.com>,
        'Shawn Guo' <shawnguo@...nel.org>,
        'Sascha Hauer' <s.hauer@...gutronix.de>,
        'Pengutronix Kernel Team' <kernel@...gutronix.de>,
        'Fabio Estevam' <festevam@...il.com>,
        'NXP Linux Team' <linux-imx@....com>,
        'Heiko Stuebner' <heiko@...ech.de>,
        'Orson Zhai' <orsonzhai@...il.com>,
        'Baolin Wang' <baolin.wang7@...il.com>,
        'Chunyan Zhang' <zhang.lyra@...il.com>,
        "'linux-acpi@...r.kernel.org'" <linux-acpi@...r.kernel.org>,
        "'netdev@...r.kernel.org'" <netdev@...r.kernel.org>,
        "'platform-driver-x86@...r.kernel.org'" 
        <platform-driver-x86@...r.kernel.org>,
        "'linux-arm-kernel@...ts.infradead.org'" 
        <linux-arm-kernel@...ts.infradead.org>,
        "'kernel@...labora.com'" <kernel@...labora.com>,
        'Barlomiej Zolnierkiewicz' <b.zolnierkie@...sung.com>
Subject: Re: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal
 devices

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@...labora.com>; linux-
>> pm@...r.kernel.org
>> Cc: Rafael J . Wysocki <rjw@...ysocki.net>; Len Brown <lenb@...nel.org>;
>> Jiri Pirko <jiri@...lanox.com>; Ido Schimmel <idosch@...lanox.com>; David
>> S . Miller <davem@...emloft.net>; Peter Kaestle <peter@...e.net>; Darren
>> Hart <dvhart@...radead.org>; Andy Shevchenko <andy@...radead.org>;
>> Support Opensource <support.opensource@...semi.com>; Daniel Lezcano
>> <daniel.lezcano@...aro.org>; Amit Kucheria
>> <amit.kucheria@...durent.com>; Shawn Guo <shawnguo@...nel.org>;
>> Sascha Hauer <s.hauer@...gutronix.de>; Pengutronix Kernel Team
>> <kernel@...gutronix.de>; Fabio Estevam <festevam@...il.com>; NXP
>> Linux Team <linux-imx@....com>; Heiko Stuebner <heiko@...ech.de>;
>> Orson Zhai <orsonzhai@...il.com>; Baolin Wang
>> <baolin.wang7@...il.com>; Chunyan Zhang <zhang.lyra@...il.com>; linux-
>> acpi@...r.kernel.org; netdev@...r.kernel.org; platform-driver-
>> x86@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
>> kernel@...labora.com; Barlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
>> 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@...labora.com>
>>> Sent: Friday, April 24, 2020 12:57 AM
>>> To: linux-pm@...r.kernel.org
>>> Cc: Zhang, Rui <rui.zhang@...el.com>; Rafael J . Wysocki
>>> <rjw@...ysocki.net>; Len Brown <lenb@...nel.org>; Jiri Pirko
>>> <jiri@...lanox.com>; Ido Schimmel <idosch@...lanox.com>; David S .
>>> Miller <davem@...emloft.net>; Peter Kaestle <peter@...e.net>; Darren
>>> Hart <dvhart@...radead.org>; Andy Shevchenko <andy@...radead.org>;
>>> Support Opensource <support.opensource@...semi.com>; Daniel Lezcano
>>> <daniel.lezcano@...aro.org>; Amit Kucheria
>>> <amit.kucheria@...durent.com>; Shawn Guo <shawnguo@...nel.org>;
>> Sascha
>>> Hauer <s.hauer@...gutronix.de>; Pengutronix Kernel Team
>>> <kernel@...gutronix.de>; Fabio Estevam <festevam@...il.com>; NXP
>> Linux
>>> Team <linux-imx@....com>; Heiko Stuebner <heiko@...ech.de>; Orson
>> Zhai
>>> <orsonzhai@...il.com>; Baolin Wang <baolin.wang7@...il.com>;
>> Chunyan
>>> Zhang <zhang.lyra@...il.com>; linux- acpi@...r.kernel.org;
>>> netdev@...r.kernel.org; platform-driver- x86@...r.kernel.org;
>>> linux-arm-kernel@...ts.infradead.org;
>>> kernel@...labora.com; Andrzej Pietrasiewicz <andrzej.p@...labora.com>;
>>> Barlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
>>> 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