The various for_each device_node iterators performs an of_node_get on each
iteration, so a break out of the loop requires an of_node_put.
The complete semantic patch that finds this problem is
(http://coccinelle.lip6.fr):
// <smpl>
@r@
local idexpression n;
expression e1,e2;
iterator name for_each_node_by_name, for_each_node_by_type,
for_each_compatible_node, for_each_matching_node,
for_each_matching_node_and_match, for_each_child_of_node,
for_each_available_child_of_node, for_each_node_with_property;
iterator i;
statement S;
expression list [n1] es;
@@
(
(
for_each_node_by_name(n,e1) S
|
for_each_node_by_type(n,e1) S
|
for_each_compatible_node(n,e1,e2) S
|
for_each_matching_node(n,e1) S
|
for_each_matching_node_and_match(n,e1,e2) S
|
for_each_child_of_node(e1,n) S
|
for_each_available_child_of_node(e1,n) S
|
for_each_node_with_property(n,e1) S
)
&
i(es,n,...) S
)
@@
local idexpression r.n;
iterator r.i;
expression e;
expression list [r.n1] es;
@@
i(es,n,...) {
...
(
of_node_put(n);
|
e = n
|
return n;
|
+ of_node_put(n);
? return ...;
)
...
}
@@
local idexpression r.n;
iterator r.i;
expression e;
expression list [r.n1] es;
@@
i(es,n,...) {
...
(
of_node_put(n);
|
e = n
|
+ of_node_put(n);
? break;
)
...
}
... when != n
@@
local idexpression r.n;
iterator r.i;
expression e;
identifier l;
expression list [r.n1] es;
@@
i(es,n,...) {
...
(
of_node_put(n);
|
e = n
|
+ of_node_put(n);
? goto l;
)
...
}
...
l: ... when != n
// </smpl>
This semantic patch puts an of_node_put before a return, but in each of
these patches, we have grouped the multiple resulting of_node_puts into a
single call at an error exit label.
---
drivers/phy/phy-bcm-cygnus-pcie.c | 16 ++++++++++++----
drivers/phy/phy-berlin-sata.c | 20 ++++++++++++++------
drivers/phy/phy-brcmstb-sata.c | 17 ++++++++++++-----
drivers/phy/phy-miphy28lp.c | 16 +++++++++++-----
drivers/phy/phy-miphy365x.c | 16 +++++++++++-----
drivers/phy/phy-mt65xx-usb3.c | 20 +++++++++++++-------
drivers/phy/phy-rockchip-usb.c | 17 ++++++++++++-----
7 files changed, 85 insertions(+), 37 deletions(-)
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 <[email protected]>
---
drivers/phy/phy-brcmstb-sata.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/phy/phy-brcmstb-sata.c b/drivers/phy/phy-brcmstb-sata.c
index 8a2cb16..cd9dba8 100644
--- a/drivers/phy/phy-brcmstb-sata.c
+++ b/drivers/phy/phy-brcmstb-sata.c
@@ -140,7 +140,7 @@ static int brcm_sata_phy_probe(struct platform_device *pdev)
struct brcm_sata_phy *priv;
struct resource *res;
struct phy_provider *provider;
- int count = 0;
+ int ret, count = 0;
if (of_get_child_count(dn) == 0)
return -ENODEV;
@@ -163,16 +163,19 @@ static int brcm_sata_phy_probe(struct platform_device *pdev)
if (of_property_read_u32(child, "reg", &id)) {
dev_err(dev, "missing reg property in node %s\n",
child->name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto put_child;
}
if (id >= MAX_PORTS) {
dev_err(dev, "invalid reg: %u\n", id);
- return -EINVAL;
+ ret = -EINVAL;
+ goto put_child;
}
if (priv->phys[id].phy) {
dev_err(dev, "already registered port %u\n", id);
- return -EINVAL;
+ ret = -EINVAL;
+ goto put_child;
}
port = &priv->phys[id];
@@ -182,7 +185,8 @@ static int brcm_sata_phy_probe(struct platform_device *pdev)
port->ssc_en = of_property_read_bool(child, "brcm,enable-ssc");
if (IS_ERR(port->phy)) {
dev_err(dev, "failed to create PHY\n");
- return PTR_ERR(port->phy);
+ ret = PTR_ERR(port->phy);
+ goto put_child;
}
phy_set_drvdata(port->phy, port);
@@ -198,6 +202,9 @@ static int brcm_sata_phy_probe(struct platform_device *pdev)
dev_info(dev, "registered %d port(s)\n", count);
return 0;
+put_child:
+ of_node_put(child);
+ return ret;
}
static struct platform_driver brcm_sata_phy_driver = {
for_each_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_child_of_node(root, child) {
... when != of_node_put(child)
when != e = child
(
return child;
|
* return ...;
)
...
}
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/phy/phy-mt65xx-usb3.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/phy/phy-mt65xx-usb3.c b/drivers/phy/phy-mt65xx-usb3.c
index f30b28b..e427c3b 100644
--- a/drivers/phy/phy-mt65xx-usb3.c
+++ b/drivers/phy/phy-mt65xx-usb3.c
@@ -415,7 +415,7 @@ static int mt65xx_u3phy_probe(struct platform_device *pdev)
struct resource *sif_res;
struct mt65xx_u3phy *u3phy;
struct resource res;
- int port;
+ int port, retval;
u3phy = devm_kzalloc(dev, sizeof(*u3phy), GFP_KERNEL);
if (!u3phy)
@@ -447,31 +447,34 @@ static int mt65xx_u3phy_probe(struct platform_device *pdev)
for_each_child_of_node(np, child_np) {
struct mt65xx_phy_instance *instance;
struct phy *phy;
- int retval;
instance = devm_kzalloc(dev, sizeof(*instance), GFP_KERNEL);
- if (!instance)
- return -ENOMEM;
+ if (!instance) {
+ retval = -ENOMEM;
+ goto put_child;
+ }
u3phy->phys[port] = instance;
phy = devm_phy_create(dev, child_np, &mt65xx_u3phy_ops);
if (IS_ERR(phy)) {
dev_err(dev, "failed to create phy\n");
- return PTR_ERR(phy);
+ retval = PTR_ERR(phy);
+ goto put_child;
}
retval = of_address_to_resource(child_np, 0, &res);
if (retval) {
dev_err(dev, "failed to get address resource(id-%d)\n",
port);
- return retval;
+ goto put_child;
}
instance->port_base = devm_ioremap_resource(&phy->dev, &res);
if (IS_ERR(instance->port_base)) {
dev_err(dev, "failed to remap phy regs\n");
- return PTR_ERR(instance->port_base);
+ retval = PTR_ERR(instance->port_base);
+ goto put_child;
}
instance->phy = phy;
@@ -483,6 +486,9 @@ static int mt65xx_u3phy_probe(struct platform_device *pdev)
provider = devm_of_phy_provider_register(dev, mt65xx_phy_xlate);
return PTR_ERR_OR_ZERO(provider);
+put_child:
+ of_node_put(child_np);
+ return retval;
}
static const struct of_device_id mt65xx_u3phy_id_table[] = {
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 <[email protected]>
---
drivers/phy/phy-berlin-sata.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/phy/phy-berlin-sata.c b/drivers/phy/phy-berlin-sata.c
index 77a2e05..f84a33a 100644
--- a/drivers/phy/phy-berlin-sata.c
+++ b/drivers/phy/phy-berlin-sata.c
@@ -195,7 +195,7 @@ static int phy_berlin_sata_probe(struct platform_device *pdev)
struct phy_provider *phy_provider;
struct phy_berlin_priv *priv;
struct resource *res;
- int i = 0;
+ int ret, i = 0;
u32 phy_id;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -237,22 +237,27 @@ static int phy_berlin_sata_probe(struct platform_device *pdev)
if (of_property_read_u32(child, "reg", &phy_id)) {
dev_err(dev, "missing reg property in node %s\n",
child->name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto put_child;
}
if (phy_id >= ARRAY_SIZE(phy_berlin_power_down_bits)) {
dev_err(dev, "invalid reg in node %s\n", child->name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto put_child;
}
phy_desc = devm_kzalloc(dev, sizeof(*phy_desc), GFP_KERNEL);
- if (!phy_desc)
- return -ENOMEM;
+ if (!phy_desc) {
+ ret = -ENOMEM;
+ goto put_child;
+ }
phy = devm_phy_create(dev, NULL, &phy_berlin_sata_ops);
if (IS_ERR(phy)) {
dev_err(dev, "failed to create PHY %d\n", phy_id);
- return PTR_ERR(phy);
+ ret = PTR_ERR(phy);
+ goto put_child;
}
phy_desc->phy = phy;
@@ -269,6 +274,9 @@ static int phy_berlin_sata_probe(struct platform_device *pdev)
phy_provider =
devm_of_phy_provider_register(dev, phy_berlin_sata_phy_xlate);
return PTR_ERR_OR_ZERO(phy_provider);
+put_child:
+ of_node_put(child);
+ return ret;
}
static const struct of_device_id phy_berlin_sata_of_match[] = {
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 <[email protected]>
---
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[] = {
for_each_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_child_of_node(root, child) {
... when != of_node_put(child)
when != e = child
(
return child;
|
* return ...;
)
...
}
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/phy/phy-miphy28lp.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/phy/phy-miphy28lp.c b/drivers/phy/phy-miphy28lp.c
index c47b56b..3acd2a1 100644
--- a/drivers/phy/phy-miphy28lp.c
+++ b/drivers/phy/phy-miphy28lp.c
@@ -1226,15 +1226,18 @@ static int miphy28lp_probe(struct platform_device *pdev)
miphy_phy = devm_kzalloc(&pdev->dev, sizeof(*miphy_phy),
GFP_KERNEL);
- if (!miphy_phy)
- return -ENOMEM;
+ if (!miphy_phy) {
+ ret = -ENOMEM;
+ goto put_child;
+ }
miphy_dev->phys[port] = miphy_phy;
phy = devm_phy_create(&pdev->dev, child, &miphy28lp_ops);
if (IS_ERR(phy)) {
dev_err(&pdev->dev, "failed to create PHY\n");
- return PTR_ERR(phy);
+ ret = PTR_ERR(phy);
+ goto put_child;
}
miphy_dev->phys[port]->phy = phy;
@@ -1242,11 +1245,11 @@ static int miphy28lp_probe(struct platform_device *pdev)
ret = miphy28lp_of_probe(child, miphy_phy);
if (ret)
- return ret;
+ goto put_child;
ret = miphy28lp_probe_resets(child, miphy_dev->phys[port]);
if (ret)
- return ret;
+ goto put_child;
phy_set_drvdata(phy, miphy_dev->phys[port]);
port++;
@@ -1255,6 +1258,9 @@ static int miphy28lp_probe(struct platform_device *pdev)
provider = devm_of_phy_provider_register(&pdev->dev, miphy28lp_xlate);
return PTR_ERR_OR_ZERO(provider);
+put_child:
+ of_node_put(child);
+ return ret;
}
static const struct of_device_id miphy28lp_of_match[] = {
for_each_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_child_of_node(root, child) {
... when != of_node_put(child)
when != e = child
(
return child;
|
* return ...;
)
...
}
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/phy/phy-miphy365x.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/phy/phy-miphy365x.c b/drivers/phy/phy-miphy365x.c
index 00a686a..e661f3b 100644
--- a/drivers/phy/phy-miphy365x.c
+++ b/drivers/phy/phy-miphy365x.c
@@ -566,22 +566,25 @@ static int miphy365x_probe(struct platform_device *pdev)
miphy_phy = devm_kzalloc(&pdev->dev, sizeof(*miphy_phy),
GFP_KERNEL);
- if (!miphy_phy)
- return -ENOMEM;
+ if (!miphy_phy) {
+ ret = -ENOMEM;
+ goto put_child;
+ }
miphy_dev->phys[port] = miphy_phy;
phy = devm_phy_create(&pdev->dev, child, &miphy365x_ops);
if (IS_ERR(phy)) {
dev_err(&pdev->dev, "failed to create PHY\n");
- return PTR_ERR(phy);
+ ret = PTR_ERR(phy);
+ goto put_child;
}
miphy_dev->phys[port]->phy = phy;
ret = miphy365x_of_probe(child, miphy_phy);
if (ret)
- return ret;
+ goto put_child;
phy_set_drvdata(phy, miphy_dev->phys[port]);
@@ -591,12 +594,15 @@ static int miphy365x_probe(struct platform_device *pdev)
&miphy_phy->ctrlreg);
if (ret) {
dev_err(&pdev->dev, "No sysconfig offset found\n");
- return ret;
+ goto put_child;
}
}
provider = devm_of_phy_provider_register(&pdev->dev, miphy365x_xlate);
return PTR_ERR_OR_ZERO(provider);
+put_child:
+ of_node_put(child);
+ return ret;
}
static const struct of_device_id miphy365x_of_match[] = {
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 <[email protected]>
---
drivers/phy/phy-bcm-cygnus-pcie.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/phy/phy-bcm-cygnus-pcie.c b/drivers/phy/phy-bcm-cygnus-pcie.c
index 7ad72b7..082c03f 100644
--- a/drivers/phy/phy-bcm-cygnus-pcie.c
+++ b/drivers/phy/phy-bcm-cygnus-pcie.c
@@ -128,6 +128,7 @@ static int cygnus_pcie_phy_probe(struct platform_device *pdev)
struct phy_provider *provider;
struct resource *res;
unsigned cnt = 0;
+ int ret;
if (of_get_child_count(node) == 0) {
dev_err(dev, "PHY no child node\n");
@@ -154,24 +155,28 @@ static int cygnus_pcie_phy_probe(struct platform_device *pdev)
if (of_property_read_u32(child, "reg", &id)) {
dev_err(dev, "missing reg property for %s\n",
child->name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto put_child;
}
if (id >= MAX_NUM_PHYS) {
dev_err(dev, "invalid PHY id: %u\n", id);
- return -EINVAL;
+ ret = -EINVAL;
+ goto put_child;
}
if (core->phys[id].phy) {
dev_err(dev, "duplicated PHY id: %u\n", id);
- return -EINVAL;
+ ret = -EINVAL;
+ goto put_child;
}
p = &core->phys[id];
p->phy = devm_phy_create(dev, child, &cygnus_pcie_phy_ops);
if (IS_ERR(p->phy)) {
dev_err(dev, "failed to create PHY\n");
- return PTR_ERR(p->phy);
+ ret = PTR_ERR(p->phy);
+ goto put_child;
}
p->core = core;
@@ -191,6 +196,9 @@ static int cygnus_pcie_phy_probe(struct platform_device *pdev)
dev_dbg(dev, "registered %u PCIe PHY(s)\n", cnt);
return 0;
+put_child:
+ of_node_put(child);
+ return ret;
}
static const struct of_device_id cygnus_pcie_phy_match_table[] = {
Hi Julia,
On 11/16/2015 3:33 AM, Julia Lawall wrote:
> 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 <[email protected]>
>
> ---
> drivers/phy/phy-bcm-cygnus-pcie.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/phy/phy-bcm-cygnus-pcie.c b/drivers/phy/phy-bcm-cygnus-pcie.c
> index 7ad72b7..082c03f 100644
> --- a/drivers/phy/phy-bcm-cygnus-pcie.c
> +++ b/drivers/phy/phy-bcm-cygnus-pcie.c
> @@ -128,6 +128,7 @@ static int cygnus_pcie_phy_probe(struct platform_device *pdev)
> struct phy_provider *provider;
> struct resource *res;
> unsigned cnt = 0;
> + int ret;
>
> if (of_get_child_count(node) == 0) {
> dev_err(dev, "PHY no child node\n");
> @@ -154,24 +155,28 @@ static int cygnus_pcie_phy_probe(struct platform_device *pdev)
> if (of_property_read_u32(child, "reg", &id)) {
> dev_err(dev, "missing reg property for %s\n",
> child->name);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto put_child;
> }
>
> if (id >= MAX_NUM_PHYS) {
> dev_err(dev, "invalid PHY id: %u\n", id);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto put_child;
> }
>
> if (core->phys[id].phy) {
> dev_err(dev, "duplicated PHY id: %u\n", id);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto put_child;
> }
>
> p = &core->phys[id];
> p->phy = devm_phy_create(dev, child, &cygnus_pcie_phy_ops);
> if (IS_ERR(p->phy)) {
> dev_err(dev, "failed to create PHY\n");
> - return PTR_ERR(p->phy);
> + ret = PTR_ERR(p->phy);
> + goto put_child;
> }
>
> p->core = core;
> @@ -191,6 +196,9 @@ static int cygnus_pcie_phy_probe(struct platform_device *pdev)
> dev_dbg(dev, "registered %u PCIe PHY(s)\n", cnt);
>
> return 0;
> +put_child:
> + of_node_put(child);
> + return ret;
> }
>
> static const struct of_device_id cygnus_pcie_phy_match_table[] = {
>
This fix looks good to me. Thanks!
Reviewed-by: Ray Jui <[email protected]>
On Mon, Nov 16, 2015 at 12:33:14PM +0100, Julia Lawall wrote:
> 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 <[email protected]>
>
> ---
For this patch:
Acked-by: Brian Norris <[email protected]>
> drivers/phy/phy-brcmstb-sata.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
[snip patch, which fixes of_node_put() handling for
for_each_available_child_of_node() loop, which creates PHY devices with
devm_phy_create()]
This reminds me of a potential problem I'm looking at in other
subsystems: from code reading (I haven't seen any issues in practice,
probably because I don't use OF_DYNAMIC) it looks like device-creating
infrastructure like the PHY subsystem should be acquiring a reference to
the device_node when they stash it away. But drivers/phy/phy-core.c does
not do this, AFAICT.
See phy_create(), which does
phy->dev.of_node = node ?: dev->of_node;
and later might reuse this of_node pointer, even though it never called
of_node_get() on this node.
Potential patch to fix this (not tested).
Signed-off-by: Brian Norris <[email protected]>
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index fc48fac003a6..8df29caeeef9 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -697,6 +697,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
phy->dev.class = phy_class;
phy->dev.parent = dev;
phy->dev.of_node = node ?: dev->of_node;
+ of_node_get(phy->dev.of_node);
phy->id = id;
phy->ops = ops;
@@ -726,6 +727,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
return phy;
put_dev:
+ of_node_put(phy->dev.of_node);
put_device(&phy->dev); /* calls phy_release() which frees resources */
return ERR_PTR(ret);
@@ -775,6 +777,7 @@ EXPORT_SYMBOL_GPL(devm_phy_create);
*/
void phy_destroy(struct phy *phy)
{
+ of_node_put(phy->dev.of_node);
pm_runtime_disable(&phy->dev);
device_unregister(&phy->dev);
}
On Mon, 16 Nov 2015, Brian Norris wrote:
> On Mon, Nov 16, 2015 at 12:33:14PM +0100, Julia Lawall wrote:
> > 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 <[email protected]>
> >
> > ---
>
> For this patch:
>
> Acked-by: Brian Norris <[email protected]>
>
> > drivers/phy/phy-brcmstb-sata.c | 17 ++++++++++++-----
> > 1 file changed, 12 insertions(+), 5 deletions(-)
>
> [snip patch, which fixes of_node_put() handling for
> for_each_available_child_of_node() loop, which creates PHY devices with
> devm_phy_create()]
>
> This reminds me of a potential problem I'm looking at in other
> subsystems: from code reading (I haven't seen any issues in practice,
> probably because I don't use OF_DYNAMIC) it looks like device-creating
> infrastructure like the PHY subsystem should be acquiring a reference to
> the device_node when they stash it away. But drivers/phy/phy-core.c does
> not do this, AFAICT.
>
> See phy_create(), which does
>
> phy->dev.of_node = node ?: dev->of_node;
>
> and later might reuse this of_node pointer, even though it never called
> of_node_get() on this node.
>
> Potential patch to fix this (not tested).
>
> Signed-off-by: Brian Norris <[email protected]>
>
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index fc48fac003a6..8df29caeeef9 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -697,6 +697,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> phy->dev.class = phy_class;
> phy->dev.parent = dev;
> phy->dev.of_node = node ?: dev->of_node;
> + of_node_get(phy->dev.of_node);
Why not put of_node_get around dev->of_node?
julia
> phy->id = id;
> phy->ops = ops;
>
> @@ -726,6 +727,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> return phy;
>
> put_dev:
> + of_node_put(phy->dev.of_node);
> put_device(&phy->dev); /* calls phy_release() which frees resources */
> return ERR_PTR(ret);
>
> @@ -775,6 +777,7 @@ EXPORT_SYMBOL_GPL(devm_phy_create);
> */
> void phy_destroy(struct phy *phy)
> {
> + of_node_put(phy->dev.of_node);
> pm_runtime_disable(&phy->dev);
> device_unregister(&phy->dev);
> }
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Tue, Nov 17, 2015 at 07:12:22AM +0100, Julia Lawall wrote:
> On Mon, 16 Nov 2015, Brian Norris wrote:
> >
> > This reminds me of a potential problem I'm looking at in other
> > subsystems: from code reading (I haven't seen any issues in practice,
> > probably because I don't use OF_DYNAMIC) it looks like device-creating
> > infrastructure like the PHY subsystem should be acquiring a reference to
> > the device_node when they stash it away. But drivers/phy/phy-core.c does
> > not do this, AFAICT.
> >
> > See phy_create(), which does
> >
> > phy->dev.of_node = node ?: dev->of_node;
> >
> > and later might reuse this of_node pointer, even though it never called
> > of_node_get() on this node.
> >
> > Potential patch to fix this (not tested).
> >
> > Signed-off-by: Brian Norris <[email protected]>
> >
> > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > index fc48fac003a6..8df29caeeef9 100644
> > --- a/drivers/phy/phy-core.c
> > +++ b/drivers/phy/phy-core.c
> > @@ -697,6 +697,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> > phy->dev.class = phy_class;
> > phy->dev.parent = dev;
> > phy->dev.of_node = node ?: dev->of_node;
> > + of_node_get(phy->dev.of_node);
>
> Why not put of_node_get around dev->of_node?
Like this?
phy->dev.of_node = node ?: of_node_get(dev->of_node);
Or this?
phy->dev.of_node = of_node_get(node ?: dev->of_node);
The former wouldn't do what I proposed; if this PHY device is created
with a sub-node of 'dev' rather than dev->of_node, then the caller will
pass it in as the 2nd argument to phy_create (i.e., 'node'), and then I
expect it's the PHY core's responsibility to refcount it.
I'd be fine with the latter. Looks a little better, I suppose.
If my understanding is correct, I'll send a proper patch to do the
latter.
Regards,
Brian
> julia
>
> > phy->id = id;
> > phy->ops = ops;
> >
> > @@ -726,6 +727,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> > return phy;
> >
> > put_dev:
> > + of_node_put(phy->dev.of_node);
> > put_device(&phy->dev); /* calls phy_release() which frees resources */
> > return ERR_PTR(ret);
> >
> > @@ -775,6 +777,7 @@ EXPORT_SYMBOL_GPL(devm_phy_create);
> > */
> > void phy_destroy(struct phy *phy)
> > {
> > + of_node_put(phy->dev.of_node);
> > pm_runtime_disable(&phy->dev);
> > device_unregister(&phy->dev);
> > }
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
On Tue, 17 Nov 2015, Brian Norris wrote:
> On Tue, Nov 17, 2015 at 07:12:22AM +0100, Julia Lawall wrote:
> > On Mon, 16 Nov 2015, Brian Norris wrote:
> > >
> > > This reminds me of a potential problem I'm looking at in other
> > > subsystems: from code reading (I haven't seen any issues in practice,
> > > probably because I don't use OF_DYNAMIC) it looks like device-creating
> > > infrastructure like the PHY subsystem should be acquiring a reference to
> > > the device_node when they stash it away. But drivers/phy/phy-core.c does
> > > not do this, AFAICT.
> > >
> > > See phy_create(), which does
> > >
> > > phy->dev.of_node = node ?: dev->of_node;
> > >
> > > and later might reuse this of_node pointer, even though it never called
> > > of_node_get() on this node.
> > >
> > > Potential patch to fix this (not tested).
> > >
> > > Signed-off-by: Brian Norris <[email protected]>
> > >
> > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > > index fc48fac003a6..8df29caeeef9 100644
> > > --- a/drivers/phy/phy-core.c
> > > +++ b/drivers/phy/phy-core.c
> > > @@ -697,6 +697,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> > > phy->dev.class = phy_class;
> > > phy->dev.parent = dev;
> > > phy->dev.of_node = node ?: dev->of_node;
> > > + of_node_get(phy->dev.of_node);
> >
> > Why not put of_node_get around dev->of_node?
>
> Like this?
>
> phy->dev.of_node = node ?: of_node_get(dev->of_node);
>
> Or this?
>
> phy->dev.of_node = of_node_get(node ?: dev->of_node);
>
> The former wouldn't do what I proposed; if this PHY device is created
> with a sub-node of 'dev' rather than dev->of_node, then the caller will
> pass it in as the 2nd argument to phy_create (i.e., 'node'), and then I
> expect it's the PHY core's responsibility to refcount it.
>
> I'd be fine with the latter. Looks a little better, I suppose.
I proposed it because I was worried that the of_node field could end up
containing something that had been freed. But probably this is not
possible? If it is not possible, then the ?: in the function argument is
probably a bit ugly...
Is this something that should be checked for elsewhere?
julia
> If my understanding is correct, I'll send a proper patch to do the
> latter.
>
> Regards,
> Brian
>
> > julia
> >
> > > phy->id = id;
> > > phy->ops = ops;
> > >
> > > @@ -726,6 +727,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> > > return phy;
> > >
> > > put_dev:
> > > + of_node_put(phy->dev.of_node);
> > > put_device(&phy->dev); /* calls phy_release() which frees resources */
> > > return ERR_PTR(ret);
> > >
> > > @@ -775,6 +777,7 @@ EXPORT_SYMBOL_GPL(devm_phy_create);
> > > */
> > > void phy_destroy(struct phy *phy)
> > > {
> > > + of_node_put(phy->dev.of_node);
> > > pm_runtime_disable(&phy->dev);
> > > device_unregister(&phy->dev);
> > > }
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
>
On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote:
> On Tue, 17 Nov 2015, Brian Norris wrote:
> > On Tue, Nov 17, 2015 at 07:12:22AM +0100, Julia Lawall wrote:
> > > On Mon, 16 Nov 2015, Brian Norris wrote:
> > > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > > > index fc48fac003a6..8df29caeeef9 100644
> > > > --- a/drivers/phy/phy-core.c
> > > > +++ b/drivers/phy/phy-core.c
> > > > @@ -697,6 +697,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> > > > phy->dev.class = phy_class;
> > > > phy->dev.parent = dev;
> > > > phy->dev.of_node = node ?: dev->of_node;
> > > > + of_node_get(phy->dev.of_node);
> > >
> > > Why not put of_node_get around dev->of_node?
> >
> > Like this?
> >
> > phy->dev.of_node = node ?: of_node_get(dev->of_node);
> >
> > Or this?
> >
> > phy->dev.of_node = of_node_get(node ?: dev->of_node);
> >
> > The former wouldn't do what I proposed; if this PHY device is created
> > with a sub-node of 'dev' rather than dev->of_node, then the caller will
> > pass it in as the 2nd argument to phy_create (i.e., 'node'), and then I
> > expect it's the PHY core's responsibility to refcount it.
> >
> > I'd be fine with the latter. Looks a little better, I suppose.
>
> I proposed it because I was worried that the of_node field could end up
> containing something that had been freed. But probably this is not
> possible?
AIUI, the caller of phy_create() should already have a refcount on both
'dev->of_node' and 'node' (if applicable), so nobody should be freeing
it from underneath us right here. But *after* phy_create() returns,
there's no guarantee the caller will hold a reference for us.
So even if it's ever possible, I'd consider it a bug in the caller, not
in phy_create().
> If it is not possible, then the ?: in the function argument is
> probably a bit ugly...
OK, then I'll go with my first proposal.
> Is this something that should be checked for elsewhere?
I expect the same sort of problem shows up plenty of other places. I
don't think many people use CONFIG_OF_DYNAMIC, so the effects of these
failures probably aren't felt by many.
Brian
On Tue, Nov 17, 2015 at 10:30:36AM -0800, Brian Norris wrote:
> I expect the same sort of problem shows up plenty of other places. I
> don't think many people use CONFIG_OF_DYNAMIC, so the effects of these
> failures probably aren't felt by many.
Also, there's a quite-relevant todo item in
Documentation/devicetree/todo.txt:
=== CONFIG_OF_DYNAMIC ===
...
- Document node lifecycle for CONFIG_OF_DYNAMIC
:)
Brian
On Tue, 17 Nov 2015, Brian Norris wrote:
> On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote:
> > On Tue, 17 Nov 2015, Brian Norris wrote:
> > > On Tue, Nov 17, 2015 at 07:12:22AM +0100, Julia Lawall wrote:
> > > > On Mon, 16 Nov 2015, Brian Norris wrote:
> > > > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > > > > index fc48fac003a6..8df29caeeef9 100644
> > > > > --- a/drivers/phy/phy-core.c
> > > > > +++ b/drivers/phy/phy-core.c
> > > > > @@ -697,6 +697,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> > > > > phy->dev.class = phy_class;
> > > > > phy->dev.parent = dev;
> > > > > phy->dev.of_node = node ?: dev->of_node;
> > > > > + of_node_get(phy->dev.of_node);
> > > >
> > > > Why not put of_node_get around dev->of_node?
> > >
> > > Like this?
> > >
> > > phy->dev.of_node = node ?: of_node_get(dev->of_node);
> > >
> > > Or this?
> > >
> > > phy->dev.of_node = of_node_get(node ?: dev->of_node);
> > >
> > > The former wouldn't do what I proposed; if this PHY device is created
> > > with a sub-node of 'dev' rather than dev->of_node, then the caller will
> > > pass it in as the 2nd argument to phy_create (i.e., 'node'), and then I
> > > expect it's the PHY core's responsibility to refcount it.
> > >
> > > I'd be fine with the latter. Looks a little better, I suppose.
> >
> > I proposed it because I was worried that the of_node field could end up
> > containing something that had been freed. But probably this is not
> > possible?
>
> AIUI, the caller of phy_create() should already have a refcount on both
> 'dev->of_node' and 'node' (if applicable), so nobody should be freeing
> it from underneath us right here. But *after* phy_create() returns,
> there's no guarantee the caller will hold a reference for us.
>
> So even if it's ever possible, I'd consider it a bug in the caller, not
> in phy_create().
>
> > If it is not possible, then the ?: in the function argument is
> > probably a bit ugly...
>
> OK, then I'll go with my first proposal.
>
> > Is this something that should be checked for elsewhere?
>
> I expect the same sort of problem shows up plenty of other places. I
> don't think many people use CONFIG_OF_DYNAMIC, so the effects of these
> failures probably aren't felt by many.
I tried the following semantic patch:
@@
struct device_node *e;
expression e1;
identifier fld;
@@
... when != of_node_get(...)
*(<+...e1->fld...+>) = e
... when != of_node_get(...)
return e1;
basically, this says that a structure field is initilized to a device node
value, the structure is returned by the containing function, and the
containing function contains no of_node_get at all. Certainly this is
quite constrained, but it does produce a number of examples.
I looked at a few of them:
drivers/clk/ingenic/cgu.c, ingenic_cgu_new
clk/pistachio/clk.c, pistachio_clk_alloc_provider
drivers/mfd/syscon.c, of_syscon_register
drivers/of/pdt.c, function of_pdt_create_node
Any idea whether these need of_node_get? In all cases the device node
value comes in as a parameter.
thanks,
julia
(changing subject, add [email protected])
On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote:
> On Tue, 17 Nov 2015, Brian Norris wrote:
> > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote:
> > > Is this something that should be checked for elsewhere?
> >
> > I expect the same sort of problem shows up plenty of other places. I
> > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these
> > failures probably aren't felt by many.
>
> I tried the following semantic patch:
>
> @@
> struct device_node *e;
> expression e1;
> identifier fld;
> @@
>
> ... when != of_node_get(...)
> *(<+...e1->fld...+>) = e
> ... when != of_node_get(...)
> return e1;
>
> basically, this says that a structure field is initilized to a device node
> value, the structure is returned by the containing function, and the
> containing function contains no of_node_get at all. Certainly this is
> quite constrained, but it does produce a number of examples.
>
> I looked at a few of them:
>
> drivers/clk/ingenic/cgu.c, ingenic_cgu_new
> clk/pistachio/clk.c, pistachio_clk_alloc_provider
It looks like the clock core (drivers/clk/clk.c) initially grabs the clk
provider node in of_clk_init(), then drops it after it's initialized,
but most of these providers use of_clk_add_provider(), which seems to
manage the device_node lifetime for the user. So I think these are OK.
> drivers/mfd/syscon.c, of_syscon_register
This one looks potentially suspect. Syscon nodes aren't usually directly
managed by a single driver, and the device_node pointer is used for
lookups later...so I think it should keep a kref, and it doesn't.
> drivers/of/pdt.c, function of_pdt_create_node
Not real sure about this one.
> Any idea whether these need of_node_get? In all cases the device node
> value comes in as a parameter.
I'm really not an expert on this stuff. I just saw a potential problem
that I happen to be looking at in other subsystems, and I wanted to know
what others thought. I think this discussion should include the DT folks
and the subsystems in question. For one, I'm as interested as anyone in
getting this todo clarified:
Documentation/devicetree/todo.txt
- Document node lifecycle for CONFIG_OF_DYNAMIC
Regards,
Brian
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
On Wed, 18 Nov 2015, Brian Norris wrote:
> (changing subject, add [email protected])
>
> On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote:
> > On Tue, 17 Nov 2015, Brian Norris wrote:
> > > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote:
> > > > Is this something that should be checked for elsewhere?
> > >
> > > I expect the same sort of problem shows up plenty of other places. I
> > > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these
> > > failures probably aren't felt by many.
> >
> > I tried the following semantic patch:
> >
> > @@
> > struct device_node *e;
> > expression e1;
> > identifier fld;
> > @@
> >
> > ... when != of_node_get(...)
> > *(<+...e1->fld...+>) = e
> > ... when != of_node_get(...)
> > return e1;
> >
> > basically, this says that a structure field is initilized to a device node
> > value, the structure is returned by the containing function, and the
> > containing function contains no of_node_get at all. Certainly this is
> > quite constrained, but it does produce a number of examples.
> >
> > I looked at a few of them:
> >
> > drivers/clk/ingenic/cgu.c, ingenic_cgu_new
> > clk/pistachio/clk.c, pistachio_clk_alloc_provider
>
> It looks like the clock core (drivers/clk/clk.c) initially grabs the clk
> provider node in of_clk_init(), then drops it after it's initialized,
> but most of these providers use of_clk_add_provider(), which seems to
> manage the device_node lifetime for the user. So I think these are OK.
>
> > drivers/mfd/syscon.c, of_syscon_register
>
> This one looks potentially suspect. Syscon nodes aren't usually directly
> managed by a single driver, and the device_node pointer is used for
> lookups later...so I think it should keep a kref, and it doesn't.
>
> > drivers/of/pdt.c, function of_pdt_create_node
>
> Not real sure about this one.
>
> > Any idea whether these need of_node_get? In all cases the device node
> > value comes in as a parameter.
>
> I'm really not an expert on this stuff. I just saw a potential problem
> that I happen to be looking at in other subsystems, and I wanted to know
> what others thought.
Thanks for the analysis. I will look into them a bit more. Hopefully at
least the maintainer of each file will know what should be done.
julia
> I think this discussion should include the DT folks
> and the subsystems in question. For one, I'm as interested as anyone in
> getting this todo clarified:
>
> Documentation/devicetree/todo.txt
> - Document node lifecycle for CONFIG_OF_DYNAMIC
>
> Regards,
> Brian
>
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 <[email protected]>
After
Reviewed-by: Heiko Stuebner <[email protected]>
on a rk3288-veyron chromebook
Tested-by: Heiko Stuebner <[email protected]>
>
> ---
> 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[] = {
On Wed, Nov 18, 2015 at 1:05 PM, Brian Norris
<[email protected]> wrote:
> (changing subject, add [email protected])
>
> On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote:
>> On Tue, 17 Nov 2015, Brian Norris wrote:
>> > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote:
>> > > Is this something that should be checked for elsewhere?
>> >
>> > I expect the same sort of problem shows up plenty of other places. I
>> > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these
>> > failures probably aren't felt by many.
The "problem" is non-existent because either CONFIG_OF_DYNAMIC is off
or where it is used is limited (memory and cpus on PSeries) and now
overlays. Overlays have the potential to be problematic, but we should
manage ref counting for overlays in a completely different way. What
that looks like, I don't know. I'll leave that to the person that
cares about removing overlays.
>> basically, this says that a structure field is initilized to a device node
>> value, the structure is returned by the containing function, and the
>> containing function contains no of_node_get at all. Certainly this is
>> quite constrained, but it does produce a number of examples.
I've got no idea if this is right or not.
>> drivers/of/pdt.c, function of_pdt_create_node
>
> Not real sure about this one.
SPARC. Stay away.
>
>> Any idea whether these need of_node_get? In all cases the device node
>> value comes in as a parameter.
>
> I'm really not an expert on this stuff. I just saw a potential problem
> that I happen to be looking at in other subsystems, and I wanted to know
> what others thought. I think this discussion should include the DT folks
> and the subsystems in question. For one, I'm as interested as anyone in
> getting this todo clarified:
>
> Documentation/devicetree/todo.txt
> - Document node lifecycle for CONFIG_OF_DYNAMIC
Step 2 after figuring out it can't be documented is "define a new way
to handle dynamic DT refcounting aka how to get rid of
of_node_get/put."
Rob
On Thu, Nov 19, 2015 at 12:44:11PM -0600, Rob Herring wrote:
> On Wed, Nov 18, 2015 at 1:05 PM, Brian Norris
> <[email protected]> wrote:
> > (changing subject, add [email protected])
> >
> > On Tue, Nov 17, 2015 at 11:33:25PM +0100, Julia Lawall wrote:
> >> On Tue, 17 Nov 2015, Brian Norris wrote:
> >> > On Tue, Nov 17, 2015 at 06:48:39PM +0100, Julia Lawall wrote:
> >> > > Is this something that should be checked for elsewhere?
> >> >
> >> > I expect the same sort of problem shows up plenty of other places. I
> >> > don't think many people use CONFIG_OF_DYNAMIC, so the effects of these
> >> > failures probably aren't felt by many.
>
> The "problem" is non-existent because either CONFIG_OF_DYNAMIC is off
> or where it is used is limited (memory and cpus on PSeries) and now
> overlays. Overlays have the potential to be problematic, but we should
> manage ref counting for overlays in a completely different way. What
> that looks like, I don't know. I'll leave that to the person that
> cares about removing overlays.
So are you saying we should just forget about of_node_put and delete all
of_node_put/of_node_get references in code outside drivers/of ? That
seems pretty obtuse given that we do have the overlay code merged, and
sounds to me like a very bad idea.
Expecting those who want to use overlays to run around checking that
the refcounting is correct in drivers is a really silly idea IMHO -
the existing API is refcounted, so either people really ought to be
using it correctly as it's already been designed (in other words, with
correct refcounting, and we shouldn't be shovelling this problem onto
other people) or the refcounting should be completely killed.
The existing half-way house of "we have refcounting, but we don't care
about it" is really insane.
Either we have refcounting, and it's used properly, or we don't have
refcounting. No middle ground IMHO.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On 16.11.2015 12:33, Julia Lawall wrote:
> 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 <[email protected]>
Acked-by: Sebastian Hesselbarth <[email protected]>
Thanks!
> ---
> drivers/phy/phy-berlin-sata.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/phy/phy-berlin-sata.c b/drivers/phy/phy-berlin-sata.c
> index 77a2e05..f84a33a 100644
> --- a/drivers/phy/phy-berlin-sata.c
> +++ b/drivers/phy/phy-berlin-sata.c
> @@ -195,7 +195,7 @@ static int phy_berlin_sata_probe(struct platform_device *pdev)
> struct phy_provider *phy_provider;
> struct phy_berlin_priv *priv;
> struct resource *res;
> - int i = 0;
> + int ret, i = 0;
> u32 phy_id;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -237,22 +237,27 @@ static int phy_berlin_sata_probe(struct platform_device *pdev)
> if (of_property_read_u32(child, "reg", &phy_id)) {
> dev_err(dev, "missing reg property in node %s\n",
> child->name);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto put_child;
> }
>
> if (phy_id >= ARRAY_SIZE(phy_berlin_power_down_bits)) {
> dev_err(dev, "invalid reg in node %s\n", child->name);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto put_child;
> }
>
> phy_desc = devm_kzalloc(dev, sizeof(*phy_desc), GFP_KERNEL);
> - if (!phy_desc)
> - return -ENOMEM;
> + if (!phy_desc) {
> + ret = -ENOMEM;
> + goto put_child;
> + }
>
> phy = devm_phy_create(dev, NULL, &phy_berlin_sata_ops);
> if (IS_ERR(phy)) {
> dev_err(dev, "failed to create PHY %d\n", phy_id);
> - return PTR_ERR(phy);
> + ret = PTR_ERR(phy);
> + goto put_child;
> }
>
> phy_desc->phy = phy;
> @@ -269,6 +274,9 @@ static int phy_berlin_sata_probe(struct platform_device *pdev)
> phy_provider =
> devm_of_phy_provider_register(dev, phy_berlin_sata_phy_xlate);
> return PTR_ERR_OR_ZERO(phy_provider);
> +put_child:
> + of_node_put(child);
> + return ret;
> }
>
> static const struct of_device_id phy_berlin_sata_of_match[] = {
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
+Grant
Hi,
On Tuesday 17 November 2015 07:08 AM, Brian Norris wrote:
> On Mon, Nov 16, 2015 at 12:33:14PM +0100, Julia Lawall wrote:
>> 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 <[email protected]>
>>
>> ---
>
> For this patch:
>
> Acked-by: Brian Norris <[email protected]>
>
>> drivers/phy/phy-brcmstb-sata.c | 17 ++++++++++++-----
>> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> [snip patch, which fixes of_node_put() handling for
> for_each_available_child_of_node() loop, which creates PHY devices with
> devm_phy_create()]
>
> This reminds me of a potential problem I'm looking at in other
> subsystems: from code reading (I haven't seen any issues in practice,
> probably because I don't use OF_DYNAMIC) it looks like device-creating
> infrastructure like the PHY subsystem should be acquiring a reference to
> the device_node when they stash it away. But drivers/phy/phy-core.c does
> not do this, AFAICT.
>
> See phy_create(), which does
>
> phy->dev.of_node = node ?: dev->of_node;
>
> and later might reuse this of_node pointer, even though it never called
> of_node_get() on this node.
>
> Potential patch to fix this (not tested).
>
> Signed-off-by: Brian Norris <[email protected]>
>
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index fc48fac003a6..8df29caeeef9 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -697,6 +697,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> phy->dev.class = phy_class;
> phy->dev.parent = dev;
> phy->dev.of_node = node ?: dev->of_node;
> + of_node_get(phy->dev.of_node);
> phy->id = id;
> phy->ops = ops;
>
> @@ -726,6 +727,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
> return phy;
>
> put_dev:
> + of_node_put(phy->dev.of_node);
> put_device(&phy->dev); /* calls phy_release() which frees resources */
> return ERR_PTR(ret);
>
> @@ -775,6 +777,7 @@ EXPORT_SYMBOL_GPL(devm_phy_create);
> */
> void phy_destroy(struct phy *phy)
> {
> + of_node_put(phy->dev.of_node);
I think it's better to have this patch in phy-core though OF_DYNAMIC is not
enabled?
Grant,
Is it safe to assume of_node_get() will prevent "anyone else" from deleting the
node?
Here phy core uses the node pointer (passed to it by phy providers) and we
would like to avoid "anyone" from removing this node pointer resulting in phy
core having an invalid node pointer. Using of_node_get() in phy core should be
sufficient for this?
We are also interested in this todo tasklist for Devicetree..
"Document node lifecycle for CONFIG_OF_DYNAMIC"
Please find the complete thread of this mail chain here [1]
[1] -> http://www.gossamer-threads.com/lists/linux/kernel/2304857
Thanks
Kishon
> pm_runtime_disable(&phy->dev);
> device_unregister(&phy->dev);
> }
>