Re: [PATCH] regulator: core: Get and put regulator of_node

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature