On 20/04/2021 16:21, Lukasz Luba wrote:

Hi Daniel,

On 4/20/21 2:30 PM, Daniel Lezcano wrote:

quoted

On 19/04/2021 10:45, Lukasz Luba wrote:

[snip]

quoted

quoted

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

quoted

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.

quoted

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.

quoted

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

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

Yeah, I noticed right after sending the comments. All that should be
moved in the lockless function.

So this function becomes:

void thermal_cdev_update(struct thermal_cooling_device *cdev)
{
	mutex_lock(&cdev->lock);
	if (!cdev->updated) {
		__thermal_cdev_update(cdev);
		cdev->updated = true;
	}
	mutex_unlock(&cdev->lock);

	dev_dbg(&cdev->device, "set to state %lu\n", target);
}

We end up with the trace_cdev_update(cdev, target) inside the mutex
section but that should be fine.

quoted

         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.

Is it still true with the amended function thermal_cdev_update() ?


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