Hi,

On Tue, Mar 3, 2015 at 6:23 AM, Mark Brown [off-list ref] wrote:

quoted

My assumption is that regulator drivers themselves shouldn't do
reference counting.  That is: if you call
rdev->desc->ops->enable(rdev) twice you should not have to call
rdev->desc->ops->disable(rdev) twice to disable.  Right?  That means
my fix is making the "ena_pin" symmetric to how normal regulator
drivers work.

quoted

The refcounting being skipped by my patch is refcounting that's used
only when the same GPIO is shared by more than one regulator.  That
is, if "vcc_a" uses GPIO1 and "vcc_b" also uses "GPIO1" we need
refcounting.  GPIO1 will be in the "on" state if either vcc_a or vcc_b
is on.  The problem came in because _regulator_do_enable() was
incrementing the shared refcount every time it was called even if the
specific regulator was already on.

This is all analysis which should have been in the changelog...
possibly not quite so verbosely but it should be there.

quoted

Anyway, I looked at Javier's patch and it's also fine / reasonable.
...and in fact I would argue that possibly we could take both patches.
Javier's patch eliminates the one known place where
_regulator_do_enable() is called for an already-enabled regulator and
my patch means that if someone else adds a new call we won't end up
back in this same subtle bug.  I'm happy to update the CL desc to make
it more obvious if you'd like.

Yes, the changelog definitely needs to be *much* clearer.  Especially
for things like locking and reference counting the changelog needs to
explain what the fix is and why it's safe, without that working it is a
lot harder to do a review as the reviewer needs to go back and check
that everything has been thought through properly.

OK, so I started working on a nice clean changelog of this.  ...and
then I found a bug.  :(

It looks as if "ena_gpio_state" is not quite what I thought it was and
I think is not actually consistent in the regulator framework itself.
In _regulator_do_enable() and _regulator_do_disable() is clear that
ena_gpio_state is 1 when an "rdev" is enabled and 0 when the "rdev" is
disabled.  That was my assumption.  It's also clear in
_regulator_is_enabled().

...but then I looked in regulator_register().  There you can see that
ena_gpio_state could be set to 1 if you've got an active low GPIO that
is disabled at boot.  That totally throws my logic for a loop.  Also
with my patch the reference counting will be all messed up for active
high / boot on regulators.  :(


I'll fix up my patch to make "ena_gpio_state" just be the state of the
"rdev" and not the true state of the pin.  Without redoing the whole
shared GPIO infrastructure I think this is the best I can do.