diff mbox

[4/7] phy: rockchip-usb: add missing of_node_put

Message ID 1447673600-8881-5-git-send-email-Julia.Lawall@lip6.fr (mailing list archive)
State New, archived
Headers show

Commit Message

Julia Lawall Nov. 16, 2015, 11:33 a.m. UTC
for_each_available_child_of_node performs an of_node_get on each iteration,
so a return from the middle of the loop requires an of_node_put.

A simplified version of the semantic patch that finds this problem is as
follows (http://coccinelle.lip6.fr):

// <smpl>
@@
expression root,e;
local idexpression child;
@@

 for_each_available_child_of_node(root, child) {
   ... when != of_node_put(child)
       when != e = child
(
   return child;
|
*  return ...;
)
   ...
 }
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/phy/phy-rockchip-usb.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Heiko Stübner Nov. 18, 2015, 7:27 p.m. UTC | #1
Hi Julia,

Am Montag, 16. November 2015, 12:33:17 schrieb Julia Lawall:
> diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
> index 91d6f34..62c43c4 100644
> --- a/drivers/phy/phy-rockchip-usb.c
> +++ b/drivers/phy/phy-rockchip-usb.c
> @@ -108,13 +108,16 @@ static int rockchip_usb_phy_probe(struct
> platform_device *pdev)
> 
>  	for_each_available_child_of_node(dev->of_node, child) {
>  		rk_phy = devm_kzalloc(dev, sizeof(*rk_phy), GFP_KERNEL);
> -		if (!rk_phy)
> -			return -ENOMEM;
> +		if (!rk_phy) {
> +			err = -ENOMEM;
> +			goto put_child;
> +		}
> 
>  		if (of_property_read_u32(child, "reg", &reg_offset)) {
>  			dev_err(dev, "missing reg property in node %s\n",
>  				child->name);
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto put_child;
>  		}
> 
>  		rk_phy->reg_offset = reg_offset;
> @@ -127,18 +130,22 @@ static int rockchip_usb_phy_probe(struct
> platform_device *pdev) rk_phy->phy = devm_phy_create(dev, child, &ops);
>  		if (IS_ERR(rk_phy->phy)) {
>  			dev_err(dev, "failed to create PHY\n");
> -			return PTR_ERR(rk_phy->phy);
> +			err = PTR_ERR(rk_phy->phy);
> +			goto put_child;
>  		}
>  		phy_set_drvdata(rk_phy->phy, rk_phy);
> 
>  		/* only power up usb phy when it use, so disable it when init*/
>  		err = rockchip_usb_phy_power(rk_phy, 1);
>  		if (err)
> -			return err;
> +			goto put_child;
>  	}
> 
>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>  	return PTR_ERR_OR_ZERO(phy_provider);
> +put_child:
> +	of_node_put(child);
> +	return err;
>  }
> 
>  static const struct of_device_id rockchip_usb_phy_dt_ids[] = {

hmm, while I agree that the rockchip phy has an issue in the node lifecycle, 
I'm not sure that patch fixes it fully.

It currently iterates over each phy, but would only of_node_put the phy it 
handled last. So if an error happens on the 3rd phy, the first 2 are already 
instantiated and would also get removed when the overall probe fails, but 
their of_node would never be "put".

I think this goes together with what Brian described in patch1 having the phy-
core also handle the node-reference. When this is included we could just 
always put the of_node when finishing its loop iteration, as the phy-core will 
have its own reference on the node.


Heiko
Brian Norris Nov. 18, 2015, 7:31 p.m. UTC | #2
On Wed, Nov 18, 2015 at 08:27:07PM +0100, Heiko Stübner wrote:
> Am Montag, 16. November 2015, 12:33:17 schrieb Julia Lawall:
> hmm, while I agree that the rockchip phy has an issue in the node lifecycle, 
> I'm not sure that patch fixes it fully.
> 
> It currently iterates over each phy, but would only of_node_put the phy it 
> handled last. So if an error happens on the 3rd phy, the first 2 are already 
> instantiated and would also get removed when the overall probe fails, but 
> their of_node would never be "put".

Note the behavior of of_get_next_child() (and
of_get_next_available_child()); it "Decrements the refcount of prev." So
the loop only keeps a reference for (at most) one node at a time.

I believe Julia's patch is correct. It's possible the commit description
could have made this aspect clearer though, since I was confused about
this at first as well.

Regards,
Brian
Heiko Stübner Nov. 18, 2015, 7:46 p.m. UTC | #3
Am Mittwoch, 18. November 2015, 11:31:29 schrieb Brian Norris:
> On Wed, Nov 18, 2015 at 08:27:07PM +0100, Heiko Stübner wrote:
> > Am Montag, 16. November 2015, 12:33:17 schrieb Julia Lawall:
> > hmm, while I agree that the rockchip phy has an issue in the node
> > lifecycle, I'm not sure that patch fixes it fully.
> > 
> > It currently iterates over each phy, but would only of_node_put the phy it
> > handled last. So if an error happens on the 3rd phy, the first 2 are
> > already instantiated and would also get removed when the overall probe
> > fails, but their of_node would never be "put".
> 
> Note the behavior of of_get_next_child() (and
> of_get_next_available_child()); it "Decrements the refcount of prev." So
> the loop only keeps a reference for (at most) one node at a time.
> 
> I believe Julia's patch is correct. It's possible the commit description
> could have made this aspect clearer though, since I was confused about
> this at first as well.

oh, I hadn't realized that :-) .

Although in this case, what happens with the last child, if only "prev"s get 
decremented? When the loop finished I'd think that the last one would keep 
it's reference, as the patch stand right - or I'm just blind.
Julia Lawall Nov. 18, 2015, 8:38 p.m. UTC | #4
On Wed, 18 Nov 2015, Heiko Stübner wrote:

> Am Mittwoch, 18. November 2015, 11:31:29 schrieb Brian Norris:
> > On Wed, Nov 18, 2015 at 08:27:07PM +0100, Heiko Stübner wrote:
> > > Am Montag, 16. November 2015, 12:33:17 schrieb Julia Lawall:
> > > hmm, while I agree that the rockchip phy has an issue in the node
> > > lifecycle, I'm not sure that patch fixes it fully.
> > > 
> > > It currently iterates over each phy, but would only of_node_put the phy it
> > > handled last. So if an error happens on the 3rd phy, the first 2 are
> > > already instantiated and would also get removed when the overall probe
> > > fails, but their of_node would never be "put".
> > 
> > Note the behavior of of_get_next_child() (and
> > of_get_next_available_child()); it "Decrements the refcount of prev." So
> > the loop only keeps a reference for (at most) one node at a time.
> > 
> > I believe Julia's patch is correct. It's possible the commit description
> > could have made this aspect clearer though, since I was confused about
> > this at first as well.
> 
> oh, I hadn't realized that :-) .
> 
> Although in this case, what happens with the last child, if only "prev"s get 
> decremented? When the loop finished I'd think that the last one would keep 
> it's reference, as the patch stand right - or I'm just blind.

The loop finishes when the child is NULL.  So there is nothing to put in 
that case.  The process of getting from the last child to the NULL does 
the of_node_put.

julia
Heiko Stübner Nov. 18, 2015, 8:40 p.m. UTC | #5
Hi Julia,

Am Mittwoch, 18. November 2015, 21:38:02 schrieb Julia Lawall:
> On Wed, 18 Nov 2015, Heiko Stübner wrote:
> > Am Mittwoch, 18. November 2015, 11:31:29 schrieb Brian Norris:
> > > On Wed, Nov 18, 2015 at 08:27:07PM +0100, Heiko Stübner wrote:
> > > > Am Montag, 16. November 2015, 12:33:17 schrieb Julia Lawall:
> > > > hmm, while I agree that the rockchip phy has an issue in the node
> > > > lifecycle, I'm not sure that patch fixes it fully.
> > > > 
> > > > It currently iterates over each phy, but would only of_node_put the
> > > > phy it
> > > > handled last. So if an error happens on the 3rd phy, the first 2 are
> > > > already instantiated and would also get removed when the overall probe
> > > > fails, but their of_node would never be "put".
> > > 
> > > Note the behavior of of_get_next_child() (and
> > > of_get_next_available_child()); it "Decrements the refcount of prev." So
> > > the loop only keeps a reference for (at most) one node at a time.
> > > 
> > > I believe Julia's patch is correct. It's possible the commit description
> > > could have made this aspect clearer though, since I was confused about
> > > this at first as well.
> > 
> > oh, I hadn't realized that :-) .
> > 
> > Although in this case, what happens with the last child, if only "prev"s
> > get decremented? When the loop finished I'd think that the last one would
> > keep it's reference, as the patch stand right - or I'm just blind.
> 
> The loop finishes when the child is NULL.  So there is nothing to put in
> that case.  The process of getting from the last child to the NULL does
> the of_node_put.

sorry for being a bit slow today ... I should probably sleep more :-)

Then the patch looks fine ... I'll add my Tag on the top, to not burry it down 
here.


Heiko
Heiko Stübner Nov. 18, 2015, 9:42 p.m. UTC | #6
Am Montag, 16. November 2015, 12:33:17 schrieb Julia Lawall:
> for_each_available_child_of_node performs an of_node_get on each iteration,
> so a return from the middle of the loop requires an of_node_put.
> 
> A simplified version of the semantic patch that finds this problem is as
> follows (http://coccinelle.lip6.fr):
> 
> // <smpl>
> @@
> expression root,e;
> local idexpression child;
> @@
> 
>  for_each_available_child_of_node(root, child) {
>    ... when != of_node_put(child)
>        when != e = child
> (
>    return child;
> 
> *  return ...;
> )
>    ...
>  }
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

After 

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

on a rk3288-veyron chromebook
Tested-by: Heiko Stuebner <heiko@sntech.de>

> 
> ---
>  drivers/phy/phy-rockchip-usb.c |   17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
> index 91d6f34..62c43c4 100644
> --- a/drivers/phy/phy-rockchip-usb.c
> +++ b/drivers/phy/phy-rockchip-usb.c
> @@ -108,13 +108,16 @@ static int rockchip_usb_phy_probe(struct
> platform_device *pdev)
> 
>  	for_each_available_child_of_node(dev->of_node, child) {
>  		rk_phy = devm_kzalloc(dev, sizeof(*rk_phy), GFP_KERNEL);
> -		if (!rk_phy)
> -			return -ENOMEM;
> +		if (!rk_phy) {
> +			err = -ENOMEM;
> +			goto put_child;
> +		}
> 
>  		if (of_property_read_u32(child, "reg", &reg_offset)) {
>  			dev_err(dev, "missing reg property in node %s\n",
>  				child->name);
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto put_child;
>  		}
> 
>  		rk_phy->reg_offset = reg_offset;
> @@ -127,18 +130,22 @@ static int rockchip_usb_phy_probe(struct
> platform_device *pdev) rk_phy->phy = devm_phy_create(dev, child, &ops);
>  		if (IS_ERR(rk_phy->phy)) {
>  			dev_err(dev, "failed to create PHY\n");
> -			return PTR_ERR(rk_phy->phy);
> +			err = PTR_ERR(rk_phy->phy);
> +			goto put_child;
>  		}
>  		phy_set_drvdata(rk_phy->phy, rk_phy);
> 
>  		/* only power up usb phy when it use, so disable it when init*/
>  		err = rockchip_usb_phy_power(rk_phy, 1);
>  		if (err)
> -			return err;
> +			goto put_child;
>  	}
> 
>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>  	return PTR_ERR_OR_ZERO(phy_provider);
> +put_child:
> +	of_node_put(child);
> +	return err;
>  }
> 
>  static const struct of_device_id rockchip_usb_phy_dt_ids[] = {
diff mbox

Patch

diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
index 91d6f34..62c43c4 100644
--- a/drivers/phy/phy-rockchip-usb.c
+++ b/drivers/phy/phy-rockchip-usb.c
@@ -108,13 +108,16 @@  static int rockchip_usb_phy_probe(struct platform_device *pdev)
 
 	for_each_available_child_of_node(dev->of_node, child) {
 		rk_phy = devm_kzalloc(dev, sizeof(*rk_phy), GFP_KERNEL);
-		if (!rk_phy)
-			return -ENOMEM;
+		if (!rk_phy) {
+			err = -ENOMEM;
+			goto put_child;
+		}
 
 		if (of_property_read_u32(child, "reg", &reg_offset)) {
 			dev_err(dev, "missing reg property in node %s\n",
 				child->name);
-			return -EINVAL;
+			err = -EINVAL;
+			goto put_child;
 		}
 
 		rk_phy->reg_offset = reg_offset;
@@ -127,18 +130,22 @@  static int rockchip_usb_phy_probe(struct platform_device *pdev)
 		rk_phy->phy = devm_phy_create(dev, child, &ops);
 		if (IS_ERR(rk_phy->phy)) {
 			dev_err(dev, "failed to create PHY\n");
-			return PTR_ERR(rk_phy->phy);
+			err = PTR_ERR(rk_phy->phy);
+			goto put_child;
 		}
 		phy_set_drvdata(rk_phy->phy, rk_phy);
 
 		/* only power up usb phy when it use, so disable it when init*/
 		err = rockchip_usb_phy_power(rk_phy, 1);
 		if (err)
-			return err;
+			goto put_child;
 	}
 
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 	return PTR_ERR_OR_ZERO(phy_provider);
+put_child:
+	of_node_put(child);
+	return err;
 }
 
 static const struct of_device_id rockchip_usb_phy_dt_ids[] = {