| Message ID | 1427707141-3152-1-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 30-03-15 11:19, Ulf Hansson wrote: > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/mmc/core/core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index c296bc0..e6b0bdb 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct mmc_host *host, > for_each_child_of_node(host->parent->of_node, node) { > if (mmc_of_get_func_num(node) == func_num) > return node; > + else > + of_node_put(node); > } > > return NULL; > I don't think this is right, non of the other users of for_each_child_of_node do this, I think that rather then doing this we should be changing the callers of mmc_of_find_child_device to do: of_node_get(), except for the call which my "mmc: Add support for marking hpi as broken through devicetree" patch adds, as that is intended to only take a temporary reference. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30 March 2015 at 15:37, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 30-03-15 11:19, Ulf Hansson wrote: >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/mmc/core/core.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index c296bc0..e6b0bdb 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct >> mmc_host *host, >> for_each_child_of_node(host->parent->of_node, node) { >> if (mmc_of_get_func_num(node) == func_num) >> return node; >> + else >> + of_node_put(node); >> } >> >> return NULL; >> > > I don't think this is right, non of the other users of > for_each_child_of_node > do this, I think that rather then doing this we should be changing the > callers So, everybody don't follow the API. Cool. :-) > of mmc_of_find_child_device to do: of_node_get(), except for the call which > my "mmc: Add support for marking hpi as broken through devicetree" patch > adds, > as that is intended to only take a temporary reference. > In principle you are saying that the implementation of for_each_child_of_node() API needs to be adopted for how users actually use it, which means leave the of_node_get|put() to be done entirely by the caller, right? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 30-03-15 17:09, Ulf Hansson wrote: > On 30 March 2015 at 15:37, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> >> On 30-03-15 11:19, Ulf Hansson wrote: >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> --- >>> drivers/mmc/core/core.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>> index c296bc0..e6b0bdb 100644 >>> --- a/drivers/mmc/core/core.c >>> +++ b/drivers/mmc/core/core.c >>> @@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct >>> mmc_host *host, >>> for_each_child_of_node(host->parent->of_node, node) { >>> if (mmc_of_get_func_num(node) == func_num) >>> return node; >>> + else >>> + of_node_put(node); >>> } >>> >>> return NULL; >>> >> >> I don't think this is right, non of the other users of >> for_each_child_of_node >> do this, I think that rather then doing this we should be changing the >> callers > > So, everybody don't follow the API. Cool. :-) Hmm, I don't know where (if anywhere) the API is specified, but if I look at the actual implementation in include/linux/of.h #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)) And in drivers/of/base.c: static struct device_node *__of_get_next_child(const struct device_node *node, struct device_node *prev) { struct device_node *next; if (!node) return NULL; next = prev ? prev->sibling : node->child; for (; next; next = next->sibling) if (of_node_get(next)) break; of_node_put(prev); return next; } (the non __ prefixed version takes a lock then calls into this one) Note the "of_node_put(prev);" in the of_get_next_child implementation, so yes we've a ref while going through the loop, but its gets freed on the "increment" part of the for. Also see e.g. include/linux/of.h : static inline int of_get_child_count(const struct device_node *np) { struct device_node *child; int num = 0; for_each_child_of_node(np, child) num++; return num; } Which I would expect to get things right. >> of mmc_of_find_child_device to do: of_node_get(), except for the call which >> my "mmc: Add support for marking hpi as broken through devicetree" patch >> adds, >> as that is intended to only take a temporary reference. >> > > In principle you are saying that the implementation of > for_each_child_of_node() API needs to be adopted for how users > actually use it, which means leave the of_node_get|put() to be done > entirely by the caller, right? What I'm saying is that, if I'm not reading the code the wrong way, that is already how the for_each_child_of_node() API works. As for the mmc subsys it seems that means that no changes are necessary, since we do: for_each_child_of_node(host->parent->of_node, node) { if (mmc_of_get_func_num(node) == func_num) return node; } So when we've found the right node, we jump out of the loop, returning the reference we have while in the loop. This does mean that my: "mmc: Add support for marking hpi as broken through devicetree" patch needs to be changed as I ended up calling mmc_of_find_child_device twice in there, since in that patch I need the of_node before mmc_add_card() gets called, so I'm leaking a reference there. I'll do a v2 fixing this. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31 March 2015 at 09:06, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 30-03-15 17:09, Ulf Hansson wrote: >> >> On 30 March 2015 at 15:37, Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi, >>> >>> >>> On 30-03-15 11:19, Ulf Hansson wrote: >>>> >>>> >>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>>> --- >>>> drivers/mmc/core/core.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>>> index c296bc0..e6b0bdb 100644 >>>> --- a/drivers/mmc/core/core.c >>>> +++ b/drivers/mmc/core/core.c >>>> @@ -1291,6 +1291,8 @@ struct device_node >>>> *mmc_of_find_child_device(struct >>>> mmc_host *host, >>>> for_each_child_of_node(host->parent->of_node, node) { >>>> if (mmc_of_get_func_num(node) == func_num) >>>> return node; >>>> + else >>>> + of_node_put(node); >>>> } >>>> >>>> return NULL; >>>> >>> >>> I don't think this is right, non of the other users of >>> for_each_child_of_node >>> do this, I think that rather then doing this we should be changing the >>> callers >> >> >> So, everybody don't follow the API. Cool. :-) > > > Hmm, I don't know where (if anywhere) the API is specified, but if I look at > the actual implementation in include/linux/of.h > > #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)) > > And in drivers/of/base.c: > > static struct device_node *__of_get_next_child(const struct device_node > *node, > struct device_node *prev) > { > struct device_node *next; > > if (!node) > return NULL; > > next = prev ? prev->sibling : node->child; > for (; next; next = next->sibling) > if (of_node_get(next)) > break; > of_node_put(prev); > return next; > } > > (the non __ prefixed version takes a lock then calls into this one) > > Note the "of_node_put(prev);" in the of_get_next_child implementation, > so yes we've a ref while going through the loop, but its gets freed > on the "increment" part of the for. > > Also see e.g. include/linux/of.h : > > static inline int of_get_child_count(const struct device_node *np) > { > struct device_node *child; > int num = 0; > > for_each_child_of_node(np, child) > num++; > > return num; > } > > Which I would expect to get things right. Agree. I didn't look at the code carefully enough. My patch is wrong! > >>> of mmc_of_find_child_device to do: of_node_get(), except for the call >>> which >>> my "mmc: Add support for marking hpi as broken through devicetree" patch >>> adds, >>> as that is intended to only take a temporary reference. >>> >> >> In principle you are saying that the implementation of >> for_each_child_of_node() API needs to be adopted for how users >> actually use it, which means leave the of_node_get|put() to be done >> entirely by the caller, right? > > > What I'm saying is that, if I'm not reading the code the wrong way, that > is already how the for_each_child_of_node() API works. > > As for the mmc subsys it seems that means that no changes are necessary, > since we do: > > for_each_child_of_node(host->parent->of_node, node) { > if (mmc_of_get_func_num(node) == func_num) > return node; > } > > So when we've found the right node, we jump out of the loop, returning > the reference we have while in the loop. > > This does mean that my: "mmc: Add support for marking hpi as broken through > devicetree" > patch needs to be changed as I ended up calling mmc_of_find_child_device > twice > in there, since in that patch I need the of_node before mmc_add_card() gets > called, > so I'm leaking a reference there. I'll do a v2 fixing this. So this discussion around $subject patch, turned out to have some valuable outcome anyway. :-) Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index c296bc0..e6b0bdb 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct mmc_host *host, for_each_child_of_node(host->parent->of_node, node) { if (mmc_of_get_func_num(node) == func_num) return node; + else + of_node_put(node); } return NULL;
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/core/core.c | 2 ++ 1 file changed, 2 insertions(+)