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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55943d6f-0f72-215d-1dd4-bf3996092df7@arm.com>
Date:   Tue, 20 Apr 2021 15:21:16 +0100
From:   Lukasz Luba <lukasz.luba@....com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        amitk@...nel.org, rui.zhang@...el.com
Subject: Re: [PATCH v2 2/2] thermal: power_allocator: update once cooling
 devices when temp is low

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