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

From: Doug Anderson
Date: Tue Mar 03 2015 - 18:21:30 EST


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/