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 stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html