On Tue, Sep 1, 2015 at 6:07 AM, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> Consider the following loop:
>
> for_each_child_of_node(&pdev->dev.of_node, child) {
> if (some_condition)
> break;
> }
>
> The use of for_each_..._of_node() leads people to believe that it's
> like other for_each_...() loops - the continue and break statements
> can be used.
>
> However, with OF, "break" can't be used without disrupting the
> reference counting on the nodes. This is because:
>
> #define for_each_child_of_node(parent, child) \
> for (child = of_get_next_child(parent, NULL); child != NULL; \
> child = of_get_next_child(parent, child))
>
> of_get_next_child() takes a reference on the node it's about to return,
> while dropping the reference on the node passed into it. In the case
> of the last iteration, where of_get_next_child() returns NULL, the
> previous child will have its reference dropped, resulting in no child
> nodes having a reference held.
>
> However, if a 'break' statement is used, the reference on the current
> child is not dropped unless code explicitly drops it.
>
> We have code which does exactly this kind of thing:
>
> for_each_child_of_node(cpus, cpu) {
> /*
> * A device tree containing CPU nodes with missing "reg"
> * properties is considered invalid to build the
> * cpu_logical_map.
> */
> if (of_property_read_u32(cpu, "reg", &hwid)) {
> pr_debug(" * %s missing reg property\n",
> cpu->full_name);
> return;
> }
>
> /*
> * 8 MSBs must be set to 0 in the DT since the reg property
> * defines the MPIDR[23:0].
> */
> if (hwid & ~MPIDR_HWID_BITMASK)
> return;
> ... more return statements
>
> for_each_child_of_node(np, np0) {
> struct device_node *fc;
> int i;
>
> res = of_dev_hwmod_lookup(np0, oh, &i, &fc);
> if (res == 0) {
> *found = fc;
> *index = i;
> return 0;
>
> for_each_child_of_node(parent, np) {
> pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> if (!pd)
> return -ENOMEM;
>
> Virtually _all_ uses of for_each_child_of_node() in the kernel today
> where the loop is terminated early leak a reference on the child node.
> Even some of the drivers/of code does it:
>
> ... for_each_child_of_node(root, child) {
> if (!of_match_node(matches, child))
> continue;
> rc = of_platform_bus_create(child, matches, NULL, parent, false);
> if (rc)
> break;
> }
>
> This pretty much shows the danger of using macros which hide details
> like this from the programmer - it leads to the assumption that it's
> fine to use 'break' and 'return' without any further consideration,
> because that's what you can do in standard C loops. The fact that
> these loops are actually more complex than that is hidden behind the
> macro, and thus gets forgotten.
>
> We could go around and fix all these sites, but that's not going to
> stop this continuing to happen into the future. So, fixing the
> existing bugs is not a fix at all, it's a papering over of a more
> fundamental problem here.
Yes, the ref counting for DT in general is difficult to get right and
needs to be redesigned. Geert did a checker and even the core and
unittests have 44 errors[1]. However, it is a nop in most cases, and
it only really matters on IBM pSeries and only for certain nodes on
those AIUI. We've had some discussions about it before, but no one has
come up with a solution. Managing this at a node level is probably too
fine grained when most nodes don't need ref counting. The implicit get
and explicit put are also a problem IMO. We need to be able to look at
code and see the calls are balanced.
Rob
[1] https://lkml.org/lkml/2015/1/23/437
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
