| Message ID | 20170821125107.20746-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Niklas, Niklas Söderlund wrote: > Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the > node being passed to of_fwnode_graph_get_port_parent(). Preserve the > usecount just like it is done in of_graph_get_port_parent(). The of_fwnode_graph_get_port_parent() is called by fwnode_graph_get_port_parent() which obtains the port node through fwnode_get_parent(). If you take a reference here, calling fwnode_graph_get_port_parent() will end up incrementing the port node's use count. In other words, my understanding is that dropping the reference to the port node isn't a problem but intended behaviour here. I wonder if I miss something. > > Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations") > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/of/property.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 067f9fab7b77c794..637dcb4833e2af60 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -922,6 +922,12 @@ of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) > { > struct device_node *np; > > + /* > + * Preserve usecount for passed in node as of_get_next_parent() > + * will do of_node_put() on it. > + */ > + of_node_get(to_of_node(fwnode)); > + > /* Get the parent of the port */ > np = of_get_next_parent(to_of_node(fwnode)); > if (!np) >
Hi Sakari, On 2017-08-21 16:30:17 +0300, Sakari Ailus wrote: > Hi Niklas, > > Niklas Söderlund wrote: > > Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the > > node being passed to of_fwnode_graph_get_port_parent(). Preserve the > > usecount just like it is done in of_graph_get_port_parent(). > > The of_fwnode_graph_get_port_parent() is called by > fwnode_graph_get_port_parent() which obtains the port node through > fwnode_get_parent(). If you take a reference here, calling > fwnode_graph_get_port_parent() will end up incrementing the port node's use > count. In other words, my understanding is that dropping the reference to > the port node isn't a problem but intended behaviour here. I'm not sure but I don't think the usecount will be incremented, without this patch I think it's decremented by one instead. Lets look at the code starting with fwnode_graph_get_port_parent(). struct fwnode_handle * fwnode_graph_get_port_parent(struct fwnode_handle *endpoint) { struct fwnode_handle *port, *parent; Increment usecount by 1 port = fwnode_get_parent(endpoint); parent = fwnode_call_ptr_op(port, graph_get_port_parent); Decrement usecount by 1 fwnode_handle_put(port); << Usecount -1 return parent; } Here it looks like the counting is correct and balanced. But without this patch it's in this function 'fwnode_handle_put(port)' which triggers the error which this patch aims to fix. Lets look at of_fwnode_graph_get_port_parent() which in my case is what is called by the fwnode_call_ptr_op(). static struct fwnode_handle * of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) { struct device_node *np; Here in of_get_next_parent() the usecount is decremented by 1 and the parents usecount is incremented by 1. So for our node node which passed in from fwnode_graph_get_port_parent() (where it's named 'port') will be decremented by 1. /* Get the parent of the port */ np = of_get_next_parent(to_of_node(fwnode)); if (!np) return NULL; /* Is this the "ports" node? If not, it's the port parent. */ if (of_node_cmp(np->name, "ports")) return of_fwnode_handle(np); return of_fwnode_handle(of_get_next_parent(np)); } So unless I miss something I do think this patch is needed to restore balance to the usecount of the node being passed to of_fwnode_graph_get_port_parent(). Or maybe I have misunderstood something? > > I wonder if I miss something. I also wonder what I missed :-) > > > > > Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations") > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/of/property.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index 067f9fab7b77c794..637dcb4833e2af60 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -922,6 +922,12 @@ of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) > > { > > struct device_node *np; > > > > + /* > > + * Preserve usecount for passed in node as of_get_next_parent() > > + * will do of_node_put() on it. > > + */ > > + of_node_get(to_of_node(fwnode)); > > + > > /* Get the parent of the port */ > > np = of_get_next_parent(to_of_node(fwnode)); > > if (!np) > > > > > -- > Sakari Ailus > sakari.ailus@linux.intel.com
Hi Niklas, On Mon, Aug 21, 2017 at 2:51 PM, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the > node being passed to of_fwnode_graph_get_port_parent(). Preserve the > usecount just like it is done in of_graph_get_port_parent(). > > Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations") > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/of/property.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 067f9fab7b77c794..637dcb4833e2af60 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -922,6 +922,12 @@ of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) > { > struct device_node *np; > > + /* > + * Preserve usecount for passed in node as of_get_next_parent() > + * will do of_node_put() on it. > + */ > + of_node_get(to_of_node(fwnode)); > + > /* Get the parent of the port */ > np = of_get_next_parent(to_of_node(fwnode)); > if (!np) FWIW, I'd use "np" to store the intermediate value: struct device_node *np = to_of_node(fwnode); /* * Preserve usecount for passed in node as of_get_next_parent() * will do of_node_put() on it. */ of_node_get(np); /* Get the parent of the port */ np = of_get_next_parent(np); Alternatively, perhaps to_of_node() should increment the refcount and call of_node_get()? Oh, there's (static) of_fwnode_get(), too. Is drivers/iommu/iommu.c:iommu_fwspec_init() really the only place outside drivers/of/property.c that calls of_node_get() on a node obtained by to_of_node()? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Niklas, Niklas Söderlund wrote: > Hi Sakari, > > On 2017-08-21 16:30:17 +0300, Sakari Ailus wrote: >> Hi Niklas, >> >> Niklas Söderlund wrote: >>> Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the >>> node being passed to of_fwnode_graph_get_port_parent(). Preserve the >>> usecount just like it is done in of_graph_get_port_parent(). >> >> The of_fwnode_graph_get_port_parent() is called by >> fwnode_graph_get_port_parent() which obtains the port node through >> fwnode_get_parent(). If you take a reference here, calling >> fwnode_graph_get_port_parent() will end up incrementing the port node's use >> count. In other words, my understanding is that dropping the reference to >> the port node isn't a problem but intended behaviour here. > > I'm not sure but I don't think the usecount will be incremented, without > this patch I think it's decremented by one instead. Lets look at the > code starting with fwnode_graph_get_port_parent(). > > struct fwnode_handle * > fwnode_graph_get_port_parent(struct fwnode_handle *endpoint) > { > struct fwnode_handle *port, *parent; > > Increment usecount by 1 > > port = fwnode_get_parent(endpoint); > parent = fwnode_call_ptr_op(port, graph_get_port_parent); > > Decrement usecount by 1 > > fwnode_handle_put(port); << Usecount -1 Here it is; this is the one I missed. I spotted something else, too. Look at of_graph_get_port_parent(); it appears to decrement the use count of the node passed to it, too: struct device_node *of_graph_get_port_parent(struct device_node *node) { unsigned int depth; /* Walk 3 levels up only if there is 'ports' node. */ for (depth = 3; depth && node; depth--) { node = of_get_next_parent(node); if (depth == 2 && of_node_cmp(node->name, "ports")) break; } return node; } EXPORT_SYMBOL(of_graph_get_port_parent); I think you'd need to of_node_get(node) first. I think it'd be good to address this at the same time. One could claim the original design principle has truly been adopted in the fwnode variant of the function. X-) On your original patch --- could you replace of_get_next_parent() by of_get_parent()? In that case it won't drop the reference to the parent, i.e. does what's required. > > return parent; > } > > Here it looks like the counting is correct and balanced. But without > this patch it's in this function 'fwnode_handle_put(port)' which > triggers the error which this patch aims to fix. Lets look at > of_fwnode_graph_get_port_parent() which in my case is what is called by > the fwnode_call_ptr_op(). > > static struct fwnode_handle * > of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) > { > struct device_node *np; > > Here in of_get_next_parent() the usecount is decremented by 1 and the > parents usecount is incremented by 1. So for our node node which passed > in from fwnode_graph_get_port_parent() (where it's named 'port') will be > decremented by 1. > > /* Get the parent of the port */ > np = of_get_next_parent(to_of_node(fwnode)); > if (!np) > return NULL; > > /* Is this the "ports" node? If not, it's the port parent. */ > if (of_node_cmp(np->name, "ports")) > return of_fwnode_handle(np); > > return of_fwnode_handle(of_get_next_parent(np)); > } > > So unless I miss something I do think this patch is needed to restore > balance to the usecount of the node being passed to > of_fwnode_graph_get_port_parent(). Or maybe I have misunderstood > something? > >> >> I wonder if I miss something. > > I also wonder what I missed :-) > >> >>> >>> Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations") >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>> --- >>> drivers/of/property.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/of/property.c b/drivers/of/property.c >>> index 067f9fab7b77c794..637dcb4833e2af60 100644 >>> --- a/drivers/of/property.c >>> +++ b/drivers/of/property.c >>> @@ -922,6 +922,12 @@ of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) >>> { >>> struct device_node *np; >>> >>> + /* >>> + * Preserve usecount for passed in node as of_get_next_parent() >>> + * will do of_node_put() on it. >>> + */ >>> + of_node_get(to_of_node(fwnode)); >>> + >>> /* Get the parent of the port */ >>> np = of_get_next_parent(to_of_node(fwnode)); >>> if (!np) >>> >> >> >> -- >> Sakari Ailus >> sakari.ailus@linux.intel.com >
Hi Sakari, On 2017-08-21 22:03:02 +0300, Sakari Ailus wrote: > Hi Niklas, > > Niklas Söderlund wrote: > > Hi Sakari, > > > > On 2017-08-21 16:30:17 +0300, Sakari Ailus wrote: > > > Hi Niklas, > > > > > > Niklas Söderlund wrote: > > > > Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the > > > > node being passed to of_fwnode_graph_get_port_parent(). Preserve the > > > > usecount just like it is done in of_graph_get_port_parent(). > > > > > > The of_fwnode_graph_get_port_parent() is called by > > > fwnode_graph_get_port_parent() which obtains the port node through > > > fwnode_get_parent(). If you take a reference here, calling > > > fwnode_graph_get_port_parent() will end up incrementing the port node's use > > > count. In other words, my understanding is that dropping the reference to > > > the port node isn't a problem but intended behaviour here. > > > > I'm not sure but I don't think the usecount will be incremented, without > > this patch I think it's decremented by one instead. Lets look at the > > code starting with fwnode_graph_get_port_parent(). > > > > struct fwnode_handle * > > fwnode_graph_get_port_parent(struct fwnode_handle *endpoint) > > { > > struct fwnode_handle *port, *parent; > > > > Increment usecount by 1 > > > > port = fwnode_get_parent(endpoint); > > parent = fwnode_call_ptr_op(port, graph_get_port_parent); > > > > Decrement usecount by 1 > > > > fwnode_handle_put(port); << Usecount -1 > > Here it is; this is the one I missed. > > I spotted something else, too. Look at of_graph_get_port_parent(); it > appears to decrement the use count of the node passed to it, too: > > struct device_node *of_graph_get_port_parent(struct device_node *node) > { > unsigned int depth; > > /* Walk 3 levels up only if there is 'ports' node. */ > for (depth = 3; depth && node; depth--) { > node = of_get_next_parent(node); > if (depth == 2 && of_node_cmp(node->name, "ports")) > break; > } > return node; > } > EXPORT_SYMBOL(of_graph_get_port_parent); > > I think you'd need to of_node_get(node) first. I think it'd be good to > address this at the same time. Your tree is old :-) I did check of_graph_get_port_parent() when looking for how this was handled elsewhere in the kernel. But I did not realise that the fix was accepted after 4.13-rc1 so I did not mention that this was just a copy of that fix in the patch description. For reference see c0a480d1acf7dc18 ("device property: Fix usecount for of_graph_get_port_parent()") > > One could claim the original design principle has truly been adopted in the > fwnode variant of the function. X-) Yes and I adopted the same fix for the original :-) > > On your original patch --- could you replace of_get_next_parent() by > of_get_parent()? In that case it won't drop the reference to the parent, > i.e. does what's required. I do however think this is a much nicer solution. So I would still be inclined to send a v2 whit this change instead. Which solution would you prefer? > > > > > return parent; > > } > > > > Here it looks like the counting is correct and balanced. But without > > this patch it's in this function 'fwnode_handle_put(port)' which > > triggers the error which this patch aims to fix. Lets look at > > of_fwnode_graph_get_port_parent() which in my case is what is called by > > the fwnode_call_ptr_op(). > > > > static struct fwnode_handle * > > of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) > > { > > struct device_node *np; > > > > Here in of_get_next_parent() the usecount is decremented by 1 and the > > parents usecount is incremented by 1. So for our node node which passed > > in from fwnode_graph_get_port_parent() (where it's named 'port') will be > > decremented by 1. > > > > /* Get the parent of the port */ > > np = of_get_next_parent(to_of_node(fwnode)); > > if (!np) > > return NULL; > > > > /* Is this the "ports" node? If not, it's the port parent. */ > > if (of_node_cmp(np->name, "ports")) > > return of_fwnode_handle(np); > > > > return of_fwnode_handle(of_get_next_parent(np)); > > } > > > > So unless I miss something I do think this patch is needed to restore > > balance to the usecount of the node being passed to > > of_fwnode_graph_get_port_parent(). Or maybe I have misunderstood > > something? > > > > > > > > I wonder if I miss something. > > > > I also wonder what I missed :-) > > > > > > > > > > > > > Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations") > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > --- > > > > drivers/of/property.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > > index 067f9fab7b77c794..637dcb4833e2af60 100644 > > > > --- a/drivers/of/property.c > > > > +++ b/drivers/of/property.c > > > > @@ -922,6 +922,12 @@ of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) > > > > { > > > > struct device_node *np; > > > > > > > > + /* > > > > + * Preserve usecount for passed in node as of_get_next_parent() > > > > + * will do of_node_put() on it. > > > > + */ > > > > + of_node_get(to_of_node(fwnode)); > > > > + > > > > /* Get the parent of the port */ > > > > np = of_get_next_parent(to_of_node(fwnode)); > > > > if (!np) > > > > > > > > > > > > > -- > > > Sakari Ailus > > > sakari.ailus@linux.intel.com > > > > > -- > Sakari Ailus > sakari.ailus@linux.intel.com
Hejssan Niklas, Niklas Söderlund wrote: > Hi Sakari, > > On 2017-08-21 22:03:02 +0300, Sakari Ailus wrote: >> Hi Niklas, >> >> Niklas Söderlund wrote: >>> Hi Sakari, >>> >>> On 2017-08-21 16:30:17 +0300, Sakari Ailus wrote: >>>> Hi Niklas, >>>> >>>> Niklas Söderlund wrote: >>>>> Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the >>>>> node being passed to of_fwnode_graph_get_port_parent(). Preserve the >>>>> usecount just like it is done in of_graph_get_port_parent(). >>>> >>>> The of_fwnode_graph_get_port_parent() is called by >>>> fwnode_graph_get_port_parent() which obtains the port node through >>>> fwnode_get_parent(). If you take a reference here, calling >>>> fwnode_graph_get_port_parent() will end up incrementing the port node's use >>>> count. In other words, my understanding is that dropping the reference to >>>> the port node isn't a problem but intended behaviour here. >>> >>> I'm not sure but I don't think the usecount will be incremented, without >>> this patch I think it's decremented by one instead. Lets look at the >>> code starting with fwnode_graph_get_port_parent(). >>> >>> struct fwnode_handle * >>> fwnode_graph_get_port_parent(struct fwnode_handle *endpoint) >>> { >>> struct fwnode_handle *port, *parent; >>> >>> Increment usecount by 1 >>> >>> port = fwnode_get_parent(endpoint); >>> parent = fwnode_call_ptr_op(port, graph_get_port_parent); >>> >>> Decrement usecount by 1 >>> >>> fwnode_handle_put(port); << Usecount -1 >> >> Here it is; this is the one I missed. >> >> I spotted something else, too. Look at of_graph_get_port_parent(); it >> appears to decrement the use count of the node passed to it, too: >> >> struct device_node *of_graph_get_port_parent(struct device_node *node) >> { >> unsigned int depth; >> >> /* Walk 3 levels up only if there is 'ports' node. */ >> for (depth = 3; depth && node; depth--) { >> node = of_get_next_parent(node); >> if (depth == 2 && of_node_cmp(node->name, "ports")) >> break; >> } >> return node; >> } >> EXPORT_SYMBOL(of_graph_get_port_parent); >> >> I think you'd need to of_node_get(node) first. I think it'd be good to >> address this at the same time. > > Your tree is old :-) > > I did check of_graph_get_port_parent() when looking for how this was > handled elsewhere in the kernel. But I did not realise that the fix was > accepted after 4.13-rc1 so I did not mention that this was just a copy > of that fix in the patch description. For reference see > > c0a480d1acf7dc18 ("device property: Fix usecount for of_graph_get_port_parent()") Ack, good. I didn't check new developments there, I have to admit. > >> >> One could claim the original design principle has truly been adopted in the >> fwnode variant of the function. X-) > > Yes and I adopted the same fix for the original :-) > >> >> On your original patch --- could you replace of_get_next_parent() by >> of_get_parent()? In that case it won't drop the reference to the parent, >> i.e. does what's required. > > I do however think this is a much nicer solution. So I would still be > inclined to send a v2 whit this change instead. Which solution would you > prefer? of_get_parent() is my preference; you can add to v2: Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> of_get_next_parent() is intended for cases where you expressly want to drop the reference AFAIK. Thanks! > >> >>> >>> return parent; >>> } >>> >>> Here it looks like the counting is correct and balanced. But without >>> this patch it's in this function 'fwnode_handle_put(port)' which >>> triggers the error which this patch aims to fix. Lets look at >>> of_fwnode_graph_get_port_parent() which in my case is what is called by >>> the fwnode_call_ptr_op(). >>> >>> static struct fwnode_handle * >>> of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) >>> { >>> struct device_node *np; >>> >>> Here in of_get_next_parent() the usecount is decremented by 1 and the >>> parents usecount is incremented by 1. So for our node node which passed >>> in from fwnode_graph_get_port_parent() (where it's named 'port') will be >>> decremented by 1. >>> >>> /* Get the parent of the port */ >>> np = of_get_next_parent(to_of_node(fwnode)); >>> if (!np) >>> return NULL; >>> >>> /* Is this the "ports" node? If not, it's the port parent. */ >>> if (of_node_cmp(np->name, "ports")) >>> return of_fwnode_handle(np); >>> >>> return of_fwnode_handle(of_get_next_parent(np)); >>> } >>> >>> So unless I miss something I do think this patch is needed to restore >>> balance to the usecount of the node being passed to >>> of_fwnode_graph_get_port_parent(). Or maybe I have misunderstood >>> something? >>> >>>> >>>> I wonder if I miss something. >>> >>> I also wonder what I missed :-) >>> >>>> >>>>> >>>>> Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations") >>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>>>> --- >>>>> drivers/of/property.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/drivers/of/property.c b/drivers/of/property.c >>>>> index 067f9fab7b77c794..637dcb4833e2af60 100644 >>>>> --- a/drivers/of/property.c >>>>> +++ b/drivers/of/property.c >>>>> @@ -922,6 +922,12 @@ of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) >>>>> { >>>>> struct device_node *np; >>>>> >>>>> + /* >>>>> + * Preserve usecount for passed in node as of_get_next_parent() >>>>> + * will do of_node_put() on it. >>>>> + */ >>>>> + of_node_get(to_of_node(fwnode)); >>>>> + >>>>> /* Get the parent of the port */ >>>>> np = of_get_next_parent(to_of_node(fwnode)); >>>>> if (!np) >>>>> >>>> >>>> >>>> -- >>>> Sakari Ailus >>>> sakari.ailus@linux.intel.com >>> >> >> >> -- >> Sakari Ailus >> sakari.ailus@linux.intel.com >
diff --git a/drivers/of/property.c b/drivers/of/property.c index 067f9fab7b77c794..637dcb4833e2af60 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -922,6 +922,12 @@ of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode) { struct device_node *np; + /* + * Preserve usecount for passed in node as of_get_next_parent() + * will do of_node_put() on it. + */ + of_node_get(to_of_node(fwnode)); + /* Get the parent of the port */ np = of_get_next_parent(to_of_node(fwnode)); if (!np)
Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the node being passed to of_fwnode_graph_get_port_parent(). Preserve the usecount just like it is done in of_graph_get_port_parent(). Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations") Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/of/property.c | 6 ++++++ 1 file changed, 6 insertions(+)