Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Doug Anderson]

Hi,

On Tue, Mar 3, 2015 at 6:23 AM, Mark Brown <broonie@xxxxxxxxxx> wrote:
>> 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.
>
>> 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.
>
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/