| Message ID | 200903111743.34708.david-b@pacbell.net (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Wed, Mar 11, 2009 at 04:43:34PM -0800, David Brownell wrote: > Buggy consumers could notice different bug symptoms. The main > example would be refcounting bugs; also, any (out-of-tree) users > of the experimental regulator_set_optimum_mode() stuff which > don't call it when they're done using a regulator. I'm OK with this from a code point of view so Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com> However any consumers that take advantage of this won't be able to safely share a regulator without extra work since they have no way of telling why a regulator is in the state that it's in without extra stuff. We should probably have something along the lines of a regulator_get_exclusive() for them. Previously the consumer counting would have stopped them interfering with enables done by other consumers. There will be other consumers that can't safely share a regulator anyway (eg, requiring additional code to notice and handle voltage changes) so it'd be a good thing to have. -- 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, 2009-03-11 at 16:43 -0800, David Brownell wrote: > From: David Brownell <dbrownell@users.sourceforge.net> > > Fix some refcounting issues in the regulator framework, supporting > regulator_disable() for regulators that were enabled at boot time > via machine constraints: > > - Update those regulators' usecounts after enabling, so they > can cleanly be disabled at that level. > > - Remove the problematic per-consumer usecount, so there's > only one level of enable/disable. > > Buggy consumers could notice different bug symptoms. The main > example would be refcounting bugs; also, any (out-of-tree) users > of the experimental regulator_set_optimum_mode() stuff which > don't call it when they're done using a regulator. > > This is a net minor codeshrink. > > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > --- Applied. Thanks Liam -- 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 Thursday 12 March 2009, Mark Brown wrote: > On Wed, Mar 11, 2009 at 04:43:34PM -0800, David Brownell wrote: > > > Buggy consumers could notice different bug symptoms. The main > > example would be refcounting bugs; also, any (out-of-tree) users > > of the experimental regulator_set_optimum_mode() stuff which > > don't call it when they're done using a regulator. > > I'm OK with this from a code point of view so > > Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > > However any consumers that take advantage of this won't be able to > safely share a regulator without extra work since they have no way of > telling why a regulator is in the state that it's in without extra > stuff. Depends what you mean by "safely". If they weren't buggy already, I don't see how they'd notice any difference. Having buggy consumers become non-buggy isn't exactly a job for the framework itself. > We should probably have something along the lines of a > regulator_get_exclusive() for them. Previously the consumer counting > would have stopped them interfering with enables done by other > consumers. I'd like to see get()/put() match the design pattern used elsewhere in the kernel: those calls signify refcount operations. Agreed that the "consumer" access model probably needs a few interface updates. I'm not sure what they would be though; one notion would be to focus on the constraints they apply (including "enabled") instead of what they do now. > There will be other consumers that can't safely share a regulator anyway > (eg, requiring additional code to notice and handle voltage changes) so > it'd be a good thing to have. -- 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 Thu, Mar 12, 2009 at 12:35:24PM -0800, David Brownell wrote: > On Thursday 12 March 2009, Mark Brown wrote: > > safely share a regulator without extra work since they have no way of > > telling why a regulator is in the state that it's in without extra > > stuff. > Depends what you mean by "safely". If they weren't buggy > already, I don't see how they'd notice any difference. > Having buggy consumers become non-buggy isn't exactly a > job for the framework itself. Previously the per-consumer reference count would've meant that they couldn't interfere with other consumers - they could set their own state but not reverse an enable something else had done. Now there is only one reference count but there's no way for a consumer to exclude other consumers and nothing which protects against breakage. > > We should probably have something along the lines of a > > regulator_get_exclusive() for them. Previously the consumer counting > > would have stopped them interfering with enables done by other > > consumers. > I'd like to see get()/put() match the design pattern used > elsewhere in the kernel: those calls signify refcount > operations. Aquiring a reference is obviously what we want in the rather common case where the supply is shared. We could name an operation that enforces non shared supplies something else but at the end of the day it's going to be the same operation. The major purpose of adding an explicit call for this is to document the requirement the consumer has for direct control of what it's doing. > Agreed that the "consumer" access model probably needs a few > interface updates. I'm not sure what they would be though; > one notion would be to focus on the constraints they apply > (including "enabled") instead of what they do now. I'm not at all sure what you mean by this - constraint narrowing by the consumers is pretty much exactly the model the existing code has. We need to do things like re-add the voltage handling that was removed pre merge but that's already the programming model we have. -- 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 Thursday 12 March 2009, Mark Brown wrote: > On Thu, Mar 12, 2009 at 12:35:24PM -0800, David Brownell wrote: > > On Thursday 12 March 2009, Mark Brown wrote: > > > > safely share a regulator without extra work since they have no way of > > > telling why a regulator is in the state that it's in without extra > > > stuff. > > > Depends what you mean by "safely". If they weren't buggy > > already, I don't see how they'd notice any difference. > > Having buggy consumers become non-buggy isn't exactly a > > job for the framework itself. > > Previously the per-consumer reference count would've meant that they > couldn't interfere with other consumers - they could set their own > state but not reverse an enable something else had done. They still can't ... *unless* they're already buggy. > Now there is > only one reference count but there's no way for a consumer to exclude > other consumers and nothing which protects against breakage. > > > > We should probably have something along the lines of a > > > regulator_get_exclusive() for them. Previously the consumer counting > > > would have stopped them interfering with enables done by other > > > consumers. > > > I'd like to see get()/put() match the design pattern used > > elsewhere in the kernel: those calls signify refcount > > operations. > > Aquiring a reference is obviously what we want in the rather common case > where the supply is shared. We could name an operation that enforces > non shared supplies something else but at the end of the day it's going > to be the same operation. The major purpose of adding an explicit call > for this is to document the requirement the consumer has for direct > control of what it's doing. Step back from the current interface for a moment though. There seem to be two things going on: * getting a handle on the regulator; * adding consumer-specific constraints to it. Currently "handle" and "regulator" are two different objects; while "handle" and "consumer-specific constraints" are one. And, as a new thing not currently addressed well in code, you are talking about "non-shared" handles. One could as easily have "handle" and "regulator" be the same ... so the get/put idioms could work like they do elsewhere in the kernel. Doing that would imply making the "constraints" be separate (as they are for machine constraints) and possibly having the ability to control sharing (like IRQF_SHARED does). > > Agreed that the "consumer" access model probably needs a few > > interface updates. I'm not sure what they would be though; > > one notion would be to focus on the constraints they apply > > (including "enabled") instead of what they do now. > > I'm not at all sure what you mean by this - constraint narrowing by the > consumers is pretty much exactly the model the existing code has. We > need to do things like re-add the voltage handling that was removed pre > merge but that's already the programming model we have. See above. Currently constraints are hidden for "consumers", behind functional accessors like regulator_set_voltage(). There are no explicit constraint objects, as there are for the machines. That's all I'm saying: the approached you sketched isn't the only one, and may not be the best one. If you want to change things, there may be better approaches. Even ones that don't change the overall interface structure much. - 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 Thu, Mar 12, 2009 at 03:02:55PM -0800, David Brownell wrote: > On Thursday 12 March 2009, Mark Brown wrote: > > Previously the per-consumer reference count would've meant that they > > couldn't interfere with other consumers - they could set their own > > state but not reverse an enable something else had done. > They still can't ... *unless* they're already buggy. As previously noted they could've worked happily via bouncing their state to match the physical state first; it's not nice or a good idea (which is why I am happy with your patch) but, well, not everyone pays attention to warnings and errors :( > And, as a new thing not currently addressed well in code, you > are talking about "non-shared" handles. You are missing my point here; this is mostly about documentation. The exclusive access issue is with devices that can't tolerate any arbitration and need the regulator to go into the state they're requesting - if the consumer is finding itself doing something like turn off a regulator which it did not enable itself then it's clearly not able to play nicely with other drivers sharing the same resource without extra communication. This may be because the driver is doing things that aren't really appropriate and perhaps ought to be done via constraints if at all; it may also be because the hardware that's being controlled demands it. If everyone is nice and careful about what they're doing it'll not make any difference at all either way. Especially in the hardware requirements case I'd hope it never even comes up that it'd make a difference. > One could as easily have "handle" and "regulator" be the > same ... so the get/put idioms could work like they do > elsewhere in the kernel. I really don't see that there is any meaningful difference here; from the point of view of the consumer the fact that the thing it gets back is a handle to a structure the core uses to keep track of the consumer rather than the underlying hardware object is an implementation detail that shouldn't make any difference to them. In terms of the programming model it seems like a layering violation to know the difference between one opaque structure and another. > See above. Currently constraints are hidden for "consumers", > behind functional accessors like regulator_set_voltage(). > There are no explicit constraint objects, as there are for > the machines. The current interface has been driven by the needs of the users: the majority of consumers want to do one operation on a regular basis - normally that's enable/disable, most devices are just powering themselves up and down, though for some things voltage changes are much more common (DVFS being the prime example). Overall it's been fairly similar to the clock API in terms of usage pattern. In terms of looking at redesigning the API I feel we're getting ahead of ourselves here and should be working more for stability until we run into problems that force us to make big changes. Right now it seems better to focus on improving the support for real systems in mainline and addressing any issues that they see so that we've got something to learn from when doing any redesign and can minimise the amount of integration churn that is created. -- 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 Thursday 12 March 2009, Mark Brown wrote: > On Thu, Mar 12, 2009 at 03:02:55PM -0800, David Brownell wrote: > > > One could as easily have "handle" and "regulator" be the > > same ... so the get/put idioms could work like they do > > elsewhere in the kernel. > > I really don't see that there is any meaningful difference here; from > the point of view of the consumer the fact that the thing it gets back > is a handle to a structure the core uses to keep track of the consumer > rather than the underlying hardware object is an implementation detail > that shouldn't make any difference to them. In terms of the programming > model it seems like a layering violation to know the difference between > one opaque structure and another. You're not stepping back from the current interface, which is a prerequisite to understanding the points I was making: * Almost everywhere else in the kernel, there's only one handle (no per-client facet idiom), for which get/put works. Having the handle alloc/free methods be called get/put is a kind of problem. We want models and idioms to converge, not diverge, in almost all cases ... using the same names to mean different things isn't good. * The thing that *is* per-client is basically a constraint set ... but it's called a "regulator", which again is confusing. In the regulator-next tree you've now moved regulator_dev into the public interface ... that's the handle to the real hardware. Sort of a hint that it can't really be hidden in the way you originally thought. > > See above. Currently constraints are hidden for "consumers", > > behind functional accessors like regulator_set_voltage(). > > There are no explicit constraint objects, as there are for > > the machines. > > The current interface has been driven by the needs of the users: the > majority of consumers want to do one operation on a regular basis - > normally that's enable/disable, most devices are just powering > themselves up and down, though for some things voltage changes are much > more common (DVFS being the prime example). Overall it's been fairly > similar to the clock API in terms of usage pattern. Except that the clock interface uses put/get in the normal way; they are not alloc/free calls, just lookup/refcount calls. > In terms of looking at redesigning the API You were the one suggesting the need for a new call, formalizing a model that didn't previously exist ... not me! :) Which is why I suggested taht if you were going to add calls, it'd be worth thinking a bit more about some existing glitches. -- 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 02:29:24PM -0700, David Brownell wrote: > You're not stepping back from the current interface, which is > a prerequisite to understanding the points I was making: I'm pretty sure I understand exactly what you're saying but we just disagree on this one. Probably best that we agree to disagree. > * Almost everywhere else in the kernel, there's only one > handle (no per-client facet idiom), for which get/put > works. Looking at things from the point of view of the consumer I just don't find that it makes any difference since as far as the consumer is concerned it's all opaque objects manipulated via an API. I certainly don't experience any conceptual jar switching between this and things like the clock API, the patterns in clients the same especially for a basic consumer doing something along the lines of: foo = foo_get(dev, name); foo_enable(foo); foo_disable(foo); foo_put(foo); Even once you start setting more properties in there I'm struggling to see the difference being visible. I feel that you are focusing too much on the implementation here, not the interface, but like I say I think we're just going to have to agree to disagree on this one. > * The thing that *is* per-client is basically a constraint > set ... but it's called a "regulator", which again is > confusing. You could go for regulated_supply or something. I think that at this point it's just not worth the trouble to try to change the name, though. If it helps think of the per client object as representing the particular physical supply to the physical device. We could represent them internally as pass through regulators but since the implementation should still be opaque to the consumers I don't think that it's worth doing that unless it buys us something in the implementation. I'm not overly concerned about the implementation right now since it's not causing any problems and the opacity we have now for the consumers supports later refactoring. Things like the issues with working with struct device for I2C and SPI devices seem to be causing far more pressing problems in actual use. > In the regulator-next tree you've now moved regulator_dev > into the public interface ... that's the handle to the > real hardware. Sort of a hint that it can't really be > hidden in the way you originally thought. It's only exposed to the drivers for the regulator hardware; it's not visible to consumers unless they go peering into the driver API. The drivers for the regulators have always had a direct handle on themselves for obvious reasons - the only change here is that they now know a bit more about the implementation. -- 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: > Looking at things from the point of view of the consumer I just don't > find that it makes any difference since as far as the consumer is > concerned it's all opaque objects manipulated via an API. These put()/get() calls are not refcount calls. They're what might be called alloc()/free() calls instead. > Â Â Â Â Â Â Â Â foo = foo_get(dev, name); The normal idiom is bar = foo_get(foo) which just increments a refcount. Then if "bar" were handed to something else ("baz") right here, and "baz" needed to keep a copy of that reference, it would be expected to grab its own refcount via foo_get(bar), and later release via foo_put(bar). > Â Â Â Â Â Â Â Â foo_enable(foo); > Â Â Â Â Â Â Â Â foo_disable(foo); > Â Â Â Â Â Â Â Â foo_put(foo); Until "baz" called foo_put(bar), that reference would still be usable. But ... regulator_put(foo) does a kfree, it's not just updating refcounts. -- 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 @@ -52,7 +52,6 @@ struct regulator { int uA_load; int min_uV; int max_uV; - int enabled; /* count of client enables */ char *supply_name; struct device_attribute dev_attr; struct regulator_dev *rdev; @@ -811,6 +810,7 @@ static int set_machine_constraints(struc rdev->constraints = NULL; goto out; } + rdev->use_count = 1; } print_constraints(rdev); @@ -1066,10 +1066,6 @@ void regulator_put(struct regulator *reg mutex_lock(®ulator_list_mutex); rdev = regulator->rdev; - if (WARN(regulator->enabled, "Releasing supply %s while enabled\n", - regulator->supply_name)) - _regulator_disable(rdev); - /* remove any sysfs entries */ if (regulator->dev) { sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name); @@ -1144,12 +1140,7 @@ int regulator_enable(struct regulator *r int ret = 0; mutex_lock(&rdev->mutex); - if (regulator->enabled == 0) - ret = _regulator_enable(rdev); - else if (regulator->enabled < 0) - ret = -EIO; - if (ret == 0) - regulator->enabled++; + ret = _regulator_enable(rdev); mutex_unlock(&rdev->mutex); return ret; } @@ -1160,6 +1151,11 @@ static int _regulator_disable(struct reg { int ret = 0; + if (WARN(rdev->use_count <= 0, + "unbalanced disables for %s\n", + rdev->desc->name)) + return -EIO; + /* are we the last user and permitted to disable ? */ if (rdev->use_count == 1 && !rdev->constraints->always_on) { @@ -1208,16 +1204,7 @@ int regulator_disable(struct regulator * int ret = 0; mutex_lock(&rdev->mutex); - if (regulator->enabled == 1) { - ret = _regulator_disable(rdev); - if (ret == 0) - regulator->uA_load = 0; - } else if (WARN(regulator->enabled <= 0, - "unbalanced disables for supply %s\n", - regulator->supply_name)) - ret = -EIO; - if (ret == 0) - regulator->enabled--; + ret = _regulator_disable(rdev); mutex_unlock(&rdev->mutex); return ret; } @@ -1264,7 +1251,6 @@ int regulator_force_disable(struct regul int ret; mutex_lock(®ulator->rdev->mutex); - regulator->enabled = 0; regulator->uA_load = 0; ret = _regulator_force_disable(regulator->rdev); mutex_unlock(®ulator->rdev->mutex);