[PATCH 0/6] Consolidate the thermal_cdev_update() function

[PATCH 0/6] Consolidate the thermal_cdev_update() function

[Daniel Lezcano]

This series does the following changes:

 - Moves the calls to thermal_cdev_update() in the throttle governor's
   path

 - Reduce the scope of the function to the thermal framework internals

After these changes, all the calls to thermal_cdev_update() are located
in the throttle governor path, setting the scene to move the call to a
single place, close to the concentrator function thermal_zone_device_update().

The micro optimization to not invoke multiple times the cooling device
update is lost at the moment but it will be regain when the call will
be moved to the core.

Daniel Lezcano (6):
  thermal: hwmon: Replace the call the thermal_cdev_update()
  thermal: core: Move thermal_cdev_update next to updated=false
  thermal: core: Remove pointless 'updated' boolean
  thermal: power_allocator: Remove useless test
  thermal: core: Move the call to thermal_cdev_udpate() to the power
    allocator
  thermal: core: Make thermal_cdev_update private

 drivers/hwmon/pwm-fan.c           |  3 +--
 drivers/thermal/fair_share.c      |  3 ---
 drivers/thermal/gov_bang_bang.c   | 13 +------------
 drivers/thermal/power_allocator.c | 10 ++++------
 drivers/thermal/step_wise.c       | 13 +------------
 drivers/thermal/thermal_core.c    |  5 -----
 drivers/thermal/thermal_core.h    |  1 +
 drivers/thermal/thermal_helpers.c |  6 ------
 include/linux/thermal.h           |  4 ----
 9 files changed, 8 insertions(+), 50 deletions(-)

-- 
2.17.1

[PATCH 1/6] thermal: hwmon: Replace the call the thermal_cdev_update()

[Daniel Lezcano]

The function thermal_cdev_upadte is called from the throttling
functions in the governors not from the cooling device itself.

The cooling device is set to its maximum state and then updated. Even
if I don't get the purpose of probing the pwm-fan to its maximum
cooling state, we can replace the thermal_cdev_update() call to the
internal set_cur_state() function directly.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/hwmon/pwm-fan.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 30b7b3ea8836..a654ecdf21ab 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ctx->pwm_fan_state = ctx->pwm_fan_max_state;
 	if (IS_ENABLED(CONFIG_THERMAL)) {
 		cdev = devm_thermal_of_cooling_device_register(dev,
 			dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops);
@@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 			return ret;
 		}
 		ctx->cdev = cdev;
-		thermal_cdev_update(cdev);
+		pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state);
 	}
 
 	return 0;
-- 
2.17.1

[PATCH 2/6] thermal: core: Move thermal_cdev_update next to updated=false

[Daniel Lezcano]

The call to the thermal_cdev_update() function is done after browsing
the thermal instances which sets the updated flag by browsing them
again.

Instead of doing this, let's move the call right after setting the
cooling device 'updated' flag as it is done in the other governors.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/gov_bang_bang.c | 10 +---------
 drivers/thermal/step_wise.c     | 10 +---------
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
index 991a1c54296d..c292a69845bb 100644
--- a/drivers/thermal/gov_bang_bang.c
+++ b/drivers/thermal/gov_bang_bang.c
@@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		mutex_lock(&instance->cdev->lock);
 		instance->cdev->updated = false; /* cdev needs update */
 		mutex_unlock(&instance->cdev->lock);
+		thermal_cdev_update(instance->cdev);
 	}
 
 	mutex_unlock(&tz->lock);
@@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
  */
 static int bang_bang_control(struct thermal_zone_device *tz, int trip)
 {
-	struct thermal_instance *instance;
-
 	thermal_zone_trip_update(tz, trip);
 
-	mutex_lock(&tz->lock);
-
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		thermal_cdev_update(instance->cdev);
-
-	mutex_unlock(&tz->lock);
-
 	return 0;
 }
 
diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 2ae7198d3067..298eedac0293 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		mutex_lock(&instance->cdev->lock);
 		instance->cdev->updated = false; /* cdev needs update */
 		mutex_unlock(&instance->cdev->lock);
+		thermal_cdev_update(instance->cdev);
 	}
 
 	mutex_unlock(&tz->lock);
@@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
  */
 static int step_wise_throttle(struct thermal_zone_device *tz, int trip)
 {
-	struct thermal_instance *instance;
-
 	thermal_zone_trip_update(tz, trip);
 
 	if (tz->forced_passive)
 		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
 
-	mutex_lock(&tz->lock);
-
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		thermal_cdev_update(instance->cdev);
-
-	mutex_unlock(&tz->lock);
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH 3/6] thermal: core: Remove pointless 'updated' boolean

[Daniel Lezcano]

The sequence to update the cooling state in the thermal instances is always:

    mutex_lock(&instance->cdev->lock);
    instance->cdev->updated = false;
    mutex_unlock(&instance->cdev->lock);
    thermal_cdev_update(instance->cdev);

So each call to thermal_cdev_update() is prefixed by resetting the updated
flag which turns on to be a pointless test in the function itself.

Remove the flag.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/fair_share.c      | 3 ---
 drivers/thermal/gov_bang_bang.c   | 3 ---
 drivers/thermal/power_allocator.c | 3 ---
 drivers/thermal/step_wise.c       | 3 ---
 drivers/thermal/thermal_core.c    | 4 ----
 drivers/thermal/thermal_helpers.c | 6 ------
 include/linux/thermal.h           | 1 -
 7 files changed, 23 deletions(-)

diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
index aaa07180ab48..718de1f96cb6 100644
--- a/drivers/thermal/fair_share.c
+++ b/drivers/thermal/fair_share.c
@@ -105,9 +105,6 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 		instance->target = get_target_state(tz, cdev, percentage,
 						    cur_trip_level);
 
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false;
-		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(cdev);
 	}
 	return 0;
diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
index c292a69845bb..d678a2a0c4d4 100644
--- a/drivers/thermal/gov_bang_bang.c
+++ b/drivers/thermal/gov_bang_bang.c
@@ -61,9 +61,6 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		dev_dbg(&instance->cdev->device, "target=%d\n",
 					(int)instance->target);
 
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false; /* cdev needs update */
-		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(instance->cdev);
 	}
 
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 44636475b2a3..f8e4219cf5de 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -530,9 +530,6 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
 			continue;
 
 		instance->target = 0;
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false;
-		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(instance->cdev);
 	}
 	mutex_unlock(&tz->lock);
diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 298eedac0293..9ddff715f3dd 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -164,9 +164,6 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 			update_passive_instance(tz, trip_type, -1);
 
 		instance->initialized = true;
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false; /* cdev needs update */
-		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(instance->cdev);
 	}
 
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index ccd2417cd42e..052f77b0b0ef 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -614,9 +614,6 @@ int power_actor_set_power(struct thermal_cooling_device *cdev,
 		return ret;
 
 	instance->target = state;
-	mutex_lock(&cdev->lock);
-	cdev->updated = false;
-	mutex_unlock(&cdev->lock);
 	thermal_cdev_update(cdev);
 
 	return 0;
@@ -990,7 +987,6 @@ __thermal_cooling_device_register(struct device_node *np,
 	INIT_LIST_HEAD(&cdev->thermal_instances);
 	cdev->np = np;
 	cdev->ops = ops;
-	cdev->updated = false;
 	cdev->device.class = &thermal_class;
 	cdev->devdata = devdata;
 	thermal_cooling_device_setup_sysfs(cdev);
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 59eaf2d0fdb3..85cae31301aa 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -180,11 +180,6 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
 	unsigned long target = 0;
 
 	mutex_lock(&cdev->lock);
-	/* cooling device is updated*/
-	if (cdev->updated) {
-		mutex_unlock(&cdev->lock);
-		return;
-	}
 
 	/* Make sure cdev enters the deepest cooling state */
 	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
@@ -199,7 +194,6 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
 	if (!cdev->ops->set_cur_state(cdev, target))
 		thermal_cooling_device_stats_update(cdev, target);
 
-	cdev->updated = true;
 	mutex_unlock(&cdev->lock);
 	trace_cdev_update(cdev, target);
 	dev_dbg(&cdev->device, "set to state %lu\n", target);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 216185bb3014..08969f0be6a0 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -114,7 +114,6 @@ struct thermal_cooling_device {
 	void *devdata;
 	void *stats;
 	const struct thermal_cooling_device_ops *ops;
-	bool updated; /* true if the cooling device does not need update */
 	struct mutex lock; /* protect thermal_instances list */
 	struct list_head thermal_instances;
 	struct list_head node;
-- 
2.17.1

[PATCH 4/6] thermal: power_allocator: Remove useless test

[Daniel Lezcano]

The function power_actor_set_power() does check if the specified
cooling device is a power actor.

Call the function and handle the return code like what do the other
calls to power_actor_* functions.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/power_allocator.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index f8e4219cf5de..fd7c8de02250 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -427,11 +427,10 @@ static int allocate_power(struct thermal_zone_device *tz,
 		if (instance->trip != trip_max_desired_temperature)
 			continue;
 
-		if (!cdev_is_power_actor(instance->cdev))
+		if (power_actor_set_power(instance->cdev, instance,
+					  granted_power[i]))
 			continue;
 
-		power_actor_set_power(instance->cdev, instance,
-				      granted_power[i]);
 		total_granted_power += granted_power[i];
 
 		i++;
-- 
2.17.1

[PATCH 5/6] thermal: core: Move the call to thermal_cdev_udpate() to the power allocator

[Daniel Lezcano]

All the calls to the thermal_cdev_update() are in the governors except
one in the power_actor_set_power(). Move the update right after the
call to power_actor_set_power(), the function will be located in the
IPA governor.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/power_allocator.c | 2 ++
 drivers/thermal/thermal_core.c    | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index fd7c8de02250..339442925dfe 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -433,6 +433,8 @@ static int allocate_power(struct thermal_zone_device *tz,
 
 		total_granted_power += granted_power[i];
 
+		thermal_cdev_update(instance->cdev);
+
 		i++;
 	}
 
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 052f77b0b0ef..a6b8c0240656 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -614,7 +614,6 @@ int power_actor_set_power(struct thermal_cooling_device *cdev,
 		return ret;
 
 	instance->target = state;
-	thermal_cdev_update(cdev);
 
 	return 0;
 }
-- 
2.17.1

[PATCH 6/6] thermal: core: Make thermal_cdev_update private

[Daniel Lezcano]

Move the function thermal_cdev_update as an internal thermal function
instead of leaking it around.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.h | 1 +
 include/linux/thermal.h        | 3 ---
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index c95689586e19..4e271016b7a9 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -120,6 +120,7 @@ int thermal_build_list_of_policies(char *buf);
 
 /* Helpers */
 void thermal_zone_set_trips(struct thermal_zone_device *tz);
+void thermal_cdev_update(struct thermal_cooling_device *cdev);
 
 /* sysfs I/F */
 int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 08969f0be6a0..faf7ad031e42 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -413,7 +413,6 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
 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(
@@ -457,8 +456,6 @@ static inline int thermal_zone_get_offset(
 		struct thermal_zone_device *tz)
 { return -ENODEV; }
 
-static inline void thermal_cdev_update(struct thermal_cooling_device *cdev)
-{ }
 static inline void thermal_notify_framework(struct thermal_zone_device *tz,
 	int trip)
 { }
-- 
2.17.1

Re: [PATCH 1/6] thermal: hwmon: Replace the call the thermal_cdev_update()

[Guenter Roeck]

On 4/10/20 3:12 PM, Daniel Lezcano wrote:
> The function thermal_cdev_upadte is called from the throttling

misspelled

> functions in the governors not from the cooling device itself.
> 
> The cooling device is set to its maximum state and then updated. Even
> if I don't get the purpose of probing the pwm-fan to its maximum
> cooling state, we can replace the thermal_cdev_update() call to the
> internal set_cur_state() function directly.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/hwmon/pwm-fan.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 30b7b3ea8836..a654ecdf21ab 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ctx->pwm_fan_state = ctx->pwm_fan_max_state;
>  	if (IS_ENABLED(CONFIG_THERMAL)) {
>  		cdev = devm_thermal_of_cooling_device_register(dev,
>  			dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops);
> @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  			return ret;
>  		}
>  		ctx->cdev = cdev;
> -		thermal_cdev_update(cdev);
> +		pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state);

So far the function would only change the state if the new
state is not equal to the old state. This was the case because
pwm_fan_state was set to pwm_fan_max_state, and the call to
thermal_cdev_update() and thus pwm_fan_set_cur_state() would
do nothing except update statistics. The old code _assumed_
that the current state is pwm_fan_max_state. The new code
enforces it. That is a substantial semantic change, and it
is not really reflected in the commit message. Is that really
what you want ? If so, the commit message needs to state that
and explain the rationale.

Thanks,
Guenter

>  	}
>  
>  	return 0;
> 

Re: [PATCH 1/6] thermal: hwmon: Replace the call the thermal_cdev_update()

[Daniel Lezcano]

On 11/04/2020 03:32, Guenter Roeck wrote:
> On 4/10/20 3:12 PM, Daniel Lezcano wrote:
>> The function thermal_cdev_upadte is called from the throttling
> 
> misspelled
> 
>> functions in the governors not from the cooling device itself.
>>
>> The cooling device is set to its maximum state and then updated. Even
>> if I don't get the purpose of probing the pwm-fan to its maximum
>> cooling state, we can replace the thermal_cdev_update() call to the
>> internal set_cur_state() function directly.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/hwmon/pwm-fan.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>> index 30b7b3ea8836..a654ecdf21ab 100644
>> --- a/drivers/hwmon/pwm-fan.c
>> +++ b/drivers/hwmon/pwm-fan.c
>> @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	ctx->pwm_fan_state = ctx->pwm_fan_max_state;
>>  	if (IS_ENABLED(CONFIG_THERMAL)) {
>>  		cdev = devm_thermal_of_cooling_device_register(dev,
>>  			dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops);
>> @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>>  			return ret;
>>  		}
>>  		ctx->cdev = cdev;
>> -		thermal_cdev_update(cdev);
>> +		pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state);
> 
> So far the function would only change the state if the new
> state is not equal to the old state. This was the case because
> pwm_fan_state was set to pwm_fan_max_state, and the call to
> thermal_cdev_update() and thus pwm_fan_set_cur_state() would
> do nothing except update statistics. The old code _assumed_
> that the current state is pwm_fan_max_state. The new code
> enforces it. That is a substantial semantic change, and it
> is not really reflected in the commit message. Is that really
> what you want ? If so, the commit message needs to state that
> and explain the rationale.

Well, to be honest I'm not getting the rational of calling
thermal_cdev_update(cdev) right after
devm_thermal_of_cooling_device_register() neither setting pwm_fan_state
to pwm_fan_max_state.

Do we have the guarantee there is at this point a thermal instance
making the target state working when thermal_cdev_update is called?

Are we sure a thermal_cdev_update(cdev) is actually right here?


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

Re: [PATCH 1/6] thermal: hwmon: Replace the call the thermal_cdev_update()

[Guenter Roeck]

On 4/11/20 9:45 AM, Daniel Lezcano wrote:
> On 11/04/2020 03:32, Guenter Roeck wrote:
>> On 4/10/20 3:12 PM, Daniel Lezcano wrote:
>>> The function thermal_cdev_upadte is called from the throttling
>>
>> misspelled
>>
>>> functions in the governors not from the cooling device itself.
>>>
>>> The cooling device is set to its maximum state and then updated. Even
>>> if I don't get the purpose of probing the pwm-fan to its maximum
>>> cooling state, we can replace the thermal_cdev_update() call to the
>>> internal set_cur_state() function directly.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>>  drivers/hwmon/pwm-fan.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>>> index 30b7b3ea8836..a654ecdf21ab 100644
>>> --- a/drivers/hwmon/pwm-fan.c
>>> +++ b/drivers/hwmon/pwm-fan.c
>>> @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> -	ctx->pwm_fan_state = ctx->pwm_fan_max_state;
>>>  	if (IS_ENABLED(CONFIG_THERMAL)) {
>>>  		cdev = devm_thermal_of_cooling_device_register(dev,
>>>  			dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops);
>>> @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>>>  			return ret;
>>>  		}
>>>  		ctx->cdev = cdev;
>>> -		thermal_cdev_update(cdev);
>>> +		pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state);
>>
>> So far the function would only change the state if the new
>> state is not equal to the old state. This was the case because
>> pwm_fan_state was set to pwm_fan_max_state, and the call to
>> thermal_cdev_update() and thus pwm_fan_set_cur_state() would
>> do nothing except update statistics. The old code _assumed_
>> that the current state is pwm_fan_max_state. The new code
>> enforces it. That is a substantial semantic change, and it
>> is not really reflected in the commit message. Is that really
>> what you want ? If so, the commit message needs to state that
>> and explain the rationale.
> 
> Well, to be honest I'm not getting the rational of calling
> thermal_cdev_update(cdev) right after
> devm_thermal_of_cooling_device_register() neither setting pwm_fan_state
> to pwm_fan_max_state.
> 
Good question. The author might know/recall. Maybe the idea was that
thermal would update the state to a lower state shortly thereafter.

> Do we have the guarantee there is at this point a thermal instance
> making the target state working when thermal_cdev_update is called?
> 
> Are we sure a thermal_cdev_update(cdev) is actually right here?
> 
I don't know. I am not exactly familiar with thermal subsystem
particulars. I do recall seeing similar code in other drivers, though.

Either case, your patch does change functionality, and we should not
do that without understanding its impact.

Thanks
Guenter

Re: [PATCH 1/6] thermal: hwmon: Replace the call the thermal_cdev_update()

[Daniel Lezcano]

On 11/04/2020 19:26, Guenter Roeck wrote:
> On 4/11/20 9:45 AM, Daniel Lezcano wrote:
>> On 11/04/2020 03:32, Guenter Roeck wrote:
>>> On 4/10/20 3:12 PM, Daniel Lezcano wrote:
>>>> The function thermal_cdev_upadte is called from the throttling
>>>
>>> misspelled
>>>
>>>> functions in the governors not from the cooling device itself.
>>>>
>>>> The cooling device is set to its maximum state and then updated. Even
>>>> if I don't get the purpose of probing the pwm-fan to its maximum
>>>> cooling state, we can replace the thermal_cdev_update() call to the
>>>> internal set_cur_state() function directly.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>>>  drivers/hwmon/pwm-fan.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>>>> index 30b7b3ea8836..a654ecdf21ab 100644
>>>> --- a/drivers/hwmon/pwm-fan.c
>>>> +++ b/drivers/hwmon/pwm-fan.c
>>>> @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	ctx->pwm_fan_state = ctx->pwm_fan_max_state;
>>>>  	if (IS_ENABLED(CONFIG_THERMAL)) {
>>>>  		cdev = devm_thermal_of_cooling_device_register(dev,
>>>>  			dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops);
>>>> @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>>>>  			return ret;
>>>>  		}
>>>>  		ctx->cdev = cdev;
>>>> -		thermal_cdev_update(cdev);
>>>> +		pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state);
>>>
>>> So far the function would only change the state if the new
>>> state is not equal to the old state. This was the case because
>>> pwm_fan_state was set to pwm_fan_max_state, and the call to
>>> thermal_cdev_update() and thus pwm_fan_set_cur_state() would
>>> do nothing except update statistics. The old code _assumed_
>>> that the current state is pwm_fan_max_state. The new code
>>> enforces it. That is a substantial semantic change, and it
>>> is not really reflected in the commit message. Is that really
>>> what you want ? If so, the commit message needs to state that
>>> and explain the rationale.
>>
>> Well, to be honest I'm not getting the rational of calling
>> thermal_cdev_update(cdev) right after
>> devm_thermal_of_cooling_device_register() neither setting pwm_fan_state
>> to pwm_fan_max_state.
>>
> Good question. The author might know/recall. Maybe the idea was that
> thermal would update the state to a lower state shortly thereafter.
> 
>> Do we have the guarantee there is at this point a thermal instance
>> making the target state working when thermal_cdev_update is called?
>>
>> Are we sure a thermal_cdev_update(cdev) is actually right here?
>>
> I don't know. I am not exactly familiar with thermal subsystem
> particulars. I do recall seeing similar code in other drivers, though.

This call is done only in the governors actually.

> Either case, your patch does change functionality, and we should not
> do that without understanding its impact.

Right, so I've been hacking my board, added a pwm-fan and binded the
thermal zone to it.

As expected, the call to thermal_cdev_update() is not needed.

ctx->pwm_fan_state = ctx->pwm_fan_max_state;

intializes to a max value (in my case it is 3). Right after it calls
thermal_cdev_update() which fails to find any instance active because we
are at init time and then calls set_cur_state with the target state set
to zero and passing through a stats usage for nothing.

The ctx->pwm_fan_state is only used by the cooling device ops, so I
don't see any reason why it is set to pwm_fan_max_state before the
compilation condition.

May be there is something subtle here.

Lukasz ? Is there any reason why thermal_cdev_update() was called here ?

IMO, this function is a governor thing and it must be removed from the
cooling device.



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