linux-kernel - Re: [PATCH] regulator: anatop: add is

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:	Tue, 17 Dec 2013 13:10:12 +0000
From:	"Anson.Huang@...escale.com" <Anson.Huang@...escale.com>
To:	Mark Brown <broonie@...nel.org>
CC:	"lgirdwood@...il.com" <lgirdwood@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regulator: anatop: add is_enabled interface



Sent from Anson's iPhone

> 在 2013年12月17日，21:00，"Mark Brown" <broonie@...nel.org> 写道：
> 
> On Tue, Dec 17, 2013 at 12:38:33PM +0000, Anson.Huang@...escale.com wrote:
> 
>>> better to add the error checking there wouldn't it?
> 
>> Okay, than what about other functions? there is such condition check
>> there is other functions too, that is why I add it here. if you think
>> it is no necessary, I will remove this check in my patch?
> 
> It shouldn't be in the other functions either.

got it, I will remove it later.


> 
>>> This sounds like you need to have some higher level synchronisation
>>> between whatever is managing this supply and your cpufreq driver - if
>>> you rely on reading back the current status from the hardware there will
>>> always be races between reading the state and the other thing doing the
>>> enable or disable.
> 
>> yes, you are right. but I think we have handled that, all the
>> operations of this LDO will via regulator interface, and regulator
>> framework already has mutex lock. so there should be no such race in
>> kernel as long as we all use regulator interface to access anatop LDO.
> 
> No, that's not going to work.  Consider this sequence:
> 
>    cpufreq        other driver
>            disable()
>    is_enabled()
>            enable()
> 
> The locking the regulator core does won't help you here, nothing stops
> the state of the regulator changing after it's read.  Your cpufreq
> driver should just change the voltage and not worry if the regulator is
> enabled, the voltage change won't have any effect while the regulator is
> off and presumably if it does get enabled then it needs to be enabled at
> whatever voltage cpufreq set anyway.
> 
> In any case I'd be much happier with this patch if it implemented the
> enable and disable operations as well.

understand now. then maybe I should remove the PU check in cpufreq, although setting PU LDO if it is off before would bring unnessary power leak. I will add all these changes together with the dynamic PU LDO management feature, thanks for your time!