From: mtitinger@baylibre•com (Marc Titinger)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH RFC 06/27] PM / Domains: add debugfs 'states' and 'timings' seq files
Date: Wed, 16 Dec 2015 15:12:09 +0100 [thread overview]
Message-ID: <56717139.8050204@baylibre.com> (raw)
In-Reply-To: <CAPDyKFqPn5StedOgd-toxKvvcM078NuVvN8yUDByRZSe9HvKbQ@mail.gmail.com>
On 16/12/2015 13:48, Ulf Hansson wrote:
> [...]
>
>>>
>>> A general comment. Static functions in genpd shall start with one of
>>> the following prefix.
>>>
>>> genpd_*
>>> _genpd_*
>>> __genpd_*
>>>
>>> Please change accordingly.
>>
>>
>> Many static routines were already prefixed like the exported functions with
>> "pm_", shall I make a separate patch for this renaming ?
>
> My point is that I don't want it to becomes worse.
>
> If you follow the above rule for new changes, I am happy.
>
> Whether you want to send a separate patch fixing the other existing
> name to be consistent with above rule, I would also be happy. :-)
Fair enough, I'll do a renaming patch :)
>
>>
>>
>>>
>>>> ---
>>>> drivers/base/power/domain.c | 115
>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 107 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>>> index c300293..9a0df09 100644
>>>> --- a/drivers/base/power/domain.c
>>>> +++ b/drivers/base/power/domain.c
>>>> @@ -2169,21 +2169,120 @@ static const struct file_operations
>>>> pm_genpd_summary_fops = {
>>>> .release = single_release,
>>>> };
>>>>
>>>> +static int pm_genpd_states_show(struct seq_file *s, void *data)
>>>> +{
>>>> + struct generic_pm_domain *genpd;
>>>> +
>>>> + seq_puts(s,
>>>> + "\n Domain State name Enter + Exit =
>>>> Min_off_on (ns)\n");
>>>> + seq_puts(s,
>>>> +
>>>> "-----------------------------------------------------------------------\n");
>>>> +
>>>
>>>
>>> You must hold the gpd_list_lock while traversing the gpd list.
>>>
>>>> + list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
>>>> +
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < genpd->state_count; i++) {
>>>
>>>
>>> To be sure you have valid data, you should hold the genpd lock here.
>>>
>>
>> At some point while testing with calling suspend from the power_off handler,
>> the cpu would go to sleep with the lock held, hence using this seq-file
>> would not work.
>
> That seems like a bad behaviour during suspend. Why does it hold the lock?
that was in the scenario where 2 or more cpus are devices of the same
power domain. IIRC you would have something like:
arm_enter_idle_state(cpu)
pm_runtime_put_sync
rpm_suspend
__rpm_get_callback
pm_genpd_runtime_suspend
[Take lock]
->platform code to enter sleep...
and race condition with another cpu of the same cluster trying to
suspend or resume at the same time. it is a bad behaviour, I cannot test
the os-initiated mode here but I assume this is done differently and is
no longer an issue (sorry for not being more specific).
>
> On the other it shouldn't matter as userspace can't access the debugfs
> nodes, since its frozen at those times, right!?
you can have cpu_j off, and cpu_i running the shell, in the scenario
above. But since this was while hacking calling psci from
genpd.power_off, I'm not sure it's worth mentioning...
>
>>
>> while I agree, I think it is not super likely that a
>> domain/child/devices/states are added or removed at this point (the DT is
>> parsed already), would using list_for_each_entry_safe be safe enough ?
>
> No it's not.
>
> The gpd_list is protected with the gdp_list_lock, which is needed
> because new genpds can be added at any point.
>
> You also need the genpd lock here, as otherwise you may print the
> latency-values in the middle of when these are being updated.
Understood, I'll put the lock back.
>
>>
>>
>>>> + seq_printf(s, "%-20s %-20s %lld+%lld=%lld\n",
>>>> + genpd->name,
>>>> + genpd->states[i].name,
>>>> + genpd->states[i].power_on_latency_ns,
>>>> + genpd->states[i].power_off_latency_ns,
>>>> + genpd->states[i].power_off_latency_ns
>>>> + +
>>>> genpd->states[i].power_on_latency_ns);
>>>> + }
>>>> +
>>>> + }
>>>> +
>>>> + seq_puts(s, "\n");
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>
> [...]
>
> Kind regards
> Uffe
>
next prev parent reply other threads:[~2015-12-16 14:12 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-17 22:37 [PATCH RFC 00/27] PM/Domains: Cluster idle support for ARM SoCs Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 01/27] PM / Domains: core changes for multiple states Lina Iyer
2015-12-09 13:58 ` Ulf Hansson
2015-12-17 17:58 ` Axel Haslam
2015-12-17 21:19 ` Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 02/27] PM / Domains: Allow domain power states to be read from DT Lina Iyer
2015-12-10 16:53 ` Ulf Hansson
2015-12-15 10:07 ` Marc Titinger
2015-12-15 22:14 ` Lina Iyer
2015-12-16 21:36 ` Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 03/27] PM / Domain: Add additional state specific param Lina Iyer
2015-11-19 21:33 ` Kevin Hilman
2015-11-17 22:37 ` [PATCH RFC 04/27] PM / Domains: make governor select deepest state Lina Iyer
2015-12-11 9:13 ` Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 05/27] PM / Domains: remove old power on/off latencies Lina Iyer
2015-11-18 14:57 ` [PATCH] ARM: imx6: pm: declare pm domain latency on power_state struct Lina Iyer
2015-11-23 13:31 ` Lucas Stach
2015-11-23 13:42 ` Lucas Stach
2015-12-04 23:19 ` Lina Iyer
2015-12-11 9:16 ` [PATCH RFC 05/27] PM / Domains: remove old power on/off latencies Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 06/27] PM / Domains: add debugfs 'states' and 'timings' seq files Lina Iyer
2015-12-11 11:46 ` Ulf Hansson
2015-12-16 11:07 ` Marc Titinger
2015-12-16 12:48 ` Ulf Hansson
2015-12-16 14:12 ` Marc Titinger [this message]
2015-11-17 22:37 ` [PATCH RFC 07/27] PM / Domains: Read domain residency from DT Lina Iyer
2015-11-24 20:41 ` Stephen Boyd
2015-12-11 11:54 ` Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 08/27] PM / Domains: Support IRQ safe PM domains Lina Iyer
2016-01-14 14:42 ` Ulf Hansson
2016-01-14 18:33 ` Lina Iyer
2016-01-15 8:55 ` Ulf Hansson
2016-01-15 16:57 ` Lina Iyer
2016-01-15 22:08 ` Ulf Hansson
2016-01-18 16:58 ` Lina Iyer
2016-01-18 17:00 ` Lina Iyer
2016-01-19 10:01 ` Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 09/27] PM / Domains: Attempt runtime suspend of IRQ safe parent domain Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 10/27] drivers: power: Introduce PM domains for CPUs/clusters Lina Iyer
2015-11-24 20:52 ` Stephen Boyd
2015-11-17 22:37 ` [PATCH RFC 11/27] drivers: cpu: Define CPU devices as IRQ safe Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 12/27] ARM: cpuidle: remove cpu parameter from the cpuidle_ops suspend hook Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 13/27] ARM: cpuidle: Add runtime PM support for CPU idle Lina Iyer
2015-11-18 8:50 ` Zhaoyang Huang
2015-11-18 14:17 ` Lina Iyer
2015-11-19 22:10 ` Kevin Hilman
2015-11-17 22:37 ` [PATCH RFC 14/27] tick: get next wakeup event for the CPU Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 15/27] PM / Domains: Add next_wakeup to device's timing data Lina Iyer
2015-11-19 22:19 ` Kevin Hilman
2015-11-20 15:58 ` Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 16/27] ARM: cpuidle: Record the next wakeup event of the CPU Lina Iyer
2015-11-19 23:35 ` Kevin Hilman
2015-11-20 16:28 ` Lina Iyer
2015-11-24 18:29 ` Kevin Hilman
2015-11-17 22:37 ` [PATCH RFC 17/27] drivers: cpu-pd: Record CPUs that are part of the domain Lina Iyer
2015-11-24 21:00 ` Stephen Boyd
2015-11-25 14:13 ` Lina Iyer
2015-11-25 19:12 ` Stephen Boyd
2015-11-25 20:20 ` Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 18/27] drivers: cpu-pd: Add PM Domain governor for CPUs Lina Iyer
2015-11-18 18:42 ` Lorenzo Pieralisi
2015-11-19 8:50 ` Marc Titinger
2015-11-20 17:39 ` Lina Iyer
2015-11-19 23:52 ` Kevin Hilman
2015-11-20 16:21 ` Lorenzo Pieralisi
2015-11-20 16:42 ` Lina Iyer
2015-11-20 0:03 ` Kevin Hilman
2015-11-17 22:37 ` [PATCH RFC 19/27] drivers: cpu-pd: Invoke CPU PM runtime on hotplug Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 20/27] Documentation: ARM: topology: 'cluster' property for cluster nodes Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 21/27] drivers: cpu-pd: Parse topology to setup CPU PM domains Lina Iyer
2015-12-07 14:54 ` Lorenzo Pieralisi
2015-12-08 18:05 ` Lina Iyer
2015-12-10 18:11 ` Lorenzo Pieralisi
2015-12-11 9:04 ` Geert Uytterhoeven
2015-12-11 20:51 ` Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 22/27] drivers: firmware: PSCI: Export psci_has_ext_power_state() Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 23/27] ARM64: psci: Support cluster idle states for OS-Initated Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 24/27] arm64: dts: Add Qualcomm MSM8916, MTP8916, APQ8016, SBC8016 ids Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 25/27] devicetree: bindings: Document qcom, msm-id and qcom, board-id Lina Iyer
2015-11-19 14:36 ` Rob Herring
2015-11-19 15:36 ` [PATCH RFC 25/27] devicetree: bindings: Document qcom, msm-id and qcom,board-id Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 26/27] ARM64: dts: Add PSCI cpuidle support for MSM8916 Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 27/27] ARM64: dts: Define CPU power domain " Lina Iyer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56717139.8050204@baylibre.com \
--to=mtitinger@baylibre$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox