| Message ID | 1447673600-8881-5-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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", ®_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
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
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.
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
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
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", ®_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 --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", ®_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[] = {
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(-)