Re: [PATCH v8 00/29] Rework the trip points creation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Marek,

On 05/10/2022 15:05, Marek Szyprowski wrote:

On 05.10.2022 14:37, Daniel Lezcano wrote:

Hi Marek,

On 03/10/2022 23:18, Daniel Lezcano wrote:

[ ... ]

I've tested this v8 patchset after fixing the issue with Exynos TMU
with
https://lore.kernel.org/all/20221003132943.1383065-1-daniel.lezcano@xxxxxxxxxx/

patch and I got the following lockdep warning on all Exynos-based
boards:


======================================================
WARNING: possible circular locking dependency detected
6.0.0-rc1-00083-ge5c9d117223e #12945 Not tainted
------------------------------------------------------
swapper/0/1 is trying to acquire lock:
c1ce66b0 (&data->lock#2){+.+.}-{3:3}, at: exynos_get_temp+0x3c/0xc8

but task is already holding lock:
c2979b94 (&tz->lock){+.+.}-{3:3}, at:
thermal_zone_device_update.part.0+0x3c/0x528

which lock already depends on the new lock.

I'm wondering if the problem is not already there and related to
data->lock ...

Doesn't the thermal zone lock already prevent racy access to the data
structure?

Another question: if the sensor clock is disabled after reading it,
how does the hardware update the temperature and detect the programed
threshold is crossed?

just a gentle ping, as the fix will depend on your answer ;)

Sorry, I've been busy with other stuff. I thought I will fix this once I
find a bit of spare time.

Ok, that is great if you can find time to fix it up because I've other drivers to convert to the generic thermal trips.


IMHO the clock management is a bit over-engineered, as there is little
(if any) benefit from such fine grade clock management. That clock is
needed only for the AHB related part of the TMU (reading/writing the
registers). The IRQ generation and temperature measurement is clocked
from so called 'sclk' (special clock).

I also briefly looked at the code and the internal lock doesn't look to
be really necessary assuming that the thermal core already serializes
all the calls.

I looked at the code and I think the driver can be simplified (fixed?) even more.

IIUC, the sensor has multiple trip point interrupts, so if the device tree is describing more trip points than the sensor supports, there is a warning and the number of trip point is capped.

IMO that can be simplified by using two trip point interrupt because the thermal_zone_device_update() will call the set_trips callback with the new boundaries. IOW, the thermal framework sets a new trip point interrupt when one is crossed.

That should result in the simplification of the tmu_control as well as the tmu_probe function. As well as removing the limitation of the number of trip points.

In order to have that correctly working, the 'set_trips' ops must be used to call the tmu_control callback instead of calling it in tmu_probe.

The intialization workflow should be:

probe->...
 ->thermal_zone_device_register()
  ->thermal_zone_device_update()
   ->update_trip_points()
    ->ops->set_trips()
      ->tmu_control()

Also, replace the workqueue by a threaded interrupt.

Does it make sense?

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



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux