linux-kernel - Re: [RFC PATCH] regulator: introduce boot protection flag

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: <57614479.1060708@linaro.org>
Date:	Wed, 15 Jun 2016 20:05:13 +0800
From:	Pingbo Wen <pingbo.wen@...aro.org>
To:	Mark Brown <broonie@...nel.org>
Cc:	pingbo.wen@...aro.org, linux-kernel@...r.kernel.org,
	lgirdwood@...il.com, vincent.guittot@...aro.org,
	stephen.boyd@...aro.org
Subject: Re: [RFC PATCH] regulator: introduce boot protection flag

Hi, Mark

On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:
> On Mon, May 09, 2016 at 03:05:08PM +0800, WEN Pingbo wrote:
> 
>> And regulator core will postpone all operations until all consumers
>> have taked their place.
> 
> It doesn't, it postpones them until late_initacall().  This is both
> after the consumers have loaded if they are built in and before any
> consumers built as modules come up.

Yes, this patch only protects a regulator from regulator
registration(built in) to late_initcall(). But, IMO, if a regulator is
critical, it's weird to build as a module. Maybe I was thoughtless here.

If we take modules under consideration, and to make this patch more
universal, I think what we really need is adding a flag to protect a
regulator from registration to a specific consumer(not the first
consumer). The regulator driver gives the initial state, and the
specific consumer need to clear this flag while finishing regulator
setting(by calling a function like regulator_clear_protect()). And what
the regulator core need to do is staging all operations during
protection. And that will cover all consumers probing order, whenever
the regulator is registered.

Any idea?

> 
>> The boot_protection flag only work before late_initicall. And as other
>> constraints liked, you can specify this flag in a board file, or in
>> dts file.
> 
> Anything added to the DT ABI needs a binding.
> 

I will add bindings in next version.

>> +	/* constraints check has already done */
>> +	if (rdev->boot_mode)
>> +		rdev->desc->ops->set_mode(rdev, rdev->boot_mode);
> 
> This whole sequence of code ignores errors - that's not great.  We
> should at least log them.
> 

OK.

>> +	mutex_unlock(&rdev->mutex);
>> +
>> +	if (regulator)
>> +		regulator_set_voltage(regulator, regulator->min_uV,
>> +				regulator->max_uV);
> 
> That's...  exciting.  There's a couple of issues here.  One is that
> this is not operating on the rdev but rather on a consumer regulator
> device, the other is that we drop out of the lock before doing the
> update which tends to be a warning sign that something fun is going on
> and at least an internal function should be used.  These two most likely
> come down to the same issue.
> 

OK, some bugs here. I will use a unlock version.

Pingbo