| Message ID | 200903141725.35541.david-b@pacbell.net (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Sat, Mar 14, 2009 at 05:25:35PM -0700, David Brownell wrote: > + } else if (ops->is_enabled) { > + /* ... if the bootloader left it on, drivers need a > + * nonzero enable count else it can't be disabled. > + */ > + ret = ops->is_enabled(rdev); > + if (ret > 0) > + rdev->use_count = 1; > + ret = 0; This means that drivers that do balanced enables and disables will never be able to cause the regulator to actually be disabled since there will always be this extra reference count there. Without this patch what'll happen with those drivers is that they'll do an enable then later on when the last one disables its supply the reference count will fall to zero and the regulator will be disabled. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 14 March 2009, Mark Brown wrote: > On Sat, Mar 14, 2009 at 05:25:35PM -0700, David Brownell wrote: > > > + } else if (ops->is_enabled) { > > + /* ... if the bootloader left it on, drivers need a > > + * nonzero enable count else it can't be disabled. > > + */ > > + ret = ops->is_enabled(rdev); > > + if (ret > 0) > > + rdev->use_count = 1; > > + ret = 0; > > This means that drivers that do balanced enables and disables will never > be able to cause the regulator to actually be disabled since there will > always be this extra reference count there. That's already true for every regulator for which the "boot_on" flag was set ... nothing changes. Except that things act the same now regardless of whether Linux or the bootloader enabled the regulator in the first place; win! :) On the other hand, every driver using a regulator for which that flag could have be set (== ALL of them) needs to be able to cope with the regulator having been enabled when the device probe() was called. It's not exactly hard to check if it was enabled, then act accordingly, in the typical "single consumer of the regulator" case. > Without this patch what'll > happen with those drivers is that they'll do an enable then later on > when the last one disables its supply the reference count will fall to > zero and the regulator will be disabled. If they're prepared to work with a regulator enabled at boot time by either the bootloader or (as its proxy) Linux, they'll first look to see if the regulator is enabled. Of course, trying to share a regulator that's set up like that can bring its own joys. This patch doesn't change that issue, but it does get rid of one nasty initialization problem. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Mar 14, 2009 at 09:05:29PM -0700, David Brownell wrote: > was called. It's not exactly hard to check if it was enabled, then > act accordingly, in the typical "single consumer of the regulator" > case. How typical that is depends very much on the devices you're looking at. Devices that need to do things like set voltages are fairly likely to own the regulator but with devices that just need to ensure that they have their supplies enabled it's much more likely that the supplies will be shared. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 16 March 2009, Mark Brown wrote: > On Sat, Mar 14, 2009 at 09:05:29PM -0700, David Brownell wrote: > > > was called. It's not exactly hard to check if it was enabled, then > > act accordingly, in the typical "single consumer of the regulator" > > case. > > How typical that is depends very much on the devices you're looking at. I'd have said "systems you're looking at" ... but so long as the boot_on flag exists, then this issue can appear with absolutely any regulator, used for anything. The basic issue addressed by $SUBJECT patch just restores internal consistency to the framework, in the face of such flags (or equivalent actions by the boot loader). > Devices that need to do things like set voltages are fairly likely to > own the regulator but with devices that just need to ensure that they > have their supplies enabled it's much more likely that the supplies will > be shared. Right. Do you have a model how such shared supplies would coexist with the "enabled at boot time" model, and still support being disabled? My first thought is that the system designer should avoid such boot_on modes. But that's not always going to work. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 17, 2009 at 11:15:06AM -0700, David Brownell wrote: > On Monday 16 March 2009, Mark Brown wrote: > > Devices that need to do things like set voltages are fairly likely to > > own the regulator but with devices that just need to ensure that they > > have their supplies enabled it's much more likely that the supplies will > > be shared. > Right. Do you have a model how such shared supplies would > coexist with the "enabled at boot time" model, and still > support being disabled? The drivers can essentially ignore the physical status of the regulator when they start, enabling when they need them and disabling when they're done. So long as at least one device has the regulator enabled the regulator will remain on but when the reference count drops to zero then the regulator will be disabled. This works well when the driver will be enabling the regulators at startup and then disabling them later on. It will also work well with a late_initcall which disables any unreferenced regulators - that should become the default at some future point (2.6.31 might be a good candiate here, I posted a patch the other day providing an implementation which warns if there are affected regulators and which machines can activate if they want). > My first thought is that the system designer should avoid > such boot_on modes. But that's not always going to work. Yes, that's not something that can realistically be achieved in the general case. Machines should probably avoid it but often people want to do things like bring LCDs up in the bootloader and providing graceful handover to the actual driver helps the user experience. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 17 March 2009, Mark Brown wrote: > On Tue, Mar 17, 2009 at 11:15:06AM -0700, David Brownell wrote: > > On Monday 16 March 2009, Mark Brown wrote: > > > > Devices that need to do things like set voltages are fairly likely to > > > own the regulator but with devices that just need to ensure that they > > > have their supplies enabled it's much more likely that the supplies will > > > be shared. > > > Right. Do you have a model how such shared supplies would > > coexist with the "enabled at boot time" model, and still > > support being disabled? > > The drivers can essentially ignore the physical status of the regulator > when they start, That is, shared supplies should adopt a different model? That approach can't be used with drivers, as for MMC slots, which need to ensure they start with a "power off" state as part of a clean reset/init sequence. Maybe "sharable" should be a regulator constraint flag, so the regulator framework can avoid committing nastiness like allocating multiple consumer handles for them. > It will also work well with a > late_initcall which disables any unreferenced regulators - The $SUBJECT patch will prevent such things from existing. Also, regulator use that kicks in before that particular late_initcall will still see self-inconsistent state in the regulator framework ... of course, $SUBJECT patch (and its predecessors) is all about preventing self-inconsistency. That self-inconsistency doesn't seem to concern you much. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 18, 2009 at 12:25:11PM -0700, David Brownell wrote: > On Tuesday 17 March 2009, Mark Brown wrote: > > The drivers can essentially ignore the physical status of the regulator > > when they start, > That is, shared supplies should adopt a different model? I think that's a bit strong, once we get past init the problem pretty much goes away AFAICT. > That approach can't be used with drivers, as for MMC slots, > which need to ensure they start with a "power off" state as > part of a clean reset/init sequence. Pretty much. They could cope if they used the enable/disable bounce hack but if they urgently need to have a specific power state they won't be able to cope with shared regulators anyway so it doesn't really make any odds. > Maybe "sharable" should be a regulator constraint flag, so > the regulator framework can avoid committing nastiness like > allocating multiple consumer handles for them. Or vice versa - it's as much a property of the consumer driver that it requires exclusivity. From the point of view of the regulator API there's very little difference between the two cases. Note that for well behaved consumers that use mapped supply names we already know about them all in advance anyway... > > It will also work well with a > > late_initcall which disables any unreferenced regulators - > The $SUBJECT patch will prevent such things from existing. I sent a patch backing that specific change out along with the late_initcall() patch. > Also, regulator use that kicks in before that particular > late_initcall will still see self-inconsistent state in > the regulator framework ... of course, $SUBJECT patch (and > its predecessors) is all about preventing self-inconsistency. A driver that can cope with sharing the regulator is not going to be able to observe anything here: it must always enable the regulator when it is using it even if it is already enabled (since otherwise some other consumer could cause it to be turned off) and it should expect that the regulator might be enabled by something else. It can't do an unbalanced disable since otherwise it could be reversing an enable something else did. It's also not possible for it to inspect the use count. If a consumer can't play with that model then I'm reasonably happy with it having to state any additional requirements it has in some way - if nothing else it gives us a bit of documentation about what's going on. > That self-inconsistency doesn't seem to concern you much. I think it's more that I'm viewing the use count as being useful information which the API can take advantage of ("do any consumers actually want this on right now?"). I think we should be able to handle this without changing the use count and that this is preferable because otherwise we make more work with shared consumers, which should be the simplest case. The trick is getting the non-shared regulators into sync with the internal status, ideally without doing things like bouncing supplies during init. I think either using regulator_force_disable() or saying that the consmer wants exclusive access on get and then bumping the use count for it if the regulator is already enabled ought to cover it. I've not thought the latter option through fully, though. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 18 March 2009, Mark Brown wrote: > > The $SUBJECT patch will prevent such things from existing. > > I sent a patch backing that specific change out along with the > late_initcall() patch. Huh? $SUBJECT patch hasn't merged. How could you have backed it out?? http://marc.info/?l=linux-kernel&m=123707714131036&w=2 I didn't see such patches. Could you post URLs so I know specifically what you're referring to? In the future, when you're proposing patches you think address bugs I've reported, please remember to CC me ... -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 18 March 2009, Mark Brown wrote: > > That self-inconsistency doesn't seem to concern you much. > > I think it's more that I'm viewing the use count as being useful > information which the API can take advantage of ("do any consumers > actually want this on right now?"). In that case, I don't understand why you didn't like the previous versions of this patch ... which just forced the regulator off (at init time) if that count was zero. > I think we should be able to handle > this without changing the use count and that this is preferable because > otherwise we make more work with shared consumers, which should be the > simplest case. That's what the earlier versions of this patch did, but you rejected that approach ... patches against both mainline (which is where the bug hurts TODAY), and against regulator-next. Also, I don't see why you'd think a shared consumer would be the "simplest", given all the special rules that you've already noted as only needing to kick in for those cases. > The trick is getting the non-shared regulators into sync with the > internal status, I don't see why that should need any tricks. At init time you have that state and the regulator; and you know there are no consumers. Put it into a self-consistent state at that time ... done. There are really only two ways to make that state self-consistent. And you have rejected both. > ideally without doing things like bouncing supplies > during init. Â I think either using regulator_force_disable() or saying The force_disable() thing looks to me like an API design botch. As you said at one point, it has no users. And since the entire point is to bypass the entire usecount scheme ... it'd be much better to remove it. > that the consmer wants exclusive access on get and then bumping the use > count for it if the regulator is already enabled ought to cover it. > I've not thought the latter option through fully, though. The problem I have with your approach here is that you have rejected quite a few alternative bugfixes applying to a common (and simple!!) configuration, but you haven't provided an alternative that can work for MMC init. I'm really at a loss to see a way out here. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 18, 2009 at 02:14:14PM -0700, David Brownell wrote: > On Wednesday 18 March 2009, Mark Brown wrote: > > I think it's more that I'm viewing the use count as being useful > > information which the API can take advantage of ("do any consumers > > actually want this on right now?"). > In that case, I don't understand why you didn't like > the previous versions of this patch ... which just > forced the regulator off (at init time) if that count > was zero. There are two issues which I raised in response to that patch. One is that this is a fairly substantial interface change which will affect existing constraints without warning and will therefore break people using the current code. At the minute you can't rely on boot_on being set for any regulator which is enabled when Linux starts. This is the most serious issue - a quick survey leads me to expect that turning off regulators without boot_on would break most existing systems somehow. The other is that I'm fairly sure that we'll end up running into systems where the setup at startup is not fixed and forcing the regulator state immediately on regulator registration is likely to cause problems dealing gracefully with such systems. You addressed that by adding devmode, though ideally that should have a clearer name. > > I think we should be able to handle > > this without changing the use count and that this is preferable because > > otherwise we make more work with shared consumers, which should be the > > simplest case. > That's what the earlier versions of this patch did, > but you rejected that approach ... patches against > both mainline (which is where the bug hurts TODAY), > and against regulator-next. I have given you two suggestions which will allow your MMC driver to use mainline without impacting other drivers. I've also provided some suggestions for how we could introduce changes in the regulator core to support this better. > Also, I don't see why you'd think a shared consumer > would be the "simplest", given all the special rules > that you've already noted as only needing to kick in > for those cases. Simplest for the consumers - they just need to do a get followed by basic reference counting, they don't need to (and can't, really) worry about anything else. > > The trick is getting the non-shared regulators into sync with the > > internal status, > I don't see why that should need any tricks. At > init time you have that state and the regulator; > and you know there are no consumers. Put it into Realistically we don't have that information at the minute. For the most part we have the physical state and we may also have some constraint information but we can't rely on the constraint information right now. The fact that we can't rely on the constraint information means that we can't tell if a regulator is enabled because something is using it at the minute or if it's just been left on because that was the default or similar. > a self-consistent state at that time ... done. > There are really only two ways to make that state > self-consistent. And you have rejected both. Both of the approaches you have suggested change the interfaces and cause problems for existing users with no warning to allow them to transition. Changing the reference count does avoid the problems with powering things off but it causes other problems in doing so, ones that look harder to resolve. When looking at bringing the use count in sync with the physical state during startup we have to deal with the fact that we can't currently rely on having information about the desired state of the hardware at the time the regulator is registered. We need to make an API change of some kind to get that information. > > during init. ?I think either using regulator_force_disable() or saying > The force_disable() thing looks to me like an API design > botch. As you said at one point, it has no users. And > since the entire point is to bypass the entire usecount > scheme ... it'd be much better to remove it. As the documentation says it was originally added for error handling cases where there is a danger of hardware damage. In that situation you really do want to power off immediately without reference to any other state. > > that the consmer wants exclusive access on get and then bumping the use > > count for it if the regulator is already enabled ought to cover it. > > I've not thought the latter option through fully, though. > The problem I have with your approach here is that you > have rejected quite a few alternative bugfixes applying > to a common (and simple!!) configuration, but you haven't > provided an alternative that can work for MMC init. I have given you a number of suggestions which should work for MMC init. These are: - Have the MMC style consumers use regulator_force_disable() to disable an enabled regulator on startup. - Have the MMC style consumers do an enable()/disable() pair to disable an enabled regulator on startup. - Changing the way the new behaviours are specified so that they don't change the behaviour of existing constraints (eg, by adding a new constraint flag or by having consumers request exclusive access). - Providing a transition method to warn about a pending change in behaviour, ideally along with a way to request that behaviour immediately (which could be deprecated and removed once the default is changed). The first two should work with current mainline and continue to work if either of the other two is implemented, though they could be removed after that has happened. The biggest reason for the pushback here is that everything you have posted so far changes the way the interface works for existing users. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 18, 2009 at 02:02:14PM -0700, David Brownell wrote: > Huh? $SUBJECT patch hasn't merged. How could you > have backed it out?? Sorry - another patch introducing a version of the same issue. Your patch: http://git.kernel.org/?p=linux/kernel/git/lrg/voltage-2.6.git;a=commit;h=67130ef073299cc7299d3ffa344122959e97753c bumped the refcount for boot_on regulators. This patch: http://git.kernel.org/?p=linux/kernel/git/lrg/voltage-2.6.git;a=commit;h=504e7d7e42fb3a58809946770ff5e07cc9d52dbf backs out that change only. This was my bad on the review. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -815,6 +815,14 @@ static int set_machine_constraints(struc goto out; } rdev->use_count = 1; + } else if (ops->is_enabled) { + /* ... if the bootloader left it on, drivers need a + * nonzero enable count else it can't be disabled. + */ + ret = ops->is_enabled(rdev); + if (ret > 0) + rdev->use_count = 1; + ret = 0; } print_constraints(rdev);