Re: [RFC PATCH] regulator: introduce boot protection flag

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]

On Mon, May 09, 2016 at 03:05:08PM +0800, WEN Pingbo wrote:

> In some platforms, critical shared regulator is initialized in
> bootloader. But during kernel booting, the driver probing order and
> conflicting operations from other regulator consumers, may set the
> regulator in a undefined state, which will cause serious problem.

> This patch try to add a boot_protection flag in regulator constraints.

This still feels like a short term hack that doesn't belong in an ABI.
It's all very implementation specific and not very robust, it's not
describing the outcome we're looking for but rather a very specific
behaviour which won't work outside of a fairly narrow system
configuration.  The difficulty in coherently explaining what the end of
boot is and what protection means is a big warning sign here.

I think you need to be looking at some combination of getting the
devices you're interested in started up early and more precisely
describing the end result you're trying to achieve.  The issues with
probe deferral do seem related here, it's another symptom of not really
making any decisions about init ordering and so sometimes making bad
ones.

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

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

> +	/* 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]