Messages in this thread | | | Subject | Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint() | From | Yang Yingliang <> | Date | Fri, 25 Nov 2022 17:49:11 +0800 |
| |
On 2022/11/25 17:32, Daniel Scally wrote: > Hello > > On 23/11/2022 13:31, Andy Shevchenko wrote: >> On Wed, Nov 23, 2022 at 10:25:42AM +0800, Yang Yingliang wrote: >>> The 'parent' returned by fwnode_graph_get_port_parent() >>> with refcount incremented when 'prev' is not NULL, it >>> needs be put when finish using it. >>> >>> Because the parent is const, introduce a new variable to >>> store the returned fwnode, then put it before returning >>> from fwnode_graph_get_next_endpoint(). >> To me this looks good enough. Not sure if Dan has a chance (time) to look at >> this, though. And maybe even test... >> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Apologies; didn't notice this earlier. I will look at and test this today
Thanks,
I tested it, without this patch, I got this message: OF: ERROR: memory leak, expected refcount 1 instead of 4, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node /i2c/pmic@34/tcpc/connector after this patch, the message is gone. > >>> Fixes: b5b41ab6b0c1 ("device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()") >>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>> --- >>> v2 -> v3: >>> Add a out label. >>> >>> v1 -> v2: >>> Introduce a new variable to store the returned fwnode. >>> --- >>> drivers/base/property.c | 18 ++++++++++++------ >>> 1 file changed, 12 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/base/property.c b/drivers/base/property.c >>> index 2a5a37fcd998..7f338cb4fb7b 100644 >>> --- a/drivers/base/property.c >>> +++ b/drivers/base/property.c >>> @@ -989,26 +989,32 @@ struct fwnode_handle * >>> fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode, >>> struct fwnode_handle *prev) >>> { >>> + struct fwnode_handle *ep, *port_parent = NULL; >>> const struct fwnode_handle *parent; >>> - struct fwnode_handle *ep; >>> >>> /* >>> * If this function is in a loop and the previous iteration returned >>> * an endpoint from fwnode->secondary, then we need to use the secondary >>> * as parent rather than @fwnode. >>> */ >>> - if (prev) >>> - parent = fwnode_graph_get_port_parent(prev); >>> - else >>> + if (prev) { >>> + port_parent = fwnode_graph_get_port_parent(prev); >>> + parent = port_parent; >>> + } else { >>> parent = fwnode; >>> + } >>> if (IS_ERR_OR_NULL(parent)) >>> return NULL; >>> >>> ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev); >>> if (ep) >>> - return ep; >>> + goto out_put_port_parent; >>> + >>> + ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL); >>> >>> - return fwnode_graph_get_next_endpoint(parent->secondary, NULL); >>> +out_put_port_parent: >>> + fwnode_handle_put(port_parent); >>> + return ep; >>> } >>> EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint); >>> >>> -- >>> 2.25.1 >>> > .
| |