Re: [PATCH v2 2/2] thermal: power_allocator: update once cooling devices when temp is low

[Lukasz Luba <lukasz.luba@arm.com>]

Hi Daniel,

On 4/20/21 2:30 PM, Daniel Lezcano wrote:
> On 19/04/2021 10:45, Lukasz Luba wrote:

[snip]

>> -		instance->cdev->updated = false;
>> +		if (update)
>> +			instance->cdev->updated = false;
>> +
>>   		mutex_unlock(&instance->cdev->lock);
>> -		(instance->cdev);
>> +
>> +		if (update)
>> +			thermal_cdev_update(instance->cdev);
> 
> This cdev update has something bad IMHO. It is protected by a mutex but
> the 'updated' field is left unprotected before calling
> thermal_cdev_update().
> 
> It is not the fault of this code but how the cooling device are updated
> and how it interacts with the thermal instances.
> 
> IMO, part of the core code needs to revisited.

I agree, but please check my comments below.

> 
> This change tight a bit more the knot.
> 
> Would it make sense to you if we create a function eg.
> __thermal_cdev_update()

I'm not sure if I assume it right that the function would only have the:
list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)

loop from the thermal_cdev_update(). But if it has only this loop then
it's too little.

> 
> And then we have:
> 
> void thermal_cdev_update(struct thermal_cooling_device *cdev)
> {
>          mutex_lock(&cdev->lock);
>          /* cooling device is updated*/
>          if (cdev->updated) {
>                  mutex_unlock(&cdev->lock);
>                  return;
>          }
> 
> 	__thermal_cdev_update(cdev);
> 
>          thermal_cdev_set_cur_state(cdev, target);

Here we are actually setting the 'target' state via:
cdev->ops->set_cur_state(cdev, target)

then we notify, then updating stats.

> 
>          cdev->updated = true;
>          mutex_unlock(&cdev->lock);
>          trace_cdev_update(cdev, target);

Also this trace is something that I'm using in my tests...

>          dev_dbg(&cdev->device, "set to state %lu\n", target);
> }
> 
> And in this file we do instead:
> 
> -		instance->cdev->updated = false;
> +		if (update)
> +			__thermal_cdev_update(instance->cdev);
>    		mutex_unlock(&instance->cdev->lock);
> -		thermal_cdev_update(instance->cdev);

Without the line above, we are not un-throttling the devices.

Regards,
Lukasz