| Message ID | E1ZiRIs-0004Id-AN@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Russell, On Sat, 03 Oct 2015 19:13:02 +0100, Russell King wrote: > + ret = devm_add_action(dev, mvebu_pcie_port_clk_put, port); I didn't know about devm_add_action(). Definitely very useful for such situations. > @@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > struct mvebu_pcie_port *port = &pcie->ports[i]; > > ret = mvebu_pcie_parse_port(pcie, port, child); > - if (ret < 0) > + if (ret < 0) { > + of_node_put(child); > return ret; > - else if (ret == 0) > + } else if (ret == 0) { > continue; > + } This is not trivial. If I understand correctly, for_each_available_child_of_node() will automatically release the reference on the previous node and take the reference on the new one before entering the loop code. So in the skipping case, we don't need to release the reference as it will be done by the next iteration of the loop, but in the error case, since we are unexpectedly breaking the loop, we need to do it manually. The sort of tricky thing that should be documented near for_each_child_of_node(), since I believe a lot of code gets this wrong. See: http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367 http://lxr.free-electrons.com/source/drivers/phy/phy-miphy365x.c#L564 and many more. Thomas
On Fri, Oct 09, 2015 at 04:52:40PM +0200, Thomas Petazzoni wrote: > Russell, > > On Sat, 03 Oct 2015 19:13:02 +0100, Russell King wrote: > > > + ret = devm_add_action(dev, mvebu_pcie_port_clk_put, port); > > I didn't know about devm_add_action(). Definitely very useful for such > situations. I was also surprised by this. Nice. > > @@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > > struct mvebu_pcie_port *port = &pcie->ports[i]; > > > > ret = mvebu_pcie_parse_port(pcie, port, child); > > - if (ret < 0) > > + if (ret < 0) { > > + of_node_put(child); > > return ret; > > - else if (ret == 0) > > + } else if (ret == 0) { > > continue; > > + } > > This is not trivial. If I understand correctly, > for_each_available_child_of_node() will automatically release the > reference on the previous node and take the reference on the new one > before entering the loop code. So in the skipping case, we don't need > to release the reference as it will be done by the next iteration of > the loop, but in the error case, since we are unexpectedly breaking the > loop, we need to do it manually. > > The sort of tricky thing that should be documented near > for_each_child_of_node(), since I believe a lot of code gets this > wrong. Sounds like a good candidate for a coccinelle script. Maybe you could ask Julia Lawall? Andrew
On Fri, Oct 09, 2015 at 04:52:40PM +0200, Thomas Petazzoni wrote: > Russell, > > On Sat, 03 Oct 2015 19:13:02 +0100, Russell King wrote: > > > + ret = devm_add_action(dev, mvebu_pcie_port_clk_put, port); > > I didn't know about devm_add_action(). Definitely very useful for such > situations. > > > @@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > > struct mvebu_pcie_port *port = &pcie->ports[i]; > > > > ret = mvebu_pcie_parse_port(pcie, port, child); > > - if (ret < 0) > > + if (ret < 0) { > > + of_node_put(child); > > return ret; > > - else if (ret == 0) > > + } else if (ret == 0) { > > continue; > > + } > > This is not trivial. If I understand correctly, > for_each_available_child_of_node() will automatically release the > reference on the previous node and take the reference on the new one > before entering the loop code. Yes. > So in the skipping case, we don't need > to release the reference as it will be done by the next iteration of > the loop, but in the error case, since we are unexpectedly breaking the > loop, we need to do it manually. Correct. > The sort of tricky thing that should be documented near > for_each_child_of_node(), since I believe a lot of code gets this > wrong. Yes, lots of code gets it wrong, even the core DT code gets it wrong. The problem will be stopping people getting it wrong in the future, now that they've learnt the wrong way to do it - I don't think documenting the site where for_each_child_of_node() is defined will work anymore - how many people know of for_each_child_of_node() and don't need to look it up, yet code exactly this bug? TBH, the fundamental issue here is wrapping this all up in what looks to be a harmless for_each_child_of_node() thing. for_each_child_of_node() has side-effects that are completely non-obvious, except to those who have been educated about it. I think the only solution now is to kill for_each_child_of_node() and similar, and have people open-code the for() loop so they can _see_ what's going on, because we can't stop people from breaking out of a for() loop. That's a major change though, outside the scope of this series. I raised the issue last month with DT people, but there isn't any interest in fixing the issue; they know about it but (iirc) have no plans to fix it.
Hello (Julia added in the list of recipients). On Fri, 9 Oct 2015 17:07:10 +0200, Andrew Lunn wrote: > > > @@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > > > struct mvebu_pcie_port *port = &pcie->ports[i]; > > > > > > ret = mvebu_pcie_parse_port(pcie, port, child); > > > - if (ret < 0) > > > + if (ret < 0) { > > > + of_node_put(child); > > > return ret; > > > - else if (ret == 0) > > > + } else if (ret == 0) { > > > continue; > > > + } > > > > This is not trivial. If I understand correctly, > > for_each_available_child_of_node() will automatically release the > > reference on the previous node and take the reference on the new one > > before entering the loop code. So in the skipping case, we don't need > > to release the reference as it will be done by the next iteration of > > the loop, but in the error case, since we are unexpectedly breaking the > > loop, we need to do it manually. > > > > The sort of tricky thing that should be documented near > > for_each_child_of_node(), since I believe a lot of code gets this > > wrong. > > Sounds like a good candidate for a coccinelle script. Maybe you could > ask Julia Lawall? Julia: some of the Device Tree iterator functions have a somewhat "interesting" behavior in that they take the reference on the current child when entering the loop, and release it before entering the next iteration (which will also take a reference to the next child). This means that if you have an exit point inside the loop (break or return), you are loosing a reference. For example, http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367 is wrong because there is a missing of_node_put(np) before the "return -ENOMEM". So essentially, every exit point in such Device Tree iteration loops should make to call of_node_put() on the current element before exiting the loop unexpectedly. As Andrew suggested, this is probably something a Coccinelle semantic patch could easily detect and probably fix automatically. Thanks, Thomas
On Fri, Oct 09, 2015 at 05:14:42PM +0200, Thomas Petazzoni wrote: > Julia: some of the Device Tree iterator functions have a somewhat > "interesting" behavior in that they take the reference on the current > child when entering the loop, and release it before entering the next > iteration (which will also take a reference to the next child). > > This means that if you have an exit point inside the loop (break or > return), you are loosing a reference. For example, > http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367 > is wrong because there is a missing of_node_put(np) before the "return > -ENOMEM". > > So essentially, every exit point in such Device Tree iteration loops > should make to call of_node_put() on the current element before exiting > the loop unexpectedly. There is an exception to that... if the loop is part of a DT node lookup function, which would return a DT node based on some search criteria, it would be valid to break out of the loop while holding a reference. For example, of_get_child_by_name(), of_find_next_cache_node(), of_graph_get_port_by_id(), are valid breaker-outer cases. ;) However, of_platform_bus_probe(), of_platform_populate(), of_overlay_apply_one(), overlay_subtree_check() are examples of incorrect break-out cases.
On Fri, 9 Oct 2015, Thomas Petazzoni wrote: > Hello > > (Julia added in the list of recipients). > > On Fri, 9 Oct 2015 17:07:10 +0200, Andrew Lunn wrote: > > > > > @@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > > > > struct mvebu_pcie_port *port = &pcie->ports[i]; > > > > > > > > ret = mvebu_pcie_parse_port(pcie, port, child); > > > > - if (ret < 0) > > > > + if (ret < 0) { > > > > + of_node_put(child); > > > > return ret; > > > > - else if (ret == 0) > > > > + } else if (ret == 0) { > > > > continue; > > > > + } > > > > > > This is not trivial. If I understand correctly, > > > for_each_available_child_of_node() will automatically release the > > > reference on the previous node and take the reference on the new one > > > before entering the loop code. So in the skipping case, we don't need > > > to release the reference as it will be done by the next iteration of > > > the loop, but in the error case, since we are unexpectedly breaking the > > > loop, we need to do it manually. > > > > > > The sort of tricky thing that should be documented near > > > for_each_child_of_node(), since I believe a lot of code gets this > > > wrong. > > > > Sounds like a good candidate for a coccinelle script. Maybe you could > > ask Julia Lawall? > > Julia: some of the Device Tree iterator functions have a somewhat > "interesting" behavior in that they take the reference on the current > child when entering the loop, and release it before entering the next > iteration (which will also take a reference to the next child). > > This means that if you have an exit point inside the loop (break or > return), you are loosing a reference. For example, > http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367 > is wrong because there is a missing of_node_put(np) before the "return > -ENOMEM". > > So essentially, every exit point in such Device Tree iteration loops > should make to call of_node_put() on the current element before exiting > the loop unexpectedly. > > As Andrew suggested, this is probably something a Coccinelle semantic > patch could easily detect and probably fix automatically. Yeah, I looked at this along time ago, including the interesting loop behavior. I'll check on it again. Thanks for the pointer. julia
On Fri, 9 Oct 2015, Russell King - ARM Linux wrote: > On Fri, Oct 09, 2015 at 05:14:42PM +0200, Thomas Petazzoni wrote: > > Julia: some of the Device Tree iterator functions have a somewhat > > "interesting" behavior in that they take the reference on the current > > child when entering the loop, and release it before entering the next > > iteration (which will also take a reference to the next child). > > > > This means that if you have an exit point inside the loop (break or > > return), you are loosing a reference. For example, > > http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367 > > is wrong because there is a missing of_node_put(np) before the "return > > -ENOMEM". > > > > So essentially, every exit point in such Device Tree iteration loops > > should make to call of_node_put() on the current element before exiting > > the loop unexpectedly. > > There is an exception to that... if the loop is part of a DT node lookup > function, which would return a DT node based on some search criteria, it > would be valid to break out of the loop while holding a reference. > > For example, of_get_child_by_name(), of_find_next_cache_node(), > of_graph_get_port_by_id(), are valid breaker-outer cases. ;) > > However, of_platform_bus_probe(), of_platform_populate(), > of_overlay_apply_one(), overlay_subtree_check() are examples of > incorrect break-out cases. Thanks for the examples. I guess that this incorrect too, for the opposite reason (double put)? drivers/clk/tegra/clk-emc.c: for_each_child_of_node(np, node) { err = of_property_read_u32(node, "nvidia,ram-code", &node_ram_code); if (err) { of_node_put(node); continue; } julia
On Fri, Oct 09, 2015 at 06:39:47PM +0200, Julia Lawall wrote: > I guess that this incorrect too, for the opposite reason (double put)? > > drivers/clk/tegra/clk-emc.c: > > for_each_child_of_node(np, node) { > err = of_property_read_u32(node, "nvidia,ram-code", > &node_ram_code); > if (err) { > of_node_put(node); > continue; > } Oh yes, most definitely. Nice find!
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index 13ab0350f7fb..e8c51bb58e99 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -928,6 +928,13 @@ static int mvebu_pcie_resume(struct device *dev) return 0; } +static void mvebu_pcie_port_clk_put(void *data) +{ + struct mvebu_pcie_port *port = data; + + clk_put(port->clk); +} + static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, struct mvebu_pcie_port *port, struct device_node *child) { @@ -946,7 +953,12 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, if (of_property_read_u32(child, "marvell,pcie-lane", &port->lane)) port->lane = 0; - port->name = kasprintf(GFP_KERNEL, "pcie%d.%d", port->port, port->lane); + port->name = devm_kasprintf(dev, GFP_KERNEL, "pcie%d.%d", port->port, + port->lane); + if (!port->name) { + ret = -ENOMEM; + goto err; + } port->devfn = of_pci_get_devfn(child); if (port->devfn < 0) @@ -960,20 +972,29 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, goto skip; } - if (resource_size(&pcie->io) != 0) + if (resource_size(&pcie->io) != 0) { mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_IO, &port->io_target, &port->io_attr); - else { + } else { port->io_target = -1; port->io_attr = -1; } port->reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags); + if (port->reset_gpio == -EPROBE_DEFER) { + ret = port->reset_gpio; + goto err; + } + if (gpio_is_valid(port->reset_gpio)) { port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW; - port->reset_name = kasprintf(GFP_KERNEL, "%s-reset", - port->name); + port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset", + port->name); + if (!port->reset_name) { + ret = -ENOMEM; + goto err; + } ret = devm_gpio_request_one(dev, port->reset_gpio, GPIOF_DIR_OUT, port->reset_name); @@ -990,10 +1011,23 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, goto skip; } + ret = devm_add_action(dev, mvebu_pcie_port_clk_put, port); + if (ret < 0) { + clk_put(port->clk); + goto err; + } + return 1; skip: ret = 0; + + /* In the case of skipping, we need to free these */ + devm_kfree(dev, port->reset_name); + port->reset_name = NULL; + devm_kfree(dev, port->name); + port->name = NULL; + err: return ret; } @@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev) struct mvebu_pcie_port *port = &pcie->ports[i]; ret = mvebu_pcie_parse_port(pcie, port, child); - if (ret < 0) + if (ret < 0) { + of_node_put(child); return ret; - else if (ret == 0) + } else if (ret == 0) { continue; + } if (gpio_is_valid(port->reset_gpio)) { u32 reset_udelay = 20000;
The mvebu PCI port parsing is weak due to: 1) allocations via kasprintf() were not cleaned up when we encounter an error or decide to skip the port. 2) kasprintf() wasn't checked for failure. 3) of_get_named_gpio_flags() returns EPROBE_DEFER if the GPIO is not present, not devm_gpio_request_one(). 4) the of_node was not being put when terminating the loop. Fix these oversights. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/pci/host/pci-mvebu.c | 50 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 7 deletions(-)