linux-kernel - Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM

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]

Date:	Wed, 20 Nov 2013 11:53:11 -0800
From:	Stephen Boyd <sboyd@...eaurora.org>
To:	Grygorii Strashko <grygorii.strashko@...com>,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	santosh.shilimkar@...com, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH] PM / Clocks: fix pm_clk_resume/suspend if CONFIG_PM_RUNTIME
 is set

On 11/20/13 11:06, Grygorii Strashko wrote:
> On 11/20/2013 08:42 PM, Stephen Boyd wrote:
>> On 11/20/13 05:31, Grygorii Strashko wrote:
>>> @@ -230,7 +230,7 @@ int pm_clk_suspend(struct device *dev)
>>>   	list_for_each_entry_reverse(ce, &psd->clock_list, node) {
>>>   		if (ce->status < PCE_STATUS_ERROR) {
>>>   			if (ce->status == PCE_STATUS_ENABLED)
>>> -				clk_disable(ce->clk);
>>> +				clk_disable_unprepare(ce->clk);
>>>   			ce->status = PCE_STATUS_ACQUIRED;
>>>   		}
>>>   	}
>>> @@ -259,7 +259,7 @@ int pm_clk_resume(struct device *dev)
>>>   
>>>   	list_for_each_entry(ce, &psd->clock_list, node) {
>>>   		if (ce->status < PCE_STATUS_ERROR) {
>>> -			clk_enable(ce->clk);
>>> +			clk_prepare_enable(ce->clk);
>>>   			ce->status = PCE_STATUS_ENABLED;
>>>   		}
>>>   	}
>> This is inside a spin_lock_irqsave(). You should be getting scheduling
>> while atomic warnings with this change. Are you testing with
>> DEBUG_ATOMIC_SLEEP=y?
> Ops, thanks. No, It's not tested with DEBUG_ATOMIC_SLEEP and 
> I agree with you.
>
> So, I see two option here:
> 1) split above loops on two
> 2) add calls of clk_prepare()/clk_unprepare() in pm_clk_notify()
>
> In my opinion option [2] is better.
>

Doesn't that mean the clock will always be prepared as long as the
device is present? That doesn't sound good. I would like the clocks to
be disabled and unprepared as long as the device is suspended.

What is the lock protecting? The linked list or something more? Can we
remove the locks?

It looks like even if you just remove the locks here, the PM core is
free to call this function with irqs disabled if pm_runtime_irq_safe()
has been called on the device. Perhaps runtime PM can only do the
clk_enable()/clk_disable() part and the clk_unprepare()/clk_prepare()
calls should happen in the system suspend callbacks?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/