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: <744357E9AAD1214791ACBA4B0B90926377CF60E3@SHSMSX108.ccr.corp.intel.com>
Date:   Fri, 24 Apr 2020 09:03:29 +0000
From:   "Zhang, Rui" <rui.zhang@...el.com>
To:     Andrzej Pietrasiewicz <andrzej.p@...labora.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, 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'll try your patches and probably make an incremental patch.

Thanks,
rui

> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@...labora.com>
> ---
>  drivers/thermal/thermal_core.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c index a2a5034f76e7..03c4d8d23284 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -305,13 +305,22 @@ static void thermal_zone_device_set_polling(struct
> thermal_zone_device *tz,
>  		cancel_delayed_work(&tz->poll_queue);
>  }
> 
> +static inline bool should_stop_polling(struct thermal_zone_device *tz)
> +{
> +	return thermal_zone_device_get_mode(tz) ==
> THERMAL_DEVICE_DISABLED; }
> +
>  static void monitor_thermal_zone(struct thermal_zone_device *tz)  {
> +	bool stop;
> +
> +	stop = should_stop_polling(tz);
> +
>  	mutex_lock(&tz->lock);
> 
> -	if (tz->passive)
> +	if (!stop && tz->passive)
>  		thermal_zone_device_set_polling(tz, tz->passive_delay);
> -	else if (tz->polling_delay)
> +	else if (!stop && tz->polling_delay)
>  		thermal_zone_device_set_polling(tz, tz->polling_delay);
>  	else
>  		thermal_zone_device_set_polling(tz, 0); @@ -503,6 +512,9
> @@ void thermal_zone_device_update(struct thermal_zone_device *tz,  {
>  	int count;
> 
> +	if (should_stop_polling(tz))
> +		return;
> +
>  	if (atomic_read(&in_suspend))
>  		return;
> 
> --
> 2.17.1