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]

Message-ID: <20131217130007.GZ3185@sirena.org.uk>
Date:	Tue, 17 Dec 2013 13:00:07 +0000
From:	Mark Brown <broonie@...nel.org>
To:	"Anson.Huang@...escale.com" <Anson.Huang@...escale.com>
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

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.

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

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)