On Fri, Nov 23, 2018 at 10:40:57AM +0100, Linus Walleij wrote: > On Thu, Nov 22, 2018 at 6:30 PM Charles Keepax > <ckee...@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.szyprow...@samsung.com> > > Signed-off-by: Charles Keepax <ckee...@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
