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]
Date:   Mon, 27 Apr 2020 14:20:17 +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



> -----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?
There is no chance to call into monitor_thermal_zone() in thermal_zone_device_update(),
or do I miss something?

> 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.

Thanks,
rui
> 
> 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