LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Mark Brown <broonie@kernel.org>
Cc: milo.kim@ti.com, Axel Lin <axel.lin@ingics.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Olof Johansson <olof@lixom.net>,
	Javier Martinez Canillas <javier.martinez@collabora.co.uk>,
	Paul Stewart <pstew@chromium.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] regulator: core: Fix enable GPIO reference counting
Date: Mon, 2 Mar 2015 13:13:56 -0800	[thread overview]
Message-ID: <CAD=FV=VDC_UbU43xWniTnqaN8Hz+1iYmwMLaAE4j3=qSq-jp4A@mail.gmail.com> (raw)
In-Reply-To: <20150302184722.GE21293@sirena.org.uk>

Mark,

On Mon, Mar 2, 2015 at 10:47 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Feb 27, 2015 at 11:41:03AM -0800, Doug Anderson wrote:
>> It is possible for _regulator_do_enable() to be called for an
>> already-enabled rdev, like in regulator_suspend_finish().  If we were
>> using an enable pin (rdev->ena_pin is set) then we'd end up
>> incrementing the reference count in regulator_ena_gpio_ctrl() over and
>> over again without a decrement.  That prevented the GPIO from going to
>> the "off" state even after all users were disabled.
>
>> Fix this by avoiding the call to regulator_ena_gpio_ctrl() when it's
>> not needed.
>
> There's a big jump in this changelog where you assert that we're
> avoiding the call "when it's not needed" without explaining the
> situations in which this is the case or why.
>
> Looking at the code it seems that you're adding checks to skip calls in
> the standard enable and disable paths but not touching other paths,
> based on this patch by itself I can't tell if this is a good idea or
> not.  It certainly doesn't feel robust - if we're missing reference
> counting skipping operations seems likely to lead to other bugs popping
> up elsewhere when the other user that isn't doing a disable currently
> decides to start doing so.

I guess it depends on whether _regulator_do_enable() on an
already-enabled rdev is supposed to be a noop or not.  My assumption
was that it was supposed to be a noop with reference counting handled
by _regulator_enable().

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.


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.

-Doug

  reply	other threads:[~2015-03-02 21:14 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-27 19:41 Doug Anderson
2015-02-27 20:08 ` Greg KH
2015-02-27 21:01 ` Javier Martinez Canillas
2015-03-02 18:57   ` Mark Brown
2015-03-02 20:21     ` Javier Martinez Canillas
2015-03-03 14:24       ` Mark Brown
2015-03-02 18:47 ` Mark Brown
2015-03-02 21:13   ` Doug Anderson [this message]
2015-03-03 14:23     ` Mark Brown
2015-03-03 23:21       ` Doug Anderson
2015-03-04 11:27         ` Mark Brown
2015-03-04 17:13           ` Doug Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAD=FV=VDC_UbU43xWniTnqaN8Hz+1iYmwMLaAE4j3=qSq-jp4A@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=axel.lin@ingics.com \
    --cc=broonie@kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=javier.martinez@collabora.co.uk \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=milo.kim@ti.com \
    --cc=olof@lixom.net \
    --cc=pstew@chromium.org \
    --cc=stable@vger.kernel.org \
    --subject='Re: [PATCH] regulator: core: Fix enable GPIO reference counting' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).