On Wed, Apr 02, 2014 at 05:04:38PM +0100, Charles Keepax wrote: > On Wed, Apr 02, 2014 at 03:23:56PM +0100, Mark Brown wrote: > > On Wed, Apr 02, 2014 at 03:06:54PM +0100, Charles Keepax wrote:
> > > We should be incrementing the reference count of the of_node for the > > > regulator when we take a copy of it. This patch does so. > > Why? > Apologies I must be missing something, take for example the fixed The immediate thing you're missing is this explanation - it's not sufficiently obvious why the reference needs to be held. > regulator driver (fixed.c). We get an of_node from > pdev->dev->of_node, pull the init_data from it, then copy the > of_node into the regulator_config and call regulator_register. > regulator_register will copy the of_node from the regulator_config > and put it into the new device, but as far as I can see no one ever > incremented the reference count? > of_node = pdev->dev.of_node; > drvdata->dev = regulator_register(&drvdata->desc, &cfg); > Looks like there are a couple of other regulator drivers in the > same boat. Just seems easier to let the core do the reference > stuff rather than needing to do it in the drivers. Two things here. One is that we're not copying the node, we're copying a pointer to it (duplicating the node would be a problem since we need to look for a phandle to the original node to do consumer lookups). The other is that if there is an issue with things never getting referenced at all here then taking a reference within the regulator code is too late - if a reference isn't already held at the point where we pass the node into the regulator core then the pointer that's getting passed about is already potentially invalid. To make this correct we need to at least ensure that the node passed into the regulator API is valid and referenced at that time so there should only be an issue for the core if the reference is dropped after that. In the above case the device model is holding a reference since this is the of_node for the device itself so taking the reference won't hurt but is redundant. In cases where we have more than one regulator and are using of_regulator_match() then things are more tricky. Something needs to drop the references it returns (which isn't happening at all at the minute). Doing it while doing the match and register seems simple and neat from an error handling point of view so having the core take an additional reference during the registration would join up with that.
signature.asc
Description: Digital signature
