[v3,2/2] thermal: core: Stop polling DISABLED thermal devices

Commit Message

Andrzej Pietrasiewicz April 23, 2020, 4:57 p.m. UTC

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.

Signed-off-by: Andrzej Pietrasiewicz <[e-mail]>
---
 drivers/thermal/thermal_core.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Zhang Rui April 24, 2020, 9:03 a.m. UTC | #1

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 <[e-mail]>
> Sent: Friday, April 24, 2020 12:57 AM
> To: [e-mail]
> Cc: Zhang, Rui <[e-mail]>; Rafael J . Wysocki
> <[e-mail]>; Len Brown <[e-mail]>; Jiri Pirko
> <[e-mail]>; Ido Schimmel <[e-mail]>; David S .
> Miller <[e-mail]>; Peter Kaestle <[e-mail]>; Darren
> Hart <[e-mail]>; Andy Shevchenko <[e-mail]>;
> Support Opensource <[e-mail]>; Daniel Lezcano
> <[e-mail]>; Amit Kucheria
> <[e-mail]>; Shawn Guo <[e-mail]>;
> Sascha Hauer <[e-mail]>; Pengutronix Kernel Team
> <[e-mail]>; Fabio Estevam <[e-mail]>; NXP
> Linux Team <[e-mail]>; Heiko Stuebner <[e-mail]>;
> Orson Zhai <[e-mail]>; Baolin Wang
> <[e-mail]>; Chunyan Zhang <[e-mail]>; linux-
> [e-mail]; [e-mail]; platform-driver-
> [e-mail]; [e-mail];
> [e-mail]; Andrzej Pietrasiewicz <[e-mail]>;
> Barlomiej Zolnierkiewicz <[e-mail]>
> 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 <[e-mail]>
> ---
>  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

Zhang Rui April 27, 2020, 2:20 p.m. UTC | #2

> -----Original Message-----
> From: Zhang, Rui
> Sent: Friday, April 24, 2020 5:03 PM
> To: Andrzej Pietrasiewicz <[e-mail]>; linux-
> [e-mail]
> Cc: Rafael J . Wysocki <[e-mail]>; Len Brown <[e-mail]>;
> Jiri Pirko <[e-mail]>; Ido Schimmel <[e-mail]>; David
> S . Miller <[e-mail]>; Peter Kaestle <[e-mail]>; Darren
> Hart <[e-mail]>; Andy Shevchenko <[e-mail]>;
> Support Opensource <[e-mail]>; Daniel Lezcano
> <[e-mail]>; Amit Kucheria
> <[e-mail]>; Shawn Guo <[e-mail]>;
> Sascha Hauer <[e-mail]>; Pengutronix Kernel Team
> <[e-mail]>; Fabio Estevam <[e-mail]>; NXP
> Linux Team <[e-mail]>; Heiko Stuebner <[e-mail]>;
> Orson Zhai <[e-mail]>; Baolin Wang
> <[e-mail]>; Chunyan Zhang <[e-mail]>; linux-
> [e-mail]; [e-mail]; platform-driver-
> [e-mail]; [e-mail];
> [e-mail]; Barlomiej Zolnierkiewicz <[e-mail]>
> 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 <[e-mail]>
> > Sent: Friday, April 24, 2020 12:57 AM
> > To: [e-mail]
> > Cc: Zhang, Rui <[e-mail]>; Rafael J . Wysocki
> > <[e-mail]>; Len Brown <[e-mail]>; Jiri Pirko
> > <[e-mail]>; Ido Schimmel <[e-mail]>; David S .
> > Miller <[e-mail]>; Peter Kaestle <[e-mail]>; Darren
> > Hart <[e-mail]>; Andy Shevchenko <[e-mail]>;
> > Support Opensource <[e-mail]>; Daniel Lezcano
> > <[e-mail]>; Amit Kucheria
> > <[e-mail]>; Shawn Guo <[e-mail]>;
> Sascha
> > Hauer <[e-mail]>; Pengutronix Kernel Team
> > <[e-mail]>; Fabio Estevam <[e-mail]>; NXP
> Linux
> > Team <[e-mail]>; Heiko Stuebner <[e-mail]>; Orson
> Zhai
> > <[e-mail]>; Baolin Wang <[e-mail]>;
> Chunyan
> > Zhang <[e-mail]>; linux- [e-mail];
> > [e-mail]; platform-driver- [e-mail];
> > [e-mail];
> > [e-mail]; Andrzej Pietrasiewicz <[e-mail]>;
> > Barlomiej Zolnierkiewicz <[e-mail]>
> > 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 <[e-mail]>
> > ---
> >  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

Andrzej Pietrasiewicz April 27, 2020, 6:34 p.m. UTC | #3

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 <[e-mail]>; linux-
>> [e-mail]
>> Cc: Rafael J . Wysocki <[e-mail]>; Len Brown <[e-mail]>;
>> Jiri Pirko <[e-mail]>; Ido Schimmel <[e-mail]>; David
>> S . Miller <[e-mail]>; Peter Kaestle <[e-mail]>; Darren
>> Hart <[e-mail]>; Andy Shevchenko <[e-mail]>;
>> Support Opensource <[e-mail]>; Daniel Lezcano
>> <[e-mail]>; Amit Kucheria
>> <[e-mail]>; Shawn Guo <[e-mail]>;
>> Sascha Hauer <[e-mail]>; Pengutronix Kernel Team
>> <[e-mail]>; Fabio Estevam <[e-mail]>; NXP
>> Linux Team <[e-mail]>; Heiko Stuebner <[e-mail]>;
>> Orson Zhai <[e-mail]>; Baolin Wang
>> <[e-mail]>; Chunyan Zhang <[e-mail]>; linux-
>> [e-mail]; [e-mail]; platform-driver-
>> [e-mail]; [e-mail];
>> [e-mail]; Barlomiej Zolnierkiewicz <[e-mail]>
>> 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 <[e-mail]>
>>> Sent: Friday, April 24, 2020 12:57 AM
>>> To: [e-mail]
>>> Cc: Zhang, Rui <[e-mail]>; Rafael J . Wysocki
>>> <[e-mail]>; Len Brown <[e-mail]>; Jiri Pirko
>>> <[e-mail]>; Ido Schimmel <[e-mail]>; David S .
>>> Miller <[e-mail]>; Peter Kaestle <[e-mail]>; Darren
>>> Hart <[e-mail]>; Andy Shevchenko <[e-mail]>;
>>> Support Opensource <[e-mail]>; Daniel Lezcano
>>> <[e-mail]>; Amit Kucheria
>>> <[e-mail]>; Shawn Guo <[e-mail]>;
>> Sascha
>>> Hauer <[e-mail]>; Pengutronix Kernel Team
>>> <[e-mail]>; Fabio Estevam <[e-mail]>; NXP
>> Linux
>>> Team <[e-mail]>; Heiko Stuebner <[e-mail]>; Orson
>> Zhai
>>> <[e-mail]>; Baolin Wang <[e-mail]>;
>> Chunyan
>>> Zhang <[e-mail]>; linux- [e-mail];
>>> [e-mail]; platform-driver- [e-mail];
>>> [e-mail];
>>> [e-mail]; Andrzej Pietrasiewicz <[e-mail]>;
>>> Barlomiej Zolnierkiewicz <[e-mail]>
>>> 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

Zhang Rui April 28, 2020, 1:55 p.m. UTC | #4

The patch is on top of this patch set.
Run into an issue during test today, will send out after the issue resolved.

Thanks,
rui
> -----Original Message-----
> From: [e-mail] <[e-mail]>
> On Behalf Of Andrzej Pietrasiewicz
> Sent: Tuesday, April 28, 2020 2:35 AM
> To: Zhang, Rui <[e-mail]>; '[e-mail]' <linux-
> [e-mail]>
> Cc: 'Rafael J . Wysocki' <[e-mail]>; 'Len Brown' <[e-mail]>;
> 'Jiri Pirko' <[e-mail]>; 'Ido Schimmel' <[e-mail]>;
> 'David S . Miller' <[e-mail]>; 'Peter Kaestle' <[e-mail]>;
> 'Darren Hart' <[e-mail]>; 'Andy Shevchenko'
> <[e-mail]>; 'Support Opensource'
> <[e-mail]>; 'Daniel Lezcano'
> <[e-mail]>; 'Amit Kucheria'
> <[e-mail]>; 'Shawn Guo' <[e-mail]>;
> 'Sascha Hauer' <[e-mail]>; 'Pengutronix Kernel Team'
> <[e-mail]>; 'Fabio Estevam' <[e-mail]>; 'NXP
> Linux Team' <[e-mail]>; 'Heiko Stuebner' <[e-mail]>;
> 'Orson Zhai' <[e-mail]>; 'Baolin Wang'
> <[e-mail]>; 'Chunyan Zhang' <[e-mail]>;
> '[e-mail]' <[e-mail]>;
> '[e-mail]' <[e-mail]>; 'platform-driver-
> [e-mail]' <[e-mail]>; 'linux-arm-
> [e-mail]' <[e-mail]>;
> '[e-mail]' <[e-mail]>; 'Barlomiej Zolnierkiewicz'
> <[e-mail]>
> Subject: Re: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal
> devices
> Importance: High
> 
> 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 <[e-mail]>; linux-
> >> [e-mail]
> >> Cc: Rafael J . Wysocki <[e-mail]>; Len Brown
> >> <[e-mail]>; Jiri Pirko <[e-mail]>; Ido Schimmel
> >> <[e-mail]>; David S . Miller <[e-mail]>;
> Peter
> >> Kaestle <[e-mail]>; Darren Hart <[e-mail]>; Andy
> >> Shevchenko <[e-mail]>; Support Opensource
> >> <[e-mail]>; Daniel Lezcano
> >> <[e-mail]>; Amit Kucheria
> >> <[e-mail]>; Shawn Guo <[e-mail]>;
> >> Sascha Hauer <[e-mail]>; Pengutronix Kernel Team
> >> <[e-mail]>; Fabio Estevam <[e-mail]>; NXP
> >> Linux Team <[e-mail]>; Heiko Stuebner <[e-mail]>;
> >> Orson Zhai <[e-mail]>; Baolin Wang
> >> <[e-mail]>; Chunyan Zhang <[e-mail]>;
> >> linux- [e-mail]; [e-mail]; platform-driver-
> >> [e-mail]; [e-mail];
> >> [e-mail]; Barlomiej Zolnierkiewicz
> >> <[e-mail]>
> >> 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 <[e-mail]>
> >>> Sent: Friday, April 24, 2020 12:57 AM
> >>> To: [e-mail]
> >>> Cc: Zhang, Rui <[e-mail]>; Rafael J . Wysocki
> >>> <[e-mail]>; Len Brown <[e-mail]>; Jiri Pirko
> >>> <[e-mail]>; Ido Schimmel <[e-mail]>; David S .
> >>> Miller <[e-mail]>; Peter Kaestle <[e-mail]>;
> Darren
> >>> Hart <[e-mail]>; Andy Shevchenko <[e-mail]>;
> >>> Support Opensource <[e-mail]>; Daniel
> Lezcano
> >>> <[e-mail]>; Amit Kucheria
> >>> <[e-mail]>; Shawn Guo <[e-mail]>;
> >> Sascha
> >>> Hauer <[e-mail]>; Pengutronix Kernel Team
> >>> <[e-mail]>; Fabio Estevam <[e-mail]>; NXP
> >> Linux
> >>> Team <[e-mail]>; Heiko Stuebner <[e-mail]>;
> Orson
> >> Zhai
> >>> <[e-mail]>; Baolin Wang <[e-mail]>;
> >> Chunyan
> >>> Zhang <[e-mail]>; linux- [e-mail];
> >>> [e-mail]; platform-driver- [e-mail];
> >>> [e-mail];
> >>> [e-mail]; Andrzej Pietrasiewicz
> >>> <[e-mail]>; Barlomiej Zolnierkiewicz
> >>> <[e-mail]>
> >>> 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

Patch

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;