[PATCH 0/6] thermal: improve handling of disabled thermal zone

[PATCH 0/6] thermal: improve handling of disabled thermal zone

[Zhang Rui]

Andrzej Pietrasiewicz has proposed two patches at
https://patchwork.kernel.org/patch/11506053/
and
https://patchwork.kernel.org/patch/11506065/
to improve the thermal zone mode support.

But there are still some issues left, and this patch set is made based on
these two patches, to improve the handling of disabled thermal zone.

Patch 1/6 to Patch 4/6 clean up the code and fix
thermal_zone_device_update() to do nothing but cancelling the polling
timer, for a disabled thermal zone.

Patch 6/6 is a prototype patch. Patch 1-5 in this patch series make it
possible to fix a known iwlwifi thermal issue, by registering the iwlwifi
thermal zone device as disabled, and enable it after firmware ready.
I'd like to get feedback from the iwlwifi experts to get it fixed.

thanks,
rui

[PATCH 1/6] iwlwifi: use thermal_zone_device_update() for temperature change

[Zhang Rui]

thermal_notify_framework() is an obsolete API, and iwlwifi is the only
user of it.
Convert iwlwifi driver to use thermal_zone_device_update() instead.

Note that, thermal_zone_device_update() is able to handle the crossed
threshold by comparing the current temperature with every trip point, so
ths_crossed variant in iwl_mvm_temp_notif() is probably not needed.
It is still left there in this patch, in case the debug information is
still needed.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
index 418e59b..6344b6b 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
@@ -203,9 +203,8 @@ void iwl_mvm_temp_notif(struct iwl_mvm *mvm, struct iwl_rx_cmd_buffer *rxb)
 
 	if (mvm->tz_device.tzone) {
 		struct iwl_mvm_thermal_device *tz_dev = &mvm->tz_device;
-
-		thermal_notify_framework(tz_dev->tzone,
-					 tz_dev->fw_trips_index[ths_crossed]);
+		thermal_zone_device_update(tz_dev->tzone,
+					   THERMAL_EVENT_UNSPECIFIED);
 	}
 #endif /* CONFIG_THERMAL */
 }
-- 
2.7.4

Re: [PATCH 1/6] iwlwifi: use thermal_zone_device_update() for temperature change

[Zhang Rui]

On Thu, 2020-04-30 at 14:32 +0800, Zhang Rui wrote:
> thermal_notify_framework() is an obsolete API, and iwlwifi is the
> only
> user of it.
> Convert iwlwifi driver to use thermal_zone_device_update() instead.
> 
> Note that, thermal_zone_device_update() is able to handle the crossed
> threshold by comparing the current temperature with every trip point,
> so
> ths_crossed variant in iwl_mvm_temp_notif() is probably not needed.
> It is still left there in this patch, in case the debug information
> is
> still needed.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Hi, Luca,

Any comments about this patch and patch 6/6?

thanks,
rui

> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> index 418e59b..6344b6b 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> @@ -203,9 +203,8 @@ void iwl_mvm_temp_notif(struct iwl_mvm *mvm,
> struct iwl_rx_cmd_buffer *rxb)
>  
>  	if (mvm->tz_device.tzone) {
>  		struct iwl_mvm_thermal_device *tz_dev = &mvm-
> >tz_device;
> -
> -		thermal_notify_framework(tz_dev->tzone,
> -					 tz_dev-
> >fw_trips_index[ths_crossed]);
> +		thermal_zone_device_update(tz_dev->tzone,
> +					   THERMAL_EVENT_UNSPECIFIED);
>  	}
>  #endif /* CONFIG_THERMAL */
>  }

Re: [PATCH 1/6] iwlwifi: use thermal_zone_device_update() for temperature change

[Luca Coelho]

On Tue, 2020-05-12 at 09:58 +0800, Zhang Rui wrote:
> On Thu, 2020-04-30 at 14:32 +0800, Zhang Rui wrote:
> > thermal_notify_framework() is an obsolete API, and iwlwifi is the
> > only
> > user of it.
> > Convert iwlwifi driver to use thermal_zone_device_update() instead.
> > 
> > Note that, thermal_zone_device_update() is able to handle the crossed
> > threshold by comparing the current temperature with every trip point,
> > so
> > ths_crossed variant in iwl_mvm_temp_notif() is probably not needed.
> > It is still left there in this patch, in case the debug information
> > is
> > still needed.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> Hi, Luca,
> 
> Any comments about this patch and patch 6/6?

Hi Rui,

Sorry for the delay, we have been making some small changes in this
code to support a new interface with our firmware and I wanted to make
sure things work together.

I'm adding Gil, who is making the changes, to this thread so he can
also take a look.

--
Cheers,
Luca.

Re: [PATCH 1/6] iwlwifi: use thermal_zone_device_update() for temperature change

[Zhang Rui]

On Mon, 2020-05-18 at 14:21 +0300, Luca Coelho wrote:
> On Tue, 2020-05-12 at 09:58 +0800, Zhang Rui wrote:
> > On Thu, 2020-04-30 at 14:32 +0800, Zhang Rui wrote:
> > > thermal_notify_framework() is an obsolete API, and iwlwifi is the
> > > only
> > > user of it.
> > > Convert iwlwifi driver to use thermal_zone_device_update()
> > > instead.
> > > 
> > > Note that, thermal_zone_device_update() is able to handle the
> > > crossed
> > > threshold by comparing the current temperature with every trip
> > > point,
> > > so
> > > ths_crossed variant in iwl_mvm_temp_notif() is probably not
> > > needed.
> > > It is still left there in this patch, in case the debug
> > > information
> > > is
> > > still needed.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > 
> > Hi, Luca,
> > 
> > Any comments about this patch and patch 6/6?
> 
> Hi Rui,
> 
> Sorry for the delay, we have been making some small changes in this
> code to support a new interface with our firmware and I wanted to
> make
> sure things work together.
> 
> I'm adding Gil, who is making the changes, to this thread so he can
> also take a look.

thanks, Luca.

Gil,
can you please take a look at this patches series?

thanks,
rui
> 
> --
> Cheers,
> Luca.
> 

Re: [PATCH 1/6] iwlwifi: use thermal_zone_device_update() for temperature change

[Rafael J. Wysocki]

On Thu, Apr 30, 2020 at 8:29 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> thermal_notify_framework() is an obsolete API, and iwlwifi is the only
> user of it.
> Convert iwlwifi driver to use thermal_zone_device_update() instead.

IMO it is rather hard to figure out what is going to change
functionally, from the driver's perspective, after this change.

In particular, is user space going to notice any change?  If so, then
what kind of change can it see?

> Note that, thermal_zone_device_update() is able to handle the crossed
> threshold by comparing the current temperature with every trip point, so
> ths_crossed variant in iwl_mvm_temp_notif() is probably not needed.
>
> It is still left there in this patch, in case the debug information is
> still needed.

Well, it should be possible to figure this out just from the code ...

> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> index 418e59b..6344b6b 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> @@ -203,9 +203,8 @@ void iwl_mvm_temp_notif(struct iwl_mvm *mvm, struct iwl_rx_cmd_buffer *rxb)
>
>         if (mvm->tz_device.tzone) {
>                 struct iwl_mvm_thermal_device *tz_dev = &mvm->tz_device;
> -
> -               thermal_notify_framework(tz_dev->tzone,
> -                                        tz_dev->fw_trips_index[ths_crossed]);
> +               thermal_zone_device_update(tz_dev->tzone,
> +                                          THERMAL_EVENT_UNSPECIFIED);
>         }
>  #endif /* CONFIG_THERMAL */
>  }
> --
> 2.7.4
>

[PATCH 2/6] thermal: core: delete thermal_notify_framework()

[Zhang Rui]

Delete thermal_notify_framework() as there is no user of it.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 18 ------------------
 include/linux/thermal.h        |  4 ----
 2 files changed, 22 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 03c4d8d..ac70545 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -532,24 +532,6 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_update);
 
-/**
- * thermal_notify_framework - Sensor drivers use this API to notify framework
- * @tz:		thermal zone device
- * @trip:	indicates which trip point has been crossed
- *
- * This function handles the trip events from sensor drivers. It starts
- * throttling the cooling devices according to the policy configured.
- * For CRITICAL and HOT trip points, this notifies the respective drivers,
- * and does actual throttling for other trip points i.e ACTIVE and PASSIVE.
- * The throttling policy is based on the configured platform data; if no
- * platform data is provided, this uses the step_wise throttling policy.
- */
-void thermal_notify_framework(struct thermal_zone_device *tz, int trip)
-{
-	handle_thermal_trip(tz, trip);
-}
-EXPORT_SYMBOL_GPL(thermal_notify_framework);
-
 static void thermal_zone_device_check(struct work_struct *work)
 {
 	struct thermal_zone_device *tz = container_of(work, struct
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index c789d4f..a87fbaf 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -420,7 +420,6 @@ int thermal_zone_get_slope(struct thermal_zone_device *tz);
 int thermal_zone_get_offset(struct thermal_zone_device *tz);
 
 void thermal_cdev_update(struct thermal_cooling_device *);
-void thermal_notify_framework(struct thermal_zone_device *, int);
 #else
 static inline struct thermal_zone_device *thermal_zone_device_register(
 	const char *type, int trips, int mask, void *devdata,
@@ -468,9 +467,6 @@ static inline int thermal_zone_get_offset(
 
 static inline void thermal_cdev_update(struct thermal_cooling_device *cdev)
 { }
-static inline void thermal_notify_framework(struct thermal_zone_device *tz,
-	int trip)
-{ }
 #endif /* CONFIG_THERMAL */
 
 static inline int thermal_zone_device_enable(struct thermal_zone_device *tz)
-- 
2.7.4

Re: [PATCH 2/6] thermal: core: delete thermal_notify_framework()

[Andrzej Pietrasiewicz]

Hi Rui,

Thanks for the series,

W dniu 30.04.2020 o 08:32, Zhang Rui pisze:
> Delete thermal_notify_framework() as there is no user of it.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>   drivers/thermal/thermal_core.c | 18 ------------------
>   include/linux/thermal.h        |  4 ----
>   2 files changed, 22 deletions(-)
> 

git grep thermal_notify_framework
Documentation/driver-api/thermal/sysfs-api.rst:4.3. thermal_notify_framework

Should the documentation be still documenting a non-existent function?

BTW, get_tz_trend() is only used in drivers/thermal/step_wise.c,
but is still exported with EXPORT_SYMBOL(). Probably does not need to
be exported anymore.

Andrzej

RE: [PATCH 2/6] thermal: core: delete thermal_notify_framework()

[Zhang, Rui]

> -----Original Message-----
> From: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Sent: Thursday, April 30, 2020 4:47 PM
> To: Zhang, Rui <rui.zhang@intel.com>; linux-pm@vger.kernel.org
> Cc: linux-wireless@vger.kernel.org; daniel.lezcano@linaro.org;
> b.zolnierkie@samsung.com; luca@coelho.fi
> Subject: Re: [PATCH 2/6] thermal: core: delete thermal_notify_framework()
> Importance: High
> 
> Hi Rui,
> 
> Thanks for the series,
> 
> W dniu 30.04.2020 o 08:32, Zhang Rui pisze:
> > Delete thermal_notify_framework() as there is no user of it.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >   drivers/thermal/thermal_core.c | 18 ------------------
> >   include/linux/thermal.h        |  4 ----
> >   2 files changed, 22 deletions(-)
> >
> 
> git grep thermal_notify_framework
> Documentation/driver-api/thermal/sysfs-api.rst:4.3.
> thermal_notify_framework
> 
> Should the documentation be still documenting a non-existent function?
> 
Right, thanks for the reminder.
Will clean it up in next version.

> BTW, get_tz_trend() is only used in drivers/thermal/step_wise.c, but is still
> exported with EXPORT_SYMBOL(). Probably does not need to be exported
> anymore.
> 
Right, that worth a separate cleanup patch.

Can you please try this patch and confirm the polling timers are queued/cancelled as expected?

thanks,
rui

Re: [PATCH 2/6] thermal: core: delete thermal_notify_framework()

[Andrzej Pietrasiewicz]

Hi Rui,

W dniu 30.04.2020 o 10:58, Zhang, Rui pisze:
> 
>> -----Original Message-----
>> From: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>> Sent: Thursday, April 30, 2020 4:47 PM
>> To: Zhang, Rui <rui.zhang@intel.com>; linux-pm@vger.kernel.org
>> Cc: linux-wireless@vger.kernel.org; daniel.lezcano@linaro.org;
>> b.zolnierkie@samsung.com; luca@coelho.fi
>> Subject: Re: [PATCH 2/6] thermal: core: delete thermal_notify_framework()
>> Importance: High
>>
>> Hi Rui,
>>
>> Thanks for the series,
>>
>> W dniu 30.04.2020 o 08:32, Zhang Rui pisze:
>>> Delete thermal_notify_framework() as there is no user of it.
>>>
>>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

<snip>

> Can you please try this patch and confirm the polling timers are queued/cancelled as expected?
> 

Yes, they are. I'm testing on an x86 machine with drivers/acpi/thermal.c.

Tested-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

Re: [PATCH 2/6] thermal: core: delete thermal_notify_framework()

[Andrzej Pietrasiewicz]

W dniu 30.04.2020 o 13:32, Andrzej Pietrasiewicz pisze:
> Hi Rui,
> 
> W dniu 30.04.2020 o 10:58, Zhang, Rui pisze:
>>
>>> -----Original Message-----
>>> From: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>>> Sent: Thursday, April 30, 2020 4:47 PM
>>> To: Zhang, Rui <rui.zhang@intel.com>; linux-pm@vger.kernel.org
>>> Cc: linux-wireless@vger.kernel.org; daniel.lezcano@linaro.org;
>>> b.zolnierkie@samsung.com; luca@coelho.fi
>>> Subject: Re: [PATCH 2/6] thermal: core: delete thermal_notify_framework()
>>> Importance: High
>>>
>>> Hi Rui,
>>>
>>> Thanks for the series,
>>>
>>> W dniu 30.04.2020 o 08:32, Zhang Rui pisze:
>>>> Delete thermal_notify_framework() as there is no user of it.
>>>>
>>>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> <snip>
> 
>> Can you please try this patch and confirm the polling timers are 
>> queued/cancelled as expected?
>>
> 
> Yes, they are. I'm testing on an x86 machine with drivers/acpi/thermal.c.

I should've said x86_64.

> 
> Tested-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> 

Re: [PATCH 2/6] thermal: core: delete thermal_notify_framework()

[Andrzej Pietrasiewicz]

Hi Rui,

W dniu 30.04.2020 o 08:32, Zhang Rui pisze:
> Delete thermal_notify_framework() as there is no user of it.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Provided that PATCH 1/6 is applied:

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
>   drivers/thermal/thermal_core.c | 18 ------------------
>   include/linux/thermal.h        |  4 ----
>   2 files changed, 22 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 03c4d8d..ac70545 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -532,24 +532,6 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
>   }
>   EXPORT_SYMBOL_GPL(thermal_zone_device_update);
>   
> -/**
> - * thermal_notify_framework - Sensor drivers use this API to notify framework
> - * @tz:		thermal zone device
> - * @trip:	indicates which trip point has been crossed
> - *
> - * This function handles the trip events from sensor drivers. It starts
> - * throttling the cooling devices according to the policy configured.
> - * For CRITICAL and HOT trip points, this notifies the respective drivers,
> - * and does actual throttling for other trip points i.e ACTIVE and PASSIVE.
> - * The throttling policy is based on the configured platform data; if no
> - * platform data is provided, this uses the step_wise throttling policy.
> - */
> -void thermal_notify_framework(struct thermal_zone_device *tz, int trip)
> -{
> -	handle_thermal_trip(tz, trip);
> -}
> -EXPORT_SYMBOL_GPL(thermal_notify_framework);
> -
>   static void thermal_zone_device_check(struct work_struct *work)
>   {
>   	struct thermal_zone_device *tz = container_of(work, struct
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index c789d4f..a87fbaf 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -420,7 +420,6 @@ int thermal_zone_get_slope(struct thermal_zone_device *tz);
>   int thermal_zone_get_offset(struct thermal_zone_device *tz);
>   
>   void thermal_cdev_update(struct thermal_cooling_device *);
> -void thermal_notify_framework(struct thermal_zone_device *, int);
>   #else
>   static inline struct thermal_zone_device *thermal_zone_device_register(
>   	const char *type, int trips, int mask, void *devdata,
> @@ -468,9 +467,6 @@ static inline int thermal_zone_get_offset(
>   
>   static inline void thermal_cdev_update(struct thermal_cooling_device *cdev)
>   { }
> -static inline void thermal_notify_framework(struct thermal_zone_device *tz,
> -	int trip)
> -{ }
>   #endif /* CONFIG_THERMAL */
>   
>   static inline int thermal_zone_device_enable(struct thermal_zone_device *tz)
> 

[PATCH 3/6] thermal: core: update polling after all trips handled

[Zhang Rui]

Move monitor_thermal_zone() from handle_thermal_trip() to
thermal_zone_device_update() because updating the polling timers after all
trips handled is sufficient.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index ac70545..04a16a9 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -430,11 +430,6 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 		handle_critical_trips(tz, trip, type);
 	else
 		handle_non_critical_trips(tz, trip);
-	/*
-	 * Alright, we handled this trip successfully.
-	 * So, start monitoring again.
-	 */
-	monitor_thermal_zone(tz);
 }
 
 static void update_temperature(struct thermal_zone_device *tz)
@@ -529,6 +524,12 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
 
 	for (count = 0; count < tz->trips; count++)
 		handle_thermal_trip(tz, count);
+
+	/*
+	 * Alright, we handled all the trips successfully.
+	 * So, start monitoring again.
+	 */
+	monitor_thermal_zone(tz);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_update);
 
-- 
2.7.4

Re: [PATCH 3/6] thermal: core: update polling after all trips handled

[Bartlomiej Zolnierkiewicz]

On 4/30/20 8:32 AM, Zhang Rui wrote:
> Move monitor_thermal_zone() from handle_thermal_trip() to
> thermal_zone_device_update() because updating the polling timers after all
> trips handled is sufficient.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  drivers/thermal/thermal_core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index ac70545..04a16a9 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -430,11 +430,6 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>  		handle_critical_trips(tz, trip, type);
>  	else
>  		handle_non_critical_trips(tz, trip);
> -	/*
> -	 * Alright, we handled this trip successfully.
> -	 * So, start monitoring again.
> -	 */
> -	monitor_thermal_zone(tz);
>  }
>  
>  static void update_temperature(struct thermal_zone_device *tz)
> @@ -529,6 +524,12 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
>  
>  	for (count = 0; count < tz->trips; count++)
>  		handle_thermal_trip(tz, count);
> +
> +	/*
> +	 * Alright, we handled all the trips successfully.
> +	 * So, start monitoring again.
> +	 */
> +	monitor_thermal_zone(tz);
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_device_update);
>  

[PATCH 4/6] thermal: core: stop polling timer for disabled thermal zone

[Zhang Rui]

For a disabled thermal zone, thermal_zone_device_update() should do
nothing but cancelling the polling timer.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 04a16a9..5f7a867 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -508,7 +508,7 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
 	int count;
 
 	if (should_stop_polling(tz))
-		return;
+		goto update_polling;
 
 	if (atomic_read(&in_suspend))
 		return;
@@ -525,6 +525,7 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
 	for (count = 0; count < tz->trips; count++)
 		handle_thermal_trip(tz, count);
 
+update_polling:
 	/*
 	 * Alright, we handled all the trips successfully.
 	 * So, start monitoring again.
-- 
2.7.4

Re: [PATCH 4/6] thermal: core: stop polling timer for disabled thermal zone

[Bartlomiej Zolnierkiewicz]

On 4/30/20 8:32 AM, Zhang Rui wrote:
> For a disabled thermal zone, thermal_zone_device_update() should do
> nothing but cancelling the polling timer.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  drivers/thermal/thermal_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 04a16a9..5f7a867 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -508,7 +508,7 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
>  	int count;
>  
>  	if (should_stop_polling(tz))
> -		return;
> +		goto update_polling;
>  
>  	if (atomic_read(&in_suspend))
>  		return;
> @@ -525,6 +525,7 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
>  	for (count = 0; count < tz->trips; count++)
>  		handle_thermal_trip(tz, count);
>  
> +update_polling:
>  	/*
>  	 * Alright, we handled all the trips successfully.
>  	 * So, start monitoring again.
> 

[PATCH 5/6] thermal: core: introduce tz_disabled() helper function

[Zhang Rui]

Rename should_stop_polling() to tz_disabled(), and make it global.
Because there are platform thermal drivers which also need this.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 17 ++++++++---------
 include/linux/thermal.h        |  2 ++
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 5f7a867..1cd5d5d0 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -305,16 +305,9 @@ 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);
+	bool stop = tz_disabled(tz);
 
 	mutex_lock(&tz->lock);
 
@@ -502,12 +495,18 @@ int thermal_zone_device_set_mode(struct thermal_zone_device *tz,
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_set_mode);
 
+bool tz_disabled(struct thermal_zone_device *tz)
+{
+	return thermal_zone_device_get_mode(tz) == THERMAL_DEVICE_DISABLED;
+}
+EXPORT_SYMBOL(tz_disabled);
+
 void thermal_zone_device_update(struct thermal_zone_device *tz,
 				enum thermal_notify_event event)
 {
 	int count;
 
-	if (should_stop_polling(tz))
+	if (tz_disabled(tz))
 		goto update_polling;
 
 	if (atomic_read(&in_suspend))
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index a87fbaf..0bc62ee 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -479,4 +479,6 @@ static inline int thermal_zone_device_disable(struct thermal_zone_device *tz)
 	return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_DISABLED);
 }
 
+bool tz_disabled(struct thermal_zone_device *tz);
+
 #endif /* __THERMAL_H__ */
-- 
2.7.4

Re: [PATCH 5/6] thermal: core: introduce tz_disabled() helper function

[Bartlomiej Zolnierkiewicz]

On 4/30/20 8:32 AM, Zhang Rui wrote:
> Rename should_stop_polling() to tz_disabled(), and make it global.
> Because there are platform thermal drivers which also need this.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/thermal_core.c | 17 ++++++++---------
>  include/linux/thermal.h        |  2 ++
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 5f7a867..1cd5d5d0 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -305,16 +305,9 @@ 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);
> +	bool stop = tz_disabled(tz);
>  
>  	mutex_lock(&tz->lock);
>  
> @@ -502,12 +495,18 @@ int thermal_zone_device_set_mode(struct thermal_zone_device *tz,
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_device_set_mode);
>  
> +bool tz_disabled(struct thermal_zone_device *tz)
> +{
> +	return thermal_zone_device_get_mode(tz) == THERMAL_DEVICE_DISABLED;
> +}
> +EXPORT_SYMBOL(tz_disabled);

Is there actual reason to not make it _GPL?

[ all other thermal core functionality seems to be _GPL anyway ]

Otherwise the patch looks fine:

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> +
>  void thermal_zone_device_update(struct thermal_zone_device *tz,
>  				enum thermal_notify_event event)
>  {
>  	int count;
>  
> -	if (should_stop_polling(tz))
> +	if (tz_disabled(tz))
>  		goto update_polling;
>  
>  	if (atomic_read(&in_suspend))
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index a87fbaf..0bc62ee 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -479,4 +479,6 @@ static inline int thermal_zone_device_disable(struct thermal_zone_device *tz)
>  	return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_DISABLED);
>  }
>  
> +bool tz_disabled(struct thermal_zone_device *tz);
> +
>  #endif /* __THERMAL_H__ */
> 

Re: [PATCH 5/6] thermal: core: introduce tz_disabled() helper function

[Zhang Rui]

On Mon, 2020-05-04 at 09:09 +0200, Bartlomiej Zolnierkiewicz wrote:
> On 4/30/20 8:32 AM, Zhang Rui wrote:
> > Rename should_stop_polling() to tz_disabled(), and make it global.
> > Because there are platform thermal drivers which also need this.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/thermal_core.c | 17 ++++++++---------
> >  include/linux/thermal.h        |  2 ++
> >  2 files changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index 5f7a867..1cd5d5d0 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -305,16 +305,9 @@ 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);
> > +	bool stop = tz_disabled(tz);
> >  
> >  	mutex_lock(&tz->lock);
> >  
> > @@ -502,12 +495,18 @@ int thermal_zone_device_set_mode(struct
> > thermal_zone_device *tz,
> >  }
> >  EXPORT_SYMBOL_GPL(thermal_zone_device_set_mode);
> >  
> > +bool tz_disabled(struct thermal_zone_device *tz)
> > +{
> > +	return thermal_zone_device_get_mode(tz) ==
> > THERMAL_DEVICE_DISABLED;
> > +}
> > +EXPORT_SYMBOL(tz_disabled);
> 
> Is there actual reason to not make it _GPL?
> 
> [ all other thermal core functionality seems to be _GPL anyway ]

Thanks for catching this, will fix in next version.
> 
> Otherwise the patch looks fine:
> 
> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

thanks for the review.

-Rui
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> > +
> >  void thermal_zone_device_update(struct thermal_zone_device *tz,
> >  				enum thermal_notify_event event)
> >  {
> >  	int count;
> >  
> > -	if (should_stop_polling(tz))
> > +	if (tz_disabled(tz))
> >  		goto update_polling;
> >  
> >  	if (atomic_read(&in_suspend))
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index a87fbaf..0bc62ee 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -479,4 +479,6 @@ static inline int
> > thermal_zone_device_disable(struct thermal_zone_device *tz)
> >  	return thermal_zone_device_set_mode(tz,
> > THERMAL_DEVICE_DISABLED);
> >  }
> >  
> > +bool tz_disabled(struct thermal_zone_device *tz);
> > +
> >  #endif /* __THERMAL_H__ */
> > 
> 
> 

[PATCH RFC 6/6] iwlwifi: enable thermal zone only when firmware loaded

[Zhang Rui]

in iwl_mvm_thermal_zone_register(), when registering a thermal zone, the
thermal subsystem will evaluate its temperature.
But iwl_mvm_tzone_get_temp() fails at this time because
iwl_mvm_firmware_running() returns false.
And that's why many users report that they see
"thermal thermal_zoneX: failed to read out thermal zone (-61)"
message during wifi driver probing.

With the changes made earlier in this patch series, the problem can be
solved by registering the iwlwifi thermal zone as disabled, and enable it
later when the firmware is ready.

*NOTE*
This is just a prototype patch to illustrate the problem and
how we can fix it based on the new thermal changes.

I'm not sure what is the proper place to enable/disable the thermal zone.
The solution in this patch causes a deadlock issue because
a) thermal_zone_device_enable() is invoked with mvm->mutex locked.
b) .get_temp() callback is invoked when enabling the thermal zone.
c) iwl_mvm_tzone_get_temp() also needs to hold mvm->mutex.
And that's why I hacked iwl_mvm_tzone_get_temp() to avoid this.

So if you have ideas on how to fix this, please feel free to propose it.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=201761
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/fw.c  |  2 ++
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h |  4 ++++
 drivers/net/wireless/intel/iwlwifi/mvm/tt.c  | 12 +++++++++---
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
index a4038f2..ff47fc6 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
@@ -1222,6 +1222,8 @@ int iwl_mvm_up(struct iwl_mvm *mvm)
 #ifdef CONFIG_THERMAL
 	/* TODO: read the budget from BIOS / Platform NVM */
 
+	if (mvm->tz_device.tzone && tz_disabled(mvm->tz_device.tzone))
+		thermal_zone_device_enable(mvm->tz_device.tzone);
 	/*
 	 * In case there is no budget from BIOS / Platform NVM the default
 	 * budget should be 2000mW (cooling state 0).
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
index afcf2b9..b964c01 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -1946,6 +1946,10 @@ static inline u32 iwl_mvm_flushable_queues(struct iwl_mvm *mvm)
 
 static inline void iwl_mvm_stop_device(struct iwl_mvm *mvm)
 {
+#ifdef CONFIG_THERMAL
+	if (mvm->tz_device.tzone && !tz_disabled(mvm->tz_device.tzone))
+		thermal_zone_device_disable(mvm->tz_device.tzone);
+#endif
 	lockdep_assert_held(&mvm->mutex);
 	iwl_fw_cancel_timestamp(&mvm->fwrt);
 	clear_bit(IWL_MVM_STATUS_FIRMWARE_RUNNING, &mvm->status);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
index 6344b6b..819dcaf 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
@@ -620,8 +620,9 @@ static int iwl_mvm_tzone_get_temp(struct thermal_zone_device *device,
 	struct iwl_mvm *mvm = (struct iwl_mvm *)device->devdata;
 	int ret;
 	int temp;
+	int locked;
 
-	mutex_lock(&mvm->mutex);
+	locked = mutex_trylock(&mvm->mutex);
 
 	if (!iwl_mvm_firmware_running(mvm) ||
 	    mvm->fwrt.cur_fw_img != IWL_UCODE_REGULAR) {
@@ -636,7 +637,8 @@ static int iwl_mvm_tzone_get_temp(struct thermal_zone_device *device,
 	*temperature = temp * 1000;
 
 out:
-	mutex_unlock(&mvm->mutex);
+	if (locked)
+		mutex_unlock(&mvm->mutex);
 	return ret;
 }
 
@@ -730,6 +732,10 @@ static  struct thermal_zone_device_ops tzone_ops = {
 /* make all trips writable */
 #define IWL_WRITABLE_TRIPS_MSK (BIT(IWL_MAX_DTS_TRIPS) - 1)
 
+static struct thermal_zone_params tz_params = {
+	.initial_mode   = THERMAL_DEVICE_DISABLED,
+};
+
 static void iwl_mvm_thermal_zone_register(struct iwl_mvm *mvm)
 {
 	int i;
@@ -749,7 +755,7 @@ static void iwl_mvm_thermal_zone_register(struct iwl_mvm *mvm)
 							IWL_MAX_DTS_TRIPS,
 							IWL_WRITABLE_TRIPS_MSK,
 							mvm, &tzone_ops,
-							NULL, 0, 0);
+							&tz_params, 0, 0);
 	if (IS_ERR(mvm->tz_device.tzone)) {
 		IWL_DEBUG_TEMP(mvm,
 			       "Failed to register to thermal zone (err = %ld)\n",
-- 
2.7.4