> -----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?
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@xxxxxxxxxxxxx>
> > ---
> > 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
