[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 3 Mar 2015 15:21:21 -0800
From: Doug Anderson <dianders@...omium.org>
To: Mark Brown <broonie@...nel.org>
Cc: milo.kim@...com, Axel Lin <axel.lin@...ics.com>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Olof Johansson <olof@...om.net>,
Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
Paul Stewart <pstew@...omium.org>,
"stable@...r.kernel.org" <stable@...r.kernel.org>,
Liam Girdwood <lgirdwood@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regulator: core: Fix enable GPIO reference counting
Hi,
On Tue, Mar 3, 2015 at 6:23 AM, Mark Brown <broonie@...nel.org> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists