Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 23 Nov 2018 10:57:29 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Linus Walleij <linus.walleij@linaro.org>
CC:     Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        <patches@opensource.cirrus.com>
Subject: Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs
Message-ID: <20181123105729.GM16508@imbe.wolfsonmicro.main>
References: <20181122173015.23905-1-ckeepax@opensource.cirrus.com>
 <20181122173015.23905-3-ckeepax@opensource.cirrus.com>
 <CACRpkdZEu44WHScd5VibYT5C9RikqcvKsgpVOpkkdrYbLaV8Ng@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CACRpkdZEu44WHScd5VibYT5C9RikqcvKsgpVOpkkdrYbLaV8Ng@mail.gmail.com>
User-Agent: Mutt/1.5.20 (2009-12-10)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 23, 2018 at 10:40:57AM +0100, Linus Walleij wrote:
> On Thu, Nov 22, 2018 at 6:30 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> 
> > Currently, a GPIO can be requested multiple times when the
> > NONEXCLUSIVE flag is set, however it must still be freed a single
> > time. This makes client code rather complex, since multiple drivers
> > may request the GPIO but only a single one can free it. Rather than
> > manually handling this in each driver add some basic reference
> > counting into the core.  Currently, this is fairly primitive but
> > so is the support for the NONEXCLUSIVE flag and the implementation
> > covers those use-cases.
> >
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> This patch is not fixing anything right now, correct?
> 

It's fixing something in the case of two regulators using the
same GPIO. The direction this patch chain takes is that the end
drivers own the GPIOs not the regulator core (except for the
legacy case). So both of the end drivers will devm_ request their
GPIOs, which means that if those drivers are unbound both of them
will attempt to free the GPIO causing the same double free
warning we saw here. Indeed there is a probably worse problem
that if one is unbound it will free the GPIO and break the second
driver.

Basically the current handling of non-exclusive GPIOs in the GPIO
core requires multiple calls to request the GPIO but only a
single call to free the GPIO. If we want to stick with that
approach, then the regulator core does need to take ownership of
the lifetime of the enable GPIOs and the end drivers can't use
devm, as per my initial patch for wm8994. But it makes getting
the error paths in regulator_register sensible really tricky, and
feels like a less "clean" solution.

> I discussed the notion of pulling reference counting for
> nonexclusive GPIOs into gpiolib with Mark but the benefit is
> a bit unclear: if the subsystem using nonexeclusive GPIOs
> (currently only regulators) would still have to keep its own
> reference count or somehow semantically know when
> the last user is gone, the point is kind of moot.
> 
> I haven't looked closely at the regulators case but I got
> the impression that it is more complex than just reference
> counting so, currently I don't know if this is such a good
> idea.
> 
> Anyway I would like to push this until we have cleaned
> up with the rest of the series I have boiling, if you don't
> mind.
> 

I don't greatly mind delaying it as I don't currently look after
any systems that share enable GPIOs on regulators, but free of
those GPIOs is currently really broken so it feels like it
shouldn't be kicked too far down the road.

Thanks,
Charles

> (Patch 1+2 should be fine anyway.)
> 
> Yours,
> Linus Walleij