10 Oct, 2015
1 commit
-
In the function cpufreq_get_requested_power, the memory allocated
for load_cpu is live within the function only. And after the
allocation it is immediately freed with devm_kfree. There is no
need to allocate memory for load_cpu with devm function so replace
devm_kcalloc with kcalloc and devm_kfree with kfree.Signed-off-by: Vaishali Thakkar
Acked-by: Viresh Kumar
Acked-by: Javi Merino
Signed-off-by: Zhang Rui
14 Sep, 2015
2 commits
-
The power table is not being freed on error from cpufreq_cooling
register or when unregistering. Free it.Fixes: c36cf0717631 ("thermal: cpu_cooling: implement the power cooling device API")
Cc: Zhang Rui
Cc: Eduardo Valentin
Signed-off-by: Javi Merino
Signed-off-by: Eduardo Valentin -
build_dyn_power_table() allocates the power table while holding
rcu_read_lock. kcalloc using GFP_KERNEL may sleep, so it can't be
called in an RCU read-side path.Move the rcu protection to the part of the function that really needs
it: the part that handles the dev_pm_opp pointer received from
dev_pm_opp_find_freq_ceil(). In the unlikely case that there is an OPP
added to the cpu while this function is running, return -EAGAIN.Fixes: c36cf0717631 ("thermal: cpu_cooling: implement the power cooling device API")
Cc: Zhang Rui
Cc: Eduardo Valentin
Signed-off-by: Javi Merino
Signed-off-by: Eduardo Valentin
15 Aug, 2015
7 commits
-
policy->max is the maximum allowed frequency defined by user and
clipped_freq is the maximum that thermal constraints allow.If clipped_freq is lower than policy->max, then we need to readjust
policy->max.But, if clipped_freq is greater than policy->max, we don't need to do
anything. We used to call cpufreq_verify_within_limits() in this case,
but it doesn't change anything in this case.Lets skip this unnecessary call and write a comment that explains this.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
That's what it is for, lets name it properly.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
That's what it is for, lets name it properly.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
We just need to take care of single event here and there is no need to
increase indentation level of most of the code (which causes lines
longer that 80 columns to break).Kill the switch block.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
If a valid cpufreq_dev is found for policy->cpu, we should update the
policy and quit the for loop. There is no need to keep traversing the
list of cpufreq_dev's.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
Its always set before getting used, don't initialize it.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
A recent change to the cpu_cooling code introduced a AB-BA deadlock
scenario between the cpufreq_policy_notifier_list rwsem and the
cooling_cpufreq_lock. This is caused by cooling_cpufreq_lock being held
before the registration/removal of the notifier block (an operation
which takes the rwsem), and the notifier code itself which takes the
locks in the reverse order:======================================================
[ INFO: possible circular locking dependency detected ]
3.18.0+ #1453 Not tainted
-------------------------------------------------------
rc.local/770 is trying to acquire lock:
(cooling_cpufreq_lock){+.+.+.}, at: [] cpufreq_thermal_notifier+0x34/0xfcbut task is already holding lock:
((cpufreq_policy_notifier_list).rwsem){++++.+}, at: [] __blocking_notifier_call_chain+0x34/0x68which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 ((cpufreq_policy_notifier_list).rwsem){++++.+}:
[] down_write+0x44/0x9c
[] blocking_notifier_chain_register+0x28/0xd8
[] cpufreq_register_notifier+0x68/0x90
[] __cpufreq_cooling_register.part.1+0x120/0x180
[] __cpufreq_cooling_register+0x98/0xa4
[] cpufreq_cooling_register+0x18/0x1c
[] imx_thermal_probe+0x1c0/0x470 [imx_thermal]
[] platform_drv_probe+0x50/0xac
[] driver_probe_device+0x114/0x234
[] __driver_attach+0x9c/0xa0
[] bus_for_each_dev+0x5c/0x90
[] driver_attach+0x24/0x28
[] bus_add_driver+0xe0/0x1d8
[] driver_register+0x80/0xfc
[] __platform_driver_register+0x50/0x64
[] 0xbf007018
[] do_one_initcall+0x88/0x1d8
[] load_module+0x1768/0x1ef8
[] SyS_init_module+0xe0/0xf4
[] ret_fast_syscall+0x0/0x48-> #0 (cooling_cpufreq_lock){+.+.+.}:
[] lock_acquire+0xb0/0x124
[] mutex_lock_nested+0x5c/0x3d8
[] cpufreq_thermal_notifier+0x34/0xfc
[] notifier_call_chain+0x4c/0x8c
[] __blocking_notifier_call_chain+0x50/0x68
[] blocking_notifier_call_chain+0x20/0x28
[] cpufreq_set_policy+0x7c/0x1d0
[] store_scaling_governor+0x74/0x9c
[] store+0x90/0xc0
[] sysfs_kf_write+0x54/0x58
[] kernfs_fop_write+0xdc/0x190
[] vfs_write+0xac/0x1b4
[] SyS_write+0x44/0x90
[] ret_fast_syscall+0x0/0x48other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock((cpufreq_policy_notifier_list).rwsem);
lock(cooling_cpufreq_lock);
lock((cpufreq_policy_notifier_list).rwsem);
lock(cooling_cpufreq_lock);*** DEADLOCK ***
7 locks held by rc.local/770:
#0: (sb_writers#6){.+.+.+}, at: [] vfs_write+0x18c/0x1b4
#1: (&of->mutex){+.+.+.}, at: [] kernfs_fop_write+0xa0/0x190
#2: (s_active#52){.+.+.+}, at: [] kernfs_fop_write+0xa8/0x190
#3: (cpu_hotplug.lock){++++++}, at: [] get_online_cpus+0x34/0x90
#4: (cpufreq_rwsem){.+.+.+}, at: [] store+0x58/0xc0
#5: (&policy->rwsem){+.+.+.}, at: [] store+0x70/0xc0
#6: ((cpufreq_policy_notifier_list).rwsem){++++.+}, at: [] __blocking_notifier_call_chain+0x34/0x68stack backtrace:
CPU: 0 PID: 770 Comm: rc.local Not tainted 3.18.0+ #1453
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
r6:c0b85a80 r5:c0b75630 r4:00000000 r3:00000000
[] (show_stack) from [] (dump_stack+0x7c/0x98)
[] (dump_stack) from [] (print_circular_bug+0x28c/0x2d8)
r4:c0b85a80 r3:d0071d40
[] (print_circular_bug) from [] (__lock_acquire+0x1acc/0x1bb0)
r10:c0b50660 r8:c09e6d80 r7:d0071d40 r6:c11d0f0c r5:00000007 r4:d0072240
[] (__lock_acquire) from [] (lock_acquire+0xb0/0x124)
r10:00000000 r9:c04abfc4 r8:00000000 r7:00000000 r6:00000000 r5:c0a06f0c
r4:00000000
[] (lock_acquire) from [] (mutex_lock_nested+0x5c/0x3d8)
r10:ec853800 r9:c0a06ed4 r8:d0071d40 r7:c0a06ed4 r6:c11d0f0c r5:00000000
r4:c04abfc4
[] (mutex_lock_nested) from [] (cpufreq_thermal_notifier+0x34/0xfc)
r10:ec853800 r9:ec85380c r8:d00d7d3c r7:c0a06ed4 r6:d00d7d3c r5:00000000
r4:fffffffe
[] (cpufreq_thermal_notifier) from [] (notifier_call_chain+0x4c/0x8c)
r7:00000000 r6:00000000 r5:00000000 r4:fffffffe
[] (notifier_call_chain) from [] (__blocking_notifier_call_chain+0x50/0x68)
r8:c0a072a4 r7:00000000 r6:d00d7d3c r5:ffffffff r4:c0a06fc8 r3:ffffffff
[] (__blocking_notifier_call_chain) from [] (blocking_notifier_call_chain+0x20/0x28)
r7:ec98b540 r6:c13ebc80 r5:ed76e600 r4:d00d7d3c
[] (blocking_notifier_call_chain) from [] (cpufreq_set_policy+0x7c/0x1d0)
[] (cpufreq_set_policy) from [] (store_scaling_governor+0x74/0x9c)
r7:ec98b540 r6:0000000c r5:ec98b540 r4:ed76e600
[] (store_scaling_governor) from [] (store+0x90/0xc0)
r6:0000000c r5:ed76e6d4 r4:ed76e600
[] (store) from [] (sysfs_kf_write+0x54/0x58)
r8:0000000c r7:d00d7f78 r6:ec98b540 r5:0000000c r4:ec853800 r3:0000000c
[] (sysfs_kf_write) from [] (kernfs_fop_write+0xdc/0x190)
r6:ec98b540 r5:00000000 r4:00000000 r3:c0175330
[] (kernfs_fop_write) from [] (vfs_write+0xac/0x1b4)
r10:0162aa70 r9:d00d6000 r8:0000000c r7:d00d7f78 r6:0162aa70 r5:0000000c
r4:eccde500
[] (vfs_write) from [] (SyS_write+0x44/0x90)
r10:0162aa70 r8:0000000c r7:eccde500 r6:eccde500 r5:00000000 r4:00000000
[] (SyS_write) from [] (ret_fast_syscall+0x0/0x48)
r10:00000000 r8:c000edc4 r7:00000004 r6:000216cc r5:0000000c r4:0162aa70Solve this by moving to finer grained locking - use one mutex to protect
the cpufreq_dev_list as a whole, and a separate lock to ensure correct
ordering of cpufreq notifier registration and removal.cooling_list_lock is taken within cooling_cpufreq_lock on
(un)registration to preserve the behavior of the code, i.e. to
atomically add/remove to the list and (un)register the notifier.Fixes: 2dcd851fe4b4 ("thermal: cpu_cooling: Update always cpufreq policy with
Reviewed-by: Viresh Kumar
Signed-off-by: Russell King
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin
05 May, 2015
5 commits
-
Ensure that the CPU for which the frequency is being requested
is online. If none of the CPUs are online the requested power is
returned as 0.Acked-by: Javi Merino
Signed-off-by: Kapileshwar Singh
Signed-off-by: Eduardo Valentin -
It was initially understood that an update to the cpu_device
(cached in cpufreq_cooling_device) was required to ascertain the
correct operating point of the device on a cpufreq policy->cpu update
or creation or deletion of a cpufreq policy.
(e.g. when the existing policy CPU goes offline).This update is not required and it is possible to ascertain the OPPs
from the leading CPU in a cpufreq domain even if the CPU is hotplugged out.Fixes: e0128d8ab423 ("thermal: cpu_cooling: implement the power cooling device API")
Acked-by: Javi Merino
Signed-off-by: Kapileshwar Singh
Signed-off-by: Eduardo Valentin -
We allocate the power_table in memory but we don't test whether the
allocation succeeded. Return -ENOMEM if kcalloc() fails.Fixes: e0128d8ab423 ("thermal: cpu_cooling: implement the power cooling device API")
Cc: Eduardo Valentin
Cc: Zhang Rui
Reported-by: kbuild test robot
Signed-off-by: Javi Merino
Signed-off-by: Eduardo Valentin -
Add trace events for the power allocator governor and the power actor
interface of the cpu cooling device.Cc: Zhang Rui
Cc: Eduardo Valentin
Cc: Frederic Weisbecker
Cc: Ingo Molnar
Acked-by: Steven Rostedt
Signed-off-by: Javi Merino
Signed-off-by: Eduardo Valentin -
Add a basic power model to the cpu cooling device to implement the
power cooling device API. The power model uses the current frequency,
current load and OPPs for the power calculations. The cpus must have
registered their OPPs using the OPP library.Cc: Zhang Rui
Cc: Eduardo Valentin
Signed-off-by: Kapileshwar Singh
Signed-off-by: Punit Agrawal
Signed-off-by: Javi Merino
Signed-off-by: Eduardo Valentin
24 Dec, 2014
1 commit
21 Dec, 2014
1 commit
-
The node field of struct cpufreq_cooling_device was reintroduced in
2dcd851fe4b4 (thermal: cpu_cooling: Update always cpufreq policy with
thermal constraints) but without the documentation that it once had.
Add it back so that all the fields of struct cpufreq_cooling_device
are documented.Cc: Yadwinder Singh Brar
Cc: Eduardo Valentin
Cc: Zhang Rui
Signed-off-by: Javi Merino
Signed-off-by: Zhang Rui
17 Dec, 2014
1 commit
-
There was a left over return here so the error handling isn't run.
It leads to a small memory leak and a static checker warning.drivers/thermal/cpu_cooling.c:351 __cpufreq_cooling_register()
info: ignoring unreachable code.Fixes: f6859014c7e7 ("thermal: cpu_cooling: Store frequencies in descending order")
Acked-by: Viresh Kumar
Signed-off-by: Dan Carpenter
Signed-off-by: Eduardo Valentin
09 Dec, 2014
22 commits
-
Adding my copyright information for two purposes:
- To get cc'd for future patches to review (Only if people read this header
while sending mail)
- Have done enough changes to earn a place here?Cc: Amit Daniel Kachhap
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
get_property() was an over complicated beast with BUGs. It used to believe that
cpufreq table is present in ascending or descending order, which might not
always be true.Previous patch has created another freq table in descending order for us and we
better use it now. With that get_property() simply goes away and another helper
get_level() comes in.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
CPUFreq framework *doesn't* guarantee that frequencies present in cpufreq table
will be in ascending or descending order. But cpu_cooling somehow assumes that.Probably because most of current users are creating this list from DT, which is
done with the help of OPP layer. And OPP layer creates the list in ascending
order of frequencies.But cpu_cooling can be used for other platforms too, which don't have
frequencies arranged in any order.This patch tries to fix this issue by creating another list of valid frequencies
in descending order. Care is also taken to throw warnings for duplicate entries.Later patches would use it to simplify code.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
We already know the value of 'cpufreq_dev->max_level' and so there is no need
calculating that once again. For this, we need to send 'cpufreq_dev' to
get_property().Make all necessary changes for this change. Because cpufreq_cooling_get_level()
doesn't have access to 'cpufreq_dev', it is updated to iterate over the list of
cpufreq_cooling_devices to get cooling device for the cpu number passed to it.
This also makes it robust to return levels only for the CPU registered via a
cooling device. We don't have to support anything that isn't registered yet.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
As we already have a list of cpufreq_cooling_devices now, lets use it instead of
a local counter.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
We don't use get_property() to find max levels anymore as it is done at boot
now. So, don't support GET_MAXL in get_property().Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
CPU frequency tables don't update after the driver is registered and so we don't
need to iterate over them to find total number of states every time
cpufreq_get_max_state() is called. Do it once at boot time.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
get_cpu_frequency() isn't doing much by itself, just calling get_property(). And
so this wrapper isn't required at all. Get rid of it.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
cpufreq_apply_cooling() has a single caller, cpufreq_set_cur_state() and
cpufreq_set_cur_state() is an unnecessary wrapper over cpufreq_apply_cooling().Get rid of it by merging both routines.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
There is no point checking for validity of 'cpufreq_val' from
cpufreq_thermal_notifier() every time the routine is called. Its guaranteed to
be 0 on the first call but will be valid otherwise.Lets update it once while the device registers.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
This makes life easy and bug free. And is scalable for future resource
allocations.Acked-by: Javi Merino
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
Because get_cpu_frequency() has returned a valid frequency, it means that the
cpufreq policy is surely valid and so no point checking that again with
is_cpufreq_valid(). Get rid of the routine as well as there are no more users.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
All CPUs present in 'allowed_cpus' share the same 'struct cpufreq_policy'
structure and so calling cpufreq_update_policy() for each of them doesn't make
sense.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
In __cpufreq_cooling_register() we try to match min/max frequencies for all CPUs
passed in 'clip_cpus' mask. This mask is the cpumask of cpus where the frequency
constraints will be applied.Same frequency constraint can be applied only to the CPUs belonging to the same
cluster (i.e. CPUs sharing clock line). For all such CPUs we have a single
'struct cpufreq_policy' structure managing them and so getting policies for all
CPUs wouldn't make any sense as they will all return the same pointer.So, remove this useless check of checking min/max for all CPUs. Also update doc
comment to make this more obvious that clip_cpus should be same as
policy->related_cpus.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
We aren't supposed to return our own error type here. Return what we got.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
ret is initialized before it is used, so no need to set it to 0 in its declaration.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
It will be overwritten soon with return value of kzalloc().
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
Its already zero, we allocated cpufreq_dev with kzalloc.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
Just following coding guidelines here.
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
This wasn't explained well anywhere and should be clearly specified. Lets add a
top level comment for this.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
cooling_cpufreq_lock isn't used to protect this structure and so the comment
over it is outdated. Fix it.Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin -
s/give/given
Acked-by: Eduardo Valentin
Signed-off-by: Viresh Kumar
Signed-off-by: Eduardo Valentin