All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] thermal: imx8mm: Add hwmon support
@ 2022-04-08 11:09 ` Alexander Stein
  0 siblings, 0 replies; 10+ messages in thread
From: Alexander Stein @ 2022-04-08 11:09 UTC (permalink / raw)
  To: Daniel Lezcano, Rafael J . Wysocki, Amit Kucheria, Zhang Rui,
	Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: Alexander Stein, linux-pm, linux-arm-kernel

Expose thermal sensors as HWMON devices.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/thermal/imx8mm_thermal.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/imx8mm_thermal.c b/drivers/thermal/imx8mm_thermal.c
index af666bd9e8d4..ca8e2c393030 100644
--- a/drivers/thermal/imx8mm_thermal.c
+++ b/drivers/thermal/imx8mm_thermal.c
@@ -16,6 +16,7 @@
 #include <linux/thermal.h>
 
 #include "thermal_core.h"
+#include "thermal_hwmon.h"
 
 #define TER			0x0	/* TMU enable */
 #define TPS			0x4
@@ -176,6 +177,9 @@ static int imx8mm_tmu_probe(struct platform_device *pdev)
 			goto disable_clk;
 		}
 		tmu->sensors[i].hw_id = i;
+
+		if (devm_thermal_add_hwmon_sysfs(tmu->sensors[i].tzd))
+			dev_warn(&pdev->dev, "failed to add hwmon sysfs attributes\n");
 	}
 
 	platform_set_drvdata(pdev, tmu);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 1/2] thermal: imx8mm: Add hwmon support
@ 2022-04-08 11:09 ` Alexander Stein
  0 siblings, 0 replies; 10+ messages in thread
From: Alexander Stein @ 2022-04-08 11:09 UTC (permalink / raw)
  To: Daniel Lezcano, Rafael J . Wysocki, Amit Kucheria, Zhang Rui,
	Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: Alexander Stein, linux-pm, linux-arm-kernel

Expose thermal sensors as HWMON devices.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/thermal/imx8mm_thermal.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/imx8mm_thermal.c b/drivers/thermal/imx8mm_thermal.c
index af666bd9e8d4..ca8e2c393030 100644
--- a/drivers/thermal/imx8mm_thermal.c
+++ b/drivers/thermal/imx8mm_thermal.c
@@ -16,6 +16,7 @@
 #include <linux/thermal.h>
 
 #include "thermal_core.h"
+#include "thermal_hwmon.h"
 
 #define TER			0x0	/* TMU enable */
 #define TPS			0x4
@@ -176,6 +177,9 @@ static int imx8mm_tmu_probe(struct platform_device *pdev)
 			goto disable_clk;
 		}
 		tmu->sensors[i].hw_id = i;
+
+		if (devm_thermal_add_hwmon_sysfs(tmu->sensors[i].tzd))
+			dev_warn(&pdev->dev, "failed to add hwmon sysfs attributes\n");
 	}
 
 	platform_set_drvdata(pdev, tmu);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format
  2022-04-08 11:09 ` Alexander Stein
@ 2022-04-08 11:09   ` Alexander Stein
  -1 siblings, 0 replies; 10+ messages in thread
From: Alexander Stein @ 2022-04-08 11:09 UTC (permalink / raw)
  To: Daniel Lezcano, Rafael J . Wysocki, Amit Kucheria, Zhang Rui,
	Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: Alexander Stein, linux-pm, linux-arm-kernel

Outputs like this where -1 is printed as unsigned is somewhat misleading
 thermal thermal_zone1: Trip3[type=0,temp=48000]:trend=4,throttle=1
 thermal cooling_device3: cur_state=1
 thermal cooling_device3: old_target=-1, target=2
 thermal cooling_device3: zone1->target=1
 thermal cooling_device3: zone1->target=2
 thermal cooling_device3: zone1->target=18446744073709551615
 thermal cooling_device3: set to state 2

With THERMAL_NO_TARGET assigning -1 as unsigned it make sense to print
the target as signed integer, even if the type is actually unsigned.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
An alternative would be to change thermal_instance::target from unsigned
long to long, but this would entail a lot of API & driver changes as well
which looks less intriguing.

 drivers/thermal/thermal_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 3edd047e144f..0d0da6670267 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -199,7 +199,7 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
 
 	/* Make sure cdev enters the deepest cooling state */
 	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
-		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
+		dev_dbg(&cdev->device, "zone%d->target=%ld\n",
 			instance->tz->id, instance->target);
 		if (instance->target == THERMAL_NO_TARGET)
 			continue;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format
@ 2022-04-08 11:09   ` Alexander Stein
  0 siblings, 0 replies; 10+ messages in thread
From: Alexander Stein @ 2022-04-08 11:09 UTC (permalink / raw)
  To: Daniel Lezcano, Rafael J . Wysocki, Amit Kucheria, Zhang Rui,
	Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: Alexander Stein, linux-pm, linux-arm-kernel

Outputs like this where -1 is printed as unsigned is somewhat misleading
 thermal thermal_zone1: Trip3[type=0,temp=48000]:trend=4,throttle=1
 thermal cooling_device3: cur_state=1
 thermal cooling_device3: old_target=-1, target=2
 thermal cooling_device3: zone1->target=1
 thermal cooling_device3: zone1->target=2
 thermal cooling_device3: zone1->target=18446744073709551615
 thermal cooling_device3: set to state 2

With THERMAL_NO_TARGET assigning -1 as unsigned it make sense to print
the target as signed integer, even if the type is actually unsigned.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
An alternative would be to change thermal_instance::target from unsigned
long to long, but this would entail a lot of API & driver changes as well
which looks less intriguing.

 drivers/thermal/thermal_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 3edd047e144f..0d0da6670267 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -199,7 +199,7 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
 
 	/* Make sure cdev enters the deepest cooling state */
 	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
-		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
+		dev_dbg(&cdev->device, "zone%d->target=%ld\n",
 			instance->tz->id, instance->target);
 		if (instance->target == THERMAL_NO_TARGET)
 			continue;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format
  2022-04-08 11:09   ` Alexander Stein
@ 2022-04-14  7:35     ` Daniel Lezcano
  -1 siblings, 0 replies; 10+ messages in thread
From: Daniel Lezcano @ 2022-04-14  7:35 UTC (permalink / raw)
  To: Alexander Stein, Rafael J . Wysocki, Amit Kucheria, Zhang Rui,
	Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: linux-pm, linux-arm-kernel

On 08/04/2022 13:09, Alexander Stein wrote:
> Outputs like this where -1 is printed as unsigned is somewhat misleading
>   thermal thermal_zone1: Trip3[type=0,temp=48000]:trend=4,throttle=1
>   thermal cooling_device3: cur_state=1
>   thermal cooling_device3: old_target=-1, target=2
>   thermal cooling_device3: zone1->target=1
>   thermal cooling_device3: zone1->target=2
>   thermal cooling_device3: zone1->target=18446744073709551615
>   thermal cooling_device3: set to state 2
> 
> With THERMAL_NO_TARGET assigning -1 as unsigned it make sense to print
> the target as signed integer, even if the type is actually unsigned.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> An alternative would be to change thermal_instance::target from unsigned
> long to long, but this would entail a lot of API & driver changes as well
> which looks less intriguing.
> 
>   drivers/thermal/thermal_helpers.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
> index 3edd047e144f..0d0da6670267 100644
> --- a/drivers/thermal/thermal_helpers.c
> +++ b/drivers/thermal/thermal_helpers.c
> @@ -199,7 +199,7 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
>   
>   	/* Make sure cdev enters the deepest cooling state */
>   	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> -		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
> +		dev_dbg(&cdev->device, "zone%d->target=%ld\n",
>   			instance->tz->id, instance->target);
>   		if (instance->target == THERMAL_NO_TARGET)
>   			continue;

Actually you pointed out something fuzzy in the target values.

The unsigned long type for the target and THERMAL_NO_TARGET are not 
compatible.

It would be much simpler to have THERMAL_NO_TARGET = 0 which 
semantically makes more sense than a negative value.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format
@ 2022-04-14  7:35     ` Daniel Lezcano
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Lezcano @ 2022-04-14  7:35 UTC (permalink / raw)
  To: Alexander Stein, Rafael J . Wysocki, Amit Kucheria, Zhang Rui,
	Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: linux-pm, linux-arm-kernel

On 08/04/2022 13:09, Alexander Stein wrote:
> Outputs like this where -1 is printed as unsigned is somewhat misleading
>   thermal thermal_zone1: Trip3[type=0,temp=48000]:trend=4,throttle=1
>   thermal cooling_device3: cur_state=1
>   thermal cooling_device3: old_target=-1, target=2
>   thermal cooling_device3: zone1->target=1
>   thermal cooling_device3: zone1->target=2
>   thermal cooling_device3: zone1->target=18446744073709551615
>   thermal cooling_device3: set to state 2
> 
> With THERMAL_NO_TARGET assigning -1 as unsigned it make sense to print
> the target as signed integer, even if the type is actually unsigned.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> An alternative would be to change thermal_instance::target from unsigned
> long to long, but this would entail a lot of API & driver changes as well
> which looks less intriguing.
> 
>   drivers/thermal/thermal_helpers.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
> index 3edd047e144f..0d0da6670267 100644
> --- a/drivers/thermal/thermal_helpers.c
> +++ b/drivers/thermal/thermal_helpers.c
> @@ -199,7 +199,7 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
>   
>   	/* Make sure cdev enters the deepest cooling state */
>   	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> -		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
> +		dev_dbg(&cdev->device, "zone%d->target=%ld\n",
>   			instance->tz->id, instance->target);
>   		if (instance->target == THERMAL_NO_TARGET)
>   			continue;

Actually you pointed out something fuzzy in the target values.

The unsigned long type for the target and THERMAL_NO_TARGET are not 
compatible.

It would be much simpler to have THERMAL_NO_TARGET = 0 which 
semantically makes more sense than a negative value.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format
  2022-04-14  7:35     ` Daniel Lezcano
@ 2022-05-10 22:48       ` Nitin Garg
  -1 siblings, 0 replies; 10+ messages in thread
From: Nitin Garg @ 2022-05-10 22:48 UTC (permalink / raw)
  To: Daniel Lezcano, Alexander Stein, Rafael J . Wysocki,
	Amit Kucheria, Zhang Rui, Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: linux-pm, linux-arm-kernel


On 08/04/2022 13:09, Alexander Stein wrote:
>> Outputs like this where -1 is printed as unsigned is somewhat misleading
>>   thermal thermal_zone1: Trip3[type=0,temp=48000]:trend=4,throttle=1
>>   thermal cooling_device3: cur_state=1
>>   thermal cooling_device3: old_target=-1, target=2
>>   thermal cooling_device3: zone1->target=1
>>   thermal cooling_device3: zone1->target=2
>>   thermal cooling_device3: zone1->target=18446744073709551615
>>   thermal cooling_device3: set to state 2
>> 
>> With THERMAL_NO_TARGET assigning -1 as unsigned it make sense to print
>> the target as signed integer, even if the type is actually unsigned.
>> 
>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>> ---
>> An alternative would be to change thermal_instance::target from unsigned
>> long to long, but this would entail a lot of API & driver changes as well
>> which looks less intriguing.
>> 
>>   drivers/thermal/thermal_helpers.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
>> index 3edd047e144f..0d0da6670267 100644
>> --- a/drivers/thermal/thermal_helpers.c
>> +++ b/drivers/thermal/thermal_helpers.c
>> @@ -199,7 +199,7 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
>>   
>>   	/* Make sure cdev enters the deepest cooling state */
>>   	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
>> -		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
>> +		dev_dbg(&cdev->device, "zone%d->target=%ld\n",
>>   			instance->tz->id, instance->target);
>>   		if (instance->target == THERMAL_NO_TARGET)
>>   			continue;
>
>Actually you pointed out something fuzzy in the target values.
>
>The unsigned long type for the target and THERMAL_NO_TARGET are not 
>compatible.
>
>It would be much simpler to have THERMAL_NO_TARGET = 0 which 
>semantically makes more sense than a negative value.

The compare of unsigned long and negative int is bad idea.
But there is serious problem introduced by "thermal: core: Add notifications call in the framework" patch.
When system resumes from mem suspend first time (this happen only on 1st resume), the thermal notification is sent to drivers with value of 0 (meaning system is no longer hot).
This is due to the fact target is init to 0 and when there is only 1 cooling device; it gets out of the loop (due to continue;) with target still set to 0 and calls thermal_cdev_set_cur_state(cdev, target).
From there thermal_notify_cdev_state_update is called with argument of 0 which notifies drivers with value of 0.

May be "unsigned long target" should be initialized to THERMAL_NO_TARGET instead of 0.

[   29.107048] OOM killer enabled.
[   29.110225] Restarting tasks ... done.
[   29.124816] thermal cooling_device0: zone0->target=18446744073709551615
[   29.138388] GPU0: Hot alarm is canceled. 
[   29.145399] thermal cooling_device0: set to state 0
[   29.198954] PM: suspend exit

-- 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=04%7C01%7Cnitin.garg%40nxp.com%7C9b8d8247b7a24882faab08da1dea3606%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637855189012474295%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=wAyLn2XgePgyZJJH50LC1PL1XHOQ%2BXjCw%2FJeq2qlSfw%3D&amp;reserved=0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format
@ 2022-05-10 22:48       ` Nitin Garg
  0 siblings, 0 replies; 10+ messages in thread
From: Nitin Garg @ 2022-05-10 22:48 UTC (permalink / raw)
  To: Daniel Lezcano, Alexander Stein, Rafael J . Wysocki,
	Amit Kucheria, Zhang Rui, Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: linux-pm, linux-arm-kernel


On 08/04/2022 13:09, Alexander Stein wrote:
>> Outputs like this where -1 is printed as unsigned is somewhat misleading
>>   thermal thermal_zone1: Trip3[type=0,temp=48000]:trend=4,throttle=1
>>   thermal cooling_device3: cur_state=1
>>   thermal cooling_device3: old_target=-1, target=2
>>   thermal cooling_device3: zone1->target=1
>>   thermal cooling_device3: zone1->target=2
>>   thermal cooling_device3: zone1->target=18446744073709551615
>>   thermal cooling_device3: set to state 2
>> 
>> With THERMAL_NO_TARGET assigning -1 as unsigned it make sense to print
>> the target as signed integer, even if the type is actually unsigned.
>> 
>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>> ---
>> An alternative would be to change thermal_instance::target from unsigned
>> long to long, but this would entail a lot of API & driver changes as well
>> which looks less intriguing.
>> 
>>   drivers/thermal/thermal_helpers.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
>> index 3edd047e144f..0d0da6670267 100644
>> --- a/drivers/thermal/thermal_helpers.c
>> +++ b/drivers/thermal/thermal_helpers.c
>> @@ -199,7 +199,7 @@ void __thermal_cdev_update(struct thermal_cooling_device *cdev)
>>   
>>   	/* Make sure cdev enters the deepest cooling state */
>>   	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
>> -		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
>> +		dev_dbg(&cdev->device, "zone%d->target=%ld\n",
>>   			instance->tz->id, instance->target);
>>   		if (instance->target == THERMAL_NO_TARGET)
>>   			continue;
>
>Actually you pointed out something fuzzy in the target values.
>
>The unsigned long type for the target and THERMAL_NO_TARGET are not 
>compatible.
>
>It would be much simpler to have THERMAL_NO_TARGET = 0 which 
>semantically makes more sense than a negative value.

The compare of unsigned long and negative int is bad idea.
But there is serious problem introduced by "thermal: core: Add notifications call in the framework" patch.
When system resumes from mem suspend first time (this happen only on 1st resume), the thermal notification is sent to drivers with value of 0 (meaning system is no longer hot).
This is due to the fact target is init to 0 and when there is only 1 cooling device; it gets out of the loop (due to continue;) with target still set to 0 and calls thermal_cdev_set_cur_state(cdev, target).
From there thermal_notify_cdev_state_update is called with argument of 0 which notifies drivers with value of 0.

May be "unsigned long target" should be initialized to THERMAL_NO_TARGET instead of 0.

[   29.107048] OOM killer enabled.
[   29.110225] Restarting tasks ... done.
[   29.124816] thermal cooling_device0: zone0->target=18446744073709551615
[   29.138388] GPU0: Hot alarm is canceled. 
[   29.145399] thermal cooling_device0: set to state 0
[   29.198954] PM: suspend exit

-- 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=04%7C01%7Cnitin.garg%40nxp.com%7C9b8d8247b7a24882faab08da1dea3606%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637855189012474295%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=wAyLn2XgePgyZJJH50LC1PL1XHOQ%2BXjCw%2FJeq2qlSfw%3D&amp;reserved=0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: (EXT) RE: [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format
  2022-05-10 22:48       ` Nitin Garg
@ 2022-05-11 10:17         ` Alexander Stein
  -1 siblings, 0 replies; 10+ messages in thread
From: Alexander Stein @ 2022-05-11 10:17 UTC (permalink / raw)
  To: Daniel Lezcano, Rafael J . Wysocki, Amit Kucheria, Zhang Rui,
	Shawn Guo, Sascha Hauer, Fabio Estevam, Nitin Garg
  Cc: linux-pm, linux-arm-kernel

Hello,

Am Mittwoch, 11. Mai 2022, 00:48:00 CEST schrieb Nitin Garg:
> On 08/04/2022 13:09, Alexander Stein wrote:
> >> Outputs like this where -1 is printed as unsigned is somewhat misleading
> >> 
> >>   thermal thermal_zone1: Trip3[type=0,temp=48000]:trend=4,throttle=1
> >>   thermal cooling_device3: cur_state=1
> >>   thermal cooling_device3: old_target=-1, target=2
> >>   thermal cooling_device3: zone1->target=1
> >>   thermal cooling_device3: zone1->target=2
> >>   thermal cooling_device3: zone1->target=18446744073709551615
> >>   thermal cooling_device3: set to state 2
> >> 
> >> With THERMAL_NO_TARGET assigning -1 as unsigned it make sense to print
> >> the target as signed integer, even if the type is actually unsigned.
> >> 
> >> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >> ---
> >> An alternative would be to change thermal_instance::target from unsigned
> >> long to long, but this would entail a lot of API & driver changes as well
> >> which looks less intriguing.
> >> 
> >>   drivers/thermal/thermal_helpers.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/thermal/thermal_helpers.c
> >> b/drivers/thermal/thermal_helpers.c index 3edd047e144f..0d0da6670267
> >> 100644
> >> --- a/drivers/thermal/thermal_helpers.c
> >> +++ b/drivers/thermal/thermal_helpers.c
> >> @@ -199,7 +199,7 @@ void __thermal_cdev_update(struct
> >> thermal_cooling_device *cdev)>> 
> >>   	/* Make sure cdev enters the deepest cooling state */
> >>   	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) 
{
> >> 
> >> -		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
> >> +		dev_dbg(&cdev->device, "zone%d->target=%ld\n",
> >> 
> >>   			instance->tz->id, instance->target);
> >>   		
> >>   		if (instance->target == THERMAL_NO_TARGET)
> >>   		
> >>   			continue;
> >
> >Actually you pointed out something fuzzy in the target values.
> >
> >The unsigned long type for the target and THERMAL_NO_TARGET are not
> >compatible.
> >
> >It would be much simpler to have THERMAL_NO_TARGET = 0 which
> >semantically makes more sense than a negative value.

Is it identical? Apparently target value is used differently in each governor. 
At least for gov_bang_bang 'THERMAL_NO_TARGET = 0' is no difference. Im not so 
sure about gov_step_wise.

> The compare of unsigned long and negative int is bad idea.

Well, THERMAL_NO_TARGET actually is an unsigned long (-1UL), so the comparison 
is unsigned long to unsigned long, so it should not be an issue.
But this implies that printing the target as unsigned int, results in a huge 
number, not immediately recognizable as -1, which I tried to address here.

> But there is serious problem introduced by "thermal: core: Add notifications
> call in the framework" patch. When system resumes from mem suspend first
> time (this happen only on 1st resume), the thermal notification is sent to
> drivers with value of 0 (meaning system is no longer hot). This is due to
> the fact target is init to 0 and when there is only 1 cooling device; it
> gets out of the loop (due to continue;) with target still set to 0 and
> calls thermal_cdev_set_cur_state(cdev, target). From there
> thermal_notify_cdev_state_update is called with argument of 0 which
> notifies drivers with value of 0.
> 
> May be "unsigned long target" should be initialized to THERMAL_NO_TARGET
> instead of 0.
> 
> [   29.107048] OOM killer enabled.
> [   29.110225] Restarting tasks ... done.
> [   29.124816] thermal cooling_device0: zone0->target=18446744073709551615
> [   29.138388] GPU0: Hot alarm is canceled.
> [   29.145399] thermal cooling_device0: set to state 0
> [   29.198954] PM: suspend exit

Is it legal to pass THERMAL_NO_TARGET to .set_cur_state()? At least pwm-fan 
will return -EINVAL in this case.

Alexander



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: (EXT) RE: [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format
@ 2022-05-11 10:17         ` Alexander Stein
  0 siblings, 0 replies; 10+ messages in thread
From: Alexander Stein @ 2022-05-11 10:17 UTC (permalink / raw)
  To: Daniel Lezcano, Rafael J . Wysocki, Amit Kucheria, Zhang Rui,
	Shawn Guo, Sascha Hauer, Fabio Estevam, Nitin Garg
  Cc: linux-pm, linux-arm-kernel

Hello,

Am Mittwoch, 11. Mai 2022, 00:48:00 CEST schrieb Nitin Garg:
> On 08/04/2022 13:09, Alexander Stein wrote:
> >> Outputs like this where -1 is printed as unsigned is somewhat misleading
> >> 
> >>   thermal thermal_zone1: Trip3[type=0,temp=48000]:trend=4,throttle=1
> >>   thermal cooling_device3: cur_state=1
> >>   thermal cooling_device3: old_target=-1, target=2
> >>   thermal cooling_device3: zone1->target=1
> >>   thermal cooling_device3: zone1->target=2
> >>   thermal cooling_device3: zone1->target=18446744073709551615
> >>   thermal cooling_device3: set to state 2
> >> 
> >> With THERMAL_NO_TARGET assigning -1 as unsigned it make sense to print
> >> the target as signed integer, even if the type is actually unsigned.
> >> 
> >> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >> ---
> >> An alternative would be to change thermal_instance::target from unsigned
> >> long to long, but this would entail a lot of API & driver changes as well
> >> which looks less intriguing.
> >> 
> >>   drivers/thermal/thermal_helpers.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/thermal/thermal_helpers.c
> >> b/drivers/thermal/thermal_helpers.c index 3edd047e144f..0d0da6670267
> >> 100644
> >> --- a/drivers/thermal/thermal_helpers.c
> >> +++ b/drivers/thermal/thermal_helpers.c
> >> @@ -199,7 +199,7 @@ void __thermal_cdev_update(struct
> >> thermal_cooling_device *cdev)>> 
> >>   	/* Make sure cdev enters the deepest cooling state */
> >>   	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) 
{
> >> 
> >> -		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
> >> +		dev_dbg(&cdev->device, "zone%d->target=%ld\n",
> >> 
> >>   			instance->tz->id, instance->target);
> >>   		
> >>   		if (instance->target == THERMAL_NO_TARGET)
> >>   		
> >>   			continue;
> >
> >Actually you pointed out something fuzzy in the target values.
> >
> >The unsigned long type for the target and THERMAL_NO_TARGET are not
> >compatible.
> >
> >It would be much simpler to have THERMAL_NO_TARGET = 0 which
> >semantically makes more sense than a negative value.

Is it identical? Apparently target value is used differently in each governor. 
At least for gov_bang_bang 'THERMAL_NO_TARGET = 0' is no difference. Im not so 
sure about gov_step_wise.

> The compare of unsigned long and negative int is bad idea.

Well, THERMAL_NO_TARGET actually is an unsigned long (-1UL), so the comparison 
is unsigned long to unsigned long, so it should not be an issue.
But this implies that printing the target as unsigned int, results in a huge 
number, not immediately recognizable as -1, which I tried to address here.

> But there is serious problem introduced by "thermal: core: Add notifications
> call in the framework" patch. When system resumes from mem suspend first
> time (this happen only on 1st resume), the thermal notification is sent to
> drivers with value of 0 (meaning system is no longer hot). This is due to
> the fact target is init to 0 and when there is only 1 cooling device; it
> gets out of the loop (due to continue;) with target still set to 0 and
> calls thermal_cdev_set_cur_state(cdev, target). From there
> thermal_notify_cdev_state_update is called with argument of 0 which
> notifies drivers with value of 0.
> 
> May be "unsigned long target" should be initialized to THERMAL_NO_TARGET
> instead of 0.
> 
> [   29.107048] OOM killer enabled.
> [   29.110225] Restarting tasks ... done.
> [   29.124816] thermal cooling_device0: zone0->target=18446744073709551615
> [   29.138388] GPU0: Hot alarm is canceled.
> [   29.145399] thermal cooling_device0: set to state 0
> [   29.198954] PM: suspend exit

Is it legal to pass THERMAL_NO_TARGET to .set_cur_state()? At least pwm-fan 
will return -EINVAL in this case.

Alexander



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-05-11 10:18 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-08 11:09 [PATCH 1/2] thermal: imx8mm: Add hwmon support Alexander Stein
2022-04-08 11:09 ` Alexander Stein
2022-04-08 11:09 ` [PATCH 2/2] thermal/drivers/thermal_helpers: Adjust output format Alexander Stein
2022-04-08 11:09   ` Alexander Stein
2022-04-14  7:35   ` Daniel Lezcano
2022-04-14  7:35     ` Daniel Lezcano
2022-05-10 22:48     ` Nitin Garg
2022-05-10 22:48       ` Nitin Garg
2022-05-11 10:17       ` (EXT) " Alexander Stein
2022-05-11 10:17         ` Alexander Stein

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.