* [PATCH] [media] omap3isp: don't call of_node_put
@ 2016-09-08 15:48 H. Nikolaus Schaller
[not found] ` <D55EF0EA-F7B8-4DA7-8F2F-BCC6650D194F@goldelico.com>
2016-09-29 8:54 ` Laurent Pinchart
0 siblings, 2 replies; 6+ messages in thread
From: H. Nikolaus Schaller @ 2016-09-08 15:48 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab
Cc: linux-media, linux-kernel, Javier Martinez Canillas, arnd,
hans.verkuil, tony, letux-kernel, H. Nikolaus Schaller
of_node_put() has already been called inside of_graph_get_next_endpoint().
Otherwise we may get warnings like
[ 10.118286] omap3isp 480bc000.isp: parsing endpoint /ocp/isp@480bc000/ports/port@0/endpoint, interface 0
[ 10.118499] ERROR: Bad of_node_put() on /ocp/isp@480bc000/ports/port@0/endpoint
[ 10.118499] CPU: 0 PID: 968 Comm: udevd Not tainted 4.7.0-rc4-letux+ #376
[ 10.118530] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[ 10.118560] [<c010f0e0>] (unwind_backtrace) from [<c010b6d8>] (show_stack+0x10/0x14)
[ 10.118591] [<c010b6d8>] (show_stack) from [<c03ecc50>] (dump_stack+0x98/0xd0)
[ 10.118591] [<c03ecc50>] (dump_stack) from [<c03eecac>] (kobject_release+0x60/0x74)
[ 10.118621] [<c03eecac>] (kobject_release) from [<c05ab128>] (__of_get_next_child+0x40/0x48)
[ 10.118652] [<c05ab128>] (__of_get_next_child) from [<c05ab158>] (of_get_next_child+0x28/0x44)
[ 10.118652] [<c05ab158>] (of_get_next_child) from [<c05ab350>] (of_graph_get_next_endpoint+0xe4/0x124)
[ 10.118804] [<c05ab350>] (of_graph_get_next_endpoint) from [<bf1c88a4>] (isp_probe+0xdc/0xd80 [omap3_isp])
[ 10.118896] [<bf1c88a4>] (isp_probe [omap3_isp]) from [<c0482008>] (platform_drv_probe+0x50/0xa0)
[ 10.118927] [<c0482008>] (platform_drv_probe) from [<c04800e8>] (driver_probe_device+0x134/0x29c)
[ 10.118957] [<c04800e8>] (driver_probe_device) from [<c04802d8>] (__driver_attach+0x88/0xac)
[ 10.118957] [<c04802d8>] (__driver_attach) from [<c047e7b8>] (bus_for_each_dev+0x6c/0x90)
[ 10.118957] [<c047e7b8>] (bus_for_each_dev) from [<c047f798>] (bus_add_driver+0xcc/0x1e8)
[ 10.118988] [<c047f798>] (bus_add_driver) from [<c0481228>] (driver_register+0xac/0xf4)
[ 10.118988] [<c0481228>] (driver_register) from [<c010192c>] (do_one_initcall+0xac/0x154)
[ 10.119018] [<c010192c>] (do_one_initcall) from [<c02015bc>] (do_init_module+0x58/0x39c)
[ 10.119049] [<c02015bc>] (do_init_module) from [<c01bd314>] (load_module+0xe5c/0x1004)
[ 10.119049] [<c01bd314>] (load_module) from [<c01bd68c>] (SyS_finit_module+0x88/0x90)
[ 10.119079] [<c01bd68c>] (SyS_finit_module) from [<c0107040>] (ret_fast_syscall+0x0/0x1c)
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
drivers/media/platform/omap3isp/isp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 5d54e2c..6e2624e 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2114,7 +2114,6 @@ static int isp_of_parse_nodes(struct device *dev,
isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
if (!isd) {
- of_node_put(node);
return -ENOMEM;
}
@@ -2126,7 +2125,7 @@ static int isp_of_parse_nodes(struct device *dev,
}
isd->asd.match.of.node = of_graph_get_remote_port_parent(node);
- of_node_put(node);
+
if (!isd->asd.match.of.node) {
dev_warn(dev, "bad remote port parent\n");
return -EINVAL;
--
2.7.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] omap3isp: don't call of_node_put
[not found] ` <D55EF0EA-F7B8-4DA7-8F2F-BCC6650D194F@goldelico.com>
@ 2016-09-29 6:59 ` H. Nikolaus Schaller
0 siblings, 0 replies; 6+ messages in thread
From: H. Nikolaus Schaller @ 2016-09-29 6:59 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab
Cc: linux-media, linux-kernel, Javier Martinez Canillas, arnd,
hans.verkuil, tony, letux-kernel
ping ping.
> Am 19.09.2016 um 11:55 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>
> ping.
>
>> Am 08.09.2016 um 17:48 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>
>> of_node_put() has already been called inside of_graph_get_next_endpoint().
>>
>> Otherwise we may get warnings like
>>
>> [ 10.118286] omap3isp 480bc000.isp: parsing endpoint /ocp/isp@480bc000/ports/port@0/endpoint, interface 0
>> [ 10.118499] ERROR: Bad of_node_put() on /ocp/isp@480bc000/ports/port@0/endpoint
>> [ 10.118499] CPU: 0 PID: 968 Comm: udevd Not tainted 4.7.0-rc4-letux+ #376
>> [ 10.118530] Hardware name: Generic OMAP36xx (Flattened Device Tree)
>> [ 10.118560] [<c010f0e0>] (unwind_backtrace) from [<c010b6d8>] (show_stack+0x10/0x14)
>> [ 10.118591] [<c010b6d8>] (show_stack) from [<c03ecc50>] (dump_stack+0x98/0xd0)
>> [ 10.118591] [<c03ecc50>] (dump_stack) from [<c03eecac>] (kobject_release+0x60/0x74)
>> [ 10.118621] [<c03eecac>] (kobject_release) from [<c05ab128>] (__of_get_next_child+0x40/0x48)
>> [ 10.118652] [<c05ab128>] (__of_get_next_child) from [<c05ab158>] (of_get_next_child+0x28/0x44)
>> [ 10.118652] [<c05ab158>] (of_get_next_child) from [<c05ab350>] (of_graph_get_next_endpoint+0xe4/0x124)
>> [ 10.118804] [<c05ab350>] (of_graph_get_next_endpoint) from [<bf1c88a4>] (isp_probe+0xdc/0xd80 [omap3_isp])
>> [ 10.118896] [<bf1c88a4>] (isp_probe [omap3_isp]) from [<c0482008>] (platform_drv_probe+0x50/0xa0)
>> [ 10.118927] [<c0482008>] (platform_drv_probe) from [<c04800e8>] (driver_probe_device+0x134/0x29c)
>> [ 10.118957] [<c04800e8>] (driver_probe_device) from [<c04802d8>] (__driver_attach+0x88/0xac)
>> [ 10.118957] [<c04802d8>] (__driver_attach) from [<c047e7b8>] (bus_for_each_dev+0x6c/0x90)
>> [ 10.118957] [<c047e7b8>] (bus_for_each_dev) from [<c047f798>] (bus_add_driver+0xcc/0x1e8)
>> [ 10.118988] [<c047f798>] (bus_add_driver) from [<c0481228>] (driver_register+0xac/0xf4)
>> [ 10.118988] [<c0481228>] (driver_register) from [<c010192c>] (do_one_initcall+0xac/0x154)
>> [ 10.119018] [<c010192c>] (do_one_initcall) from [<c02015bc>] (do_init_module+0x58/0x39c)
>> [ 10.119049] [<c02015bc>] (do_init_module) from [<c01bd314>] (load_module+0xe5c/0x1004)
>> [ 10.119049] [<c01bd314>] (load_module) from [<c01bd68c>] (SyS_finit_module+0x88/0x90)
>> [ 10.119079] [<c01bd68c>] (SyS_finit_module) from [<c0107040>] (ret_fast_syscall+0x0/0x1c)
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/media/platform/omap3isp/isp.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
>> index 5d54e2c..6e2624e 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -2114,7 +2114,6 @@ static int isp_of_parse_nodes(struct device *dev,
>>
>> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
>> if (!isd) {
>> - of_node_put(node);
>> return -ENOMEM;
>> }
>>
>> @@ -2126,7 +2125,7 @@ static int isp_of_parse_nodes(struct device *dev,
>> }
>>
>> isd->asd.match.of.node = of_graph_get_remote_port_parent(node);
>> - of_node_put(node);
>> +
>> if (!isd->asd.match.of.node) {
>> dev_warn(dev, "bad remote port parent\n");
>> return -EINVAL;
>> --
>> 2.7.3
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] omap3isp: don't call of_node_put
2016-09-08 15:48 [PATCH] [media] omap3isp: don't call of_node_put H. Nikolaus Schaller
[not found] ` <D55EF0EA-F7B8-4DA7-8F2F-BCC6650D194F@goldelico.com>
@ 2016-09-29 8:54 ` Laurent Pinchart
2016-09-30 8:28 ` H. Nikolaus Schaller
1 sibling, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2016-09-29 8:54 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel,
Javier Martinez Canillas, arnd, hans.verkuil, tony, letux-kernel
Hi Nikolaus,
On Thursday 08 Sep 2016 17:48:33 H. Nikolaus Schaller wrote:
> of_node_put() has already been called inside of_graph_get_next_endpoint().
>
> Otherwise we may get warnings like
>
> [ 10.118286] omap3isp 480bc000.isp: parsing endpoint
> /ocp/isp@480bc000/ports/port@0/endpoint, interface 0 [ 10.118499] ERROR:
> Bad of_node_put() on /ocp/isp@480bc000/ports/port@0/endpoint [ 10.118499]
> CPU: 0 PID: 968 Comm: udevd Not tainted 4.7.0-rc4-letux+ #376 [
> 10.118530] Hardware name: Generic OMAP36xx (Flattened Device Tree) [
> 10.118560] [<c010f0e0>] (unwind_backtrace) from [<c010b6d8>]
> (show_stack+0x10/0x14) [ 10.118591] [<c010b6d8>] (show_stack) from
> [<c03ecc50>] (dump_stack+0x98/0xd0) [ 10.118591] [<c03ecc50>]
> (dump_stack) from [<c03eecac>] (kobject_release+0x60/0x74) [ 10.118621]
> [<c03eecac>] (kobject_release) from [<c05ab128>]
> (__of_get_next_child+0x40/0x48) [ 10.118652] [<c05ab128>]
> (__of_get_next_child) from [<c05ab158>] (of_get_next_child+0x28/0x44) [
> 10.118652] [<c05ab158>] (of_get_next_child) from [<c05ab350>]
> (of_graph_get_next_endpoint+0xe4/0x124) [ 10.118804] [<c05ab350>]
> (of_graph_get_next_endpoint) from [<bf1c88a4>] (isp_probe+0xdc/0xd80
> [omap3_isp]) [ 10.118896] [<bf1c88a4>] (isp_probe [omap3_isp]) from
> [<c0482008>] (platform_drv_probe+0x50/0xa0) [ 10.118927] [<c0482008>]
> (platform_drv_probe) from [<c04800e8>] (driver_probe_device+0x134/0x29c) [
> 10.118957] [<c04800e8>] (driver_probe_device) from [<c04802d8>]
> (__driver_attach+0x88/0xac) [ 10.118957] [<c04802d8>] (__driver_attach)
> from [<c047e7b8>] (bus_for_each_dev+0x6c/0x90) [ 10.118957] [<c047e7b8>]
> (bus_for_each_dev) from [<c047f798>] (bus_add_driver+0xcc/0x1e8) [
> 10.118988] [<c047f798>] (bus_add_driver) from [<c0481228>]
> (driver_register+0xac/0xf4) [ 10.118988] [<c0481228>] (driver_register)
> from [<c010192c>] (do_one_initcall+0xac/0x154) [ 10.119018] [<c010192c>]
> (do_one_initcall) from [<c02015bc>] (do_init_module+0x58/0x39c) [
> 10.119049] [<c02015bc>] (do_init_module) from [<c01bd314>]
> (load_module+0xe5c/0x1004) [ 10.119049] [<c01bd314>] (load_module) from
> [<c01bd68c>] (SyS_finit_module+0x88/0x90) [ 10.119079] [<c01bd68c>]
> (SyS_finit_module) from [<c0107040>] (ret_fast_syscall+0x0/0x1c)
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/media/platform/omap3isp/isp.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 5d54e2c..6e2624e 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2114,7 +2114,6 @@ static int isp_of_parse_nodes(struct device *dev,
>
> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> if (!isd) {
> - of_node_put(node);
I don't think this one is correct. Looking at the context
while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
(node = of_graph_get_next_endpoint(dev->of_node, node))) {
struct isp_async_subdev *isd;
isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
if (!isd) {
of_node_put(node);
return -ENOMEM;
}
notifier->subdevs[notifier->num_subdevs] = &isd->asd;
if (isp_of_parse_node(dev, node, isd)) {
of_node_put(node);
return -EINVAL;
}
isd->asd.match.of.node =
of_graph_get_remote_port_parent(node);
of_node_put(node);
if (!isd->asd.match.of.node) {
dev_warn(dev, "bad remote port parent\n");
return -EINVAL;
}
isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
notifier->num_subdevs++;
}
of_graph_get_next_endpoint() increases the reference count of the node it
returns, which needs a corresponding of_node_put() in the error paths. It
thus look like to me that the function isn't correct in that the
devm_kzalloc() and !isd->asd.match.of.node error paths.
> return -ENOMEM;
> }
>
> @@ -2126,7 +2125,7 @@ static int isp_of_parse_nodes(struct device *dev,
> }
>
> isd->asd.match.of.node = of_graph_get_remote_port_parent(node);
> - of_node_put(node);
> +
This change is correct not because of_graph_get_next_endpoint() has called
of_node_put() but because it *will* call it on the next iteration.
How about the following patch instead ?
commit 4ed9893bf52c90181c8d5b1ae29a37556b89f1da
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: Thu Sep 29 11:41:24 2016 +0300
omap3isp: Fix OF node double put when parsing OF graph
When parsing the graph the driver loops over all endpoints using
of_graph_get_next_endpoint(). The function handles reference counting of
the passed and returned nodes, so the returned node's reference count
must not be decreased manually in the normal path.
Move the offending of_node_put() call to the error path that requires
manual reference count handling.
Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 5e212668f726..f8b437cc8943 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2131,23 +2131,18 @@ static int isp_of_parse_nodes(struct device *dev,
struct isp_async_subdev *isd;
isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
- if (!isd) {
- of_node_put(node);
- return -ENOMEM;
- }
+ if (!isd)
+ goto error;
notifier->subdevs[notifier->num_subdevs] = &isd->asd;
- if (isp_of_parse_node(dev, node, isd)) {
- of_node_put(node);
- return -EINVAL;
- }
+ if (isp_of_parse_node(dev, node, isd))
+ goto error;
isd->asd.match.of.node = of_graph_get_remote_port_parent(node);
- of_node_put(node);
if (!isd->asd.match.of.node) {
dev_warn(dev, "bad remote port parent\n");
- return -EINVAL;
+ goto error;
}
isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
@@ -2155,6 +2150,10 @@ static int isp_of_parse_nodes(struct device *dev,
}
return notifier->num_subdevs;
+
+error:
+ of_node_put(node);
+ return -EINVAL;
}
static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> if (!isd->asd.match.of.node) {
> dev_warn(dev, "bad remote port parent\n");
> return -EINVAL;
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] omap3isp: don't call of_node_put
2016-09-29 8:54 ` Laurent Pinchart
@ 2016-09-30 8:28 ` H. Nikolaus Schaller
2016-09-30 8:52 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: H. Nikolaus Schaller @ 2016-09-30 8:28 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel,
Javier Martinez Canillas, arnd, hans.verkuil, tony, letux-kernel
Hi Laurent,
> Am 29.09.2016 um 10:54 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>
> Hi Nikolaus,
>
> On Thursday 08 Sep 2016 17:48:33 H. Nikolaus Schaller wrote:
>> of_node_put() has already been called inside of_graph_get_next_endpoint().
>>
>> Otherwise we may get warnings like
>>
>> [ 10.118286] omap3isp 480bc000.isp: parsing endpoint
>> /ocp/isp@480bc000/ports/port@0/endpoint, interface 0 [ 10.118499] ERROR:
>> Bad of_node_put() on /ocp/isp@480bc000/ports/port@0/endpoint [ 10.118499]
>> CPU: 0 PID: 968 Comm: udevd Not tainted 4.7.0-rc4-letux+ #376 [
>> 10.118530] Hardware name: Generic OMAP36xx (Flattened Device Tree) [
>> 10.118560] [<c010f0e0>] (unwind_backtrace) from [<c010b6d8>]
>> (show_stack+0x10/0x14) [ 10.118591] [<c010b6d8>] (show_stack) from
>> [<c03ecc50>] (dump_stack+0x98/0xd0) [ 10.118591] [<c03ecc50>]
>> (dump_stack) from [<c03eecac>] (kobject_release+0x60/0x74) [ 10.118621]
>> [<c03eecac>] (kobject_release) from [<c05ab128>]
>> (__of_get_next_child+0x40/0x48) [ 10.118652] [<c05ab128>]
>> (__of_get_next_child) from [<c05ab158>] (of_get_next_child+0x28/0x44) [
>> 10.118652] [<c05ab158>] (of_get_next_child) from [<c05ab350>]
>> (of_graph_get_next_endpoint+0xe4/0x124) [ 10.118804] [<c05ab350>]
>> (of_graph_get_next_endpoint) from [<bf1c88a4>] (isp_probe+0xdc/0xd80
>> [omap3_isp]) [ 10.118896] [<bf1c88a4>] (isp_probe [omap3_isp]) from
>> [<c0482008>] (platform_drv_probe+0x50/0xa0) [ 10.118927] [<c0482008>]
>> (platform_drv_probe) from [<c04800e8>] (driver_probe_device+0x134/0x29c) [
>> 10.118957] [<c04800e8>] (driver_probe_device) from [<c04802d8>]
>> (__driver_attach+0x88/0xac) [ 10.118957] [<c04802d8>] (__driver_attach)
>> from [<c047e7b8>] (bus_for_each_dev+0x6c/0x90) [ 10.118957] [<c047e7b8>]
>> (bus_for_each_dev) from [<c047f798>] (bus_add_driver+0xcc/0x1e8) [
>> 10.118988] [<c047f798>] (bus_add_driver) from [<c0481228>]
>> (driver_register+0xac/0xf4) [ 10.118988] [<c0481228>] (driver_register)
>> from [<c010192c>] (do_one_initcall+0xac/0x154) [ 10.119018] [<c010192c>]
>> (do_one_initcall) from [<c02015bc>] (do_init_module+0x58/0x39c) [
>> 10.119049] [<c02015bc>] (do_init_module) from [<c01bd314>]
>> (load_module+0xe5c/0x1004) [ 10.119049] [<c01bd314>] (load_module) from
>> [<c01bd68c>] (SyS_finit_module+0x88/0x90) [ 10.119079] [<c01bd68c>]
>> (SyS_finit_module) from [<c0107040>] (ret_fast_syscall+0x0/0x1c)
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/media/platform/omap3isp/isp.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c index 5d54e2c..6e2624e 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -2114,7 +2114,6 @@ static int isp_of_parse_nodes(struct device *dev,
>>
>> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
>> if (!isd) {
>> - of_node_put(node);
>
> I don't think this one is correct. Looking at the context
>
> while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
> (node = of_graph_get_next_endpoint(dev->of_node, node))) {
> struct isp_async_subdev *isd;
>
> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> if (!isd) {
> of_node_put(node);
> return -ENOMEM;
> }
>
> notifier->subdevs[notifier->num_subdevs] = &isd->asd;
>
> if (isp_of_parse_node(dev, node, isd)) {
> of_node_put(node);
> return -EINVAL;
> }
>
> isd->asd.match.of.node =
> of_graph_get_remote_port_parent(node);
> of_node_put(node);
>
> if (!isd->asd.match.of.node) {
> dev_warn(dev, "bad remote port parent\n");
> return -EINVAL;
> }
>
> isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> notifier->num_subdevs++;
> }
>
> of_graph_get_next_endpoint() increases the reference count of the node it
> returns, which needs a corresponding of_node_put() in the error paths. It
> thus look like to me that the function isn't correct in that the
> devm_kzalloc() and !isd->asd.match.of.node error paths.
Ah ok, the of_node_put() is not always wrong.
>
>> return -ENOMEM;
>> }
>>
>> @@ -2126,7 +2125,7 @@ static int isp_of_parse_nodes(struct device *dev,
>> }
>>
>> isd->asd.match.of.node = of_graph_get_remote_port_parent(node);
>> - of_node_put(node);
>> +
>
> This change is correct not because of_graph_get_next_endpoint() has called
> of_node_put() but because it *will* call it on the next iteration.
>
> How about the following patch instead ?
>
> commit 4ed9893bf52c90181c8d5b1ae29a37556b89f1da
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date: Thu Sep 29 11:41:24 2016 +0300
>
> omap3isp: Fix OF node double put when parsing OF graph
>
> When parsing the graph the driver loops over all endpoints using
> of_graph_get_next_endpoint(). The function handles reference counting of
> the passed and returned nodes, so the returned node's reference count
> must not be decreased manually in the normal path.
>
> Move the offending of_node_put() call to the error path that requires
> manual reference count handling.
>
> Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 5e212668f726..f8b437cc8943 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2131,23 +2131,18 @@ static int isp_of_parse_nodes(struct device *dev,
> struct isp_async_subdev *isd;
>
> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> - if (!isd) {
> - of_node_put(node);
> - return -ENOMEM;
> - }
> + if (!isd)
> + goto error;
>
> notifier->subdevs[notifier->num_subdevs] = &isd->asd;
>
> - if (isp_of_parse_node(dev, node, isd)) {
> - of_node_put(node);
> - return -EINVAL;
> - }
> + if (isp_of_parse_node(dev, node, isd))
> + goto error;
>
> isd->asd.match.of.node = of_graph_get_remote_port_parent(node);
> - of_node_put(node);
> if (!isd->asd.match.of.node) {
> dev_warn(dev, "bad remote port parent\n");
> - return -EINVAL;
> + goto error;
> }
>
> isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> @@ -2155,6 +2150,10 @@ static int isp_of_parse_nodes(struct device *dev,
> }
>
> return notifier->num_subdevs;
> +
> +error:
> + of_node_put(node);
> + return -EINVAL;
> }
>
> static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
>
>> if (!isd->asd.match.of.node) {
>> dev_warn(dev, "bad remote port parent\n");
>> return -EINVAL;
Seems to fix the reported warning, but obviously I can't test the error paths.
So let's go this way (until someone shows a bug in the error path).
BR and thanks,
Nikolaus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] omap3isp: don't call of_node_put
2016-09-30 8:28 ` H. Nikolaus Schaller
@ 2016-09-30 8:52 ` Laurent Pinchart
2016-10-25 14:32 ` H. Nikolaus Schaller
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2016-09-30 8:52 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel,
Javier Martinez Canillas, arnd, hans.verkuil, tony, letux-kernel
Hello Nikolaus,
On Friday 30 Sep 2016 10:28:40 H. Nikolaus Schaller wrote:
> > Am 29.09.2016 um 10:54 schrieb Laurent Pinchart:
> > On Thursday 08 Sep 2016 17:48:33 H. Nikolaus Schaller wrote:
> >> of_node_put() has already been called inside
> >> of_graph_get_next_endpoint().
> >>
> >> Otherwise we may get warnings like
> >>
> >> [ 10.118286] omap3isp 480bc000.isp: parsing endpoint
> >> /ocp/isp@480bc000/ports/port@0/endpoint, interface 0 [ 10.118499]
> >> ERROR:
> >> Bad of_node_put() on /ocp/isp@480bc000/ports/port@0/endpoint [
> >> 10.118499]
> >> CPU: 0 PID: 968 Comm: udevd Not tainted 4.7.0-rc4-letux+ #376 [
> >> 10.118530] Hardware name: Generic OMAP36xx (Flattened Device Tree) [
> >> 10.118560] [<c010f0e0>] (unwind_backtrace) from [<c010b6d8>]
> >> (show_stack+0x10/0x14) [ 10.118591] [<c010b6d8>] (show_stack) from
> >> [<c03ecc50>] (dump_stack+0x98/0xd0) [ 10.118591] [<c03ecc50>]
> >> (dump_stack) from [<c03eecac>] (kobject_release+0x60/0x74) [ 10.118621]
> >> [<c03eecac>] (kobject_release) from [<c05ab128>]
> >> (__of_get_next_child+0x40/0x48) [ 10.118652] [<c05ab128>]
> >> (__of_get_next_child) from [<c05ab158>] (of_get_next_child+0x28/0x44) [
> >> 10.118652] [<c05ab158>] (of_get_next_child) from [<c05ab350>]
> >> (of_graph_get_next_endpoint+0xe4/0x124) [ 10.118804] [<c05ab350>]
> >> (of_graph_get_next_endpoint) from [<bf1c88a4>] (isp_probe+0xdc/0xd80
> >> [omap3_isp]) [ 10.118896] [<bf1c88a4>] (isp_probe [omap3_isp]) from
> >> [<c0482008>] (platform_drv_probe+0x50/0xa0) [ 10.118927] [<c0482008>]
> >> (platform_drv_probe) from [<c04800e8>] (driver_probe_device+0x134/0x29c)
> >> [
> >> 10.118957] [<c04800e8>] (driver_probe_device) from [<c04802d8>]
> >> (__driver_attach+0x88/0xac) [ 10.118957] [<c04802d8>] (__driver_attach)
> >> from [<c047e7b8>] (bus_for_each_dev+0x6c/0x90) [ 10.118957]
> >> [<c047e7b8>]
> >> (bus_for_each_dev) from [<c047f798>] (bus_add_driver+0xcc/0x1e8) [
> >> 10.118988] [<c047f798>] (bus_add_driver) from [<c0481228>]
> >> (driver_register+0xac/0xf4) [ 10.118988] [<c0481228>] (driver_register)
> >> from [<c010192c>] (do_one_initcall+0xac/0x154) [ 10.119018]
> >> [<c010192c>]
> >> (do_one_initcall) from [<c02015bc>] (do_init_module+0x58/0x39c) [
> >> 10.119049] [<c02015bc>] (do_init_module) from [<c01bd314>]
> >> (load_module+0xe5c/0x1004) [ 10.119049] [<c01bd314>] (load_module) from
> >> [<c01bd68c>] (SyS_finit_module+0x88/0x90) [ 10.119079] [<c01bd68c>]
> >> (SyS_finit_module) from [<c0107040>] (ret_fast_syscall+0x0/0x1c)
> >>
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >> drivers/media/platform/omap3isp/isp.c | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index 5d54e2c..6e2624e 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -2114,7 +2114,6 @@ static int isp_of_parse_nodes(struct device *dev,
> >>
> >> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> >> if (!isd) {
> >> - of_node_put(node);
> >
> > I don't think this one is correct. Looking at the context
> >
> > while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
> > (node = of_graph_get_next_endpoint(dev->of_node, node))) {
> > struct isp_async_subdev *isd;
> >
> > isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> > if (!isd) {
> > of_node_put(node);
> > return -ENOMEM;
> > }
> >
> > notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> >
> > if (isp_of_parse_node(dev, node, isd)) {
> > of_node_put(node);
> > return -EINVAL;
> > }
> >
> > isd->asd.match.of.node =
> > of_graph_get_remote_port_parent(node);
> > of_node_put(node);
> >
> > if (!isd->asd.match.of.node) {
> > dev_warn(dev, "bad remote port parent\n");
> > return -EINVAL;
> > }
> >
> > isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> > notifier->num_subdevs++;
> > }
> >
> > of_graph_get_next_endpoint() increases the reference count of the node it
> > returns, which needs a corresponding of_node_put() in the error paths. It
> > thus look like to me that the function isn't correct in that the
> > devm_kzalloc() and !isd->asd.match.of.node error paths.
>
> Ah ok, the of_node_put() is not always wrong.
>
> >> return -ENOMEM;
> >> }
> >>
> >> @@ -2126,7 +2125,7 @@ static int isp_of_parse_nodes(struct device *dev,
> >> }
> >>
> >> isd->asd.match.of.node =
> >> of_graph_get_remote_port_parent(node);
> >> - of_node_put(node);
> >> +
> >
> > This change is correct not because of_graph_get_next_endpoint() has called
> > of_node_put() but because it *will* call it on the next iteration.
> >
> > How about the following patch instead ?
> >
> > commit 4ed9893bf52c90181c8d5b1ae29a37556b89f1da
> > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Date: Thu Sep 29 11:41:24 2016 +0300
> >
> > omap3isp: Fix OF node double put when parsing OF graph
> >
> > When parsing the graph the driver loops over all endpoints using
> > of_graph_get_next_endpoint(). The function handles reference counting
> > of the passed and returned nodes, so the returned node's reference
> > count must not be decreased manually in the normal path.
> >
> > Move the offending of_node_put() call to the error path that requires
> > manual reference count handling.
> >
> > Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 5e212668f726..f8b437cc8943
> > 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2131,23 +2131,18 @@ static int isp_of_parse_nodes(struct device *dev,
> > struct isp_async_subdev *isd;
> >
> > isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> > - if (!isd) {
> > - of_node_put(node);
> > - return -ENOMEM;
> > - }
> > + if (!isd)
> > + goto error;
> >
> > notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> >
> > - if (isp_of_parse_node(dev, node, isd)) {
> > - of_node_put(node);
> > - return -EINVAL;
> > - }
> > + if (isp_of_parse_node(dev, node, isd))
> > + goto error;
> >
> > isd->asd.match.of.node =
> > of_graph_get_remote_port_parent(node);
> > - of_node_put(node);
> > if (!isd->asd.match.of.node) {
> > dev_warn(dev, "bad remote port parent\n");
> > - return -EINVAL;
> > + goto error;
> > }
> >
> > isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> > @@ -2155,6 +2150,10 @@ static int isp_of_parse_nodes(struct device *dev,
> > }
> >
> > return notifier->num_subdevs;
> > +
> > +error:
> > + of_node_put(node);
> > + return -EINVAL;
> > }
> >
> > static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
>
> Seems to fix the reported warning, but obviously I can't test the error
> paths.
Can I add
Tested-by: H. Nikolaus Schaller <hns@goldelico.com>
to the patch ?
> So let's go this way (until someone shows a bug in the error path).
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] omap3isp: don't call of_node_put
2016-09-30 8:52 ` Laurent Pinchart
@ 2016-10-25 14:32 ` H. Nikolaus Schaller
0 siblings, 0 replies; 6+ messages in thread
From: H. Nikolaus Schaller @ 2016-10-25 14:32 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel,
Javier Martinez Canillas, arnd, hans.verkuil, tony, letux-kernel
Hi,
has it been merged somewhere? I can't find it in linux-next yet.
BR and thanks,
Nikolaus
> Am 30.09.2016 um 10:52 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>
> Hello Nikolaus,
>
> On Friday 30 Sep 2016 10:28:40 H. Nikolaus Schaller wrote:
>>> Am 29.09.2016 um 10:54 schrieb Laurent Pinchart:
>>> On Thursday 08 Sep 2016 17:48:33 H. Nikolaus Schaller wrote:
>>>> of_node_put() has already been called inside
>>>> of_graph_get_next_endpoint().
>>>>
>>>> Otherwise we may get warnings like
>>>>
>>>> [ 10.118286] omap3isp 480bc000.isp: parsing endpoint
>>>> /ocp/isp@480bc000/ports/port@0/endpoint, interface 0 [ 10.118499]
>>>> ERROR:
>>>> Bad of_node_put() on /ocp/isp@480bc000/ports/port@0/endpoint [
>>>> 10.118499]
>>>> CPU: 0 PID: 968 Comm: udevd Not tainted 4.7.0-rc4-letux+ #376 [
>>>> 10.118530] Hardware name: Generic OMAP36xx (Flattened Device Tree) [
>>>> 10.118560] [<c010f0e0>] (unwind_backtrace) from [<c010b6d8>]
>>>> (show_stack+0x10/0x14) [ 10.118591] [<c010b6d8>] (show_stack) from
>>>> [<c03ecc50>] (dump_stack+0x98/0xd0) [ 10.118591] [<c03ecc50>]
>>>> (dump_stack) from [<c03eecac>] (kobject_release+0x60/0x74) [ 10.118621]
>>>> [<c03eecac>] (kobject_release) from [<c05ab128>]
>>>> (__of_get_next_child+0x40/0x48) [ 10.118652] [<c05ab128>]
>>>> (__of_get_next_child) from [<c05ab158>] (of_get_next_child+0x28/0x44) [
>>>> 10.118652] [<c05ab158>] (of_get_next_child) from [<c05ab350>]
>>>> (of_graph_get_next_endpoint+0xe4/0x124) [ 10.118804] [<c05ab350>]
>>>> (of_graph_get_next_endpoint) from [<bf1c88a4>] (isp_probe+0xdc/0xd80
>>>> [omap3_isp]) [ 10.118896] [<bf1c88a4>] (isp_probe [omap3_isp]) from
>>>> [<c0482008>] (platform_drv_probe+0x50/0xa0) [ 10.118927] [<c0482008>]
>>>> (platform_drv_probe) from [<c04800e8>] (driver_probe_device+0x134/0x29c)
>>>> [
>>>> 10.118957] [<c04800e8>] (driver_probe_device) from [<c04802d8>]
>>>> (__driver_attach+0x88/0xac) [ 10.118957] [<c04802d8>] (__driver_attach)
>>>> from [<c047e7b8>] (bus_for_each_dev+0x6c/0x90) [ 10.118957]
>>>> [<c047e7b8>]
>>>> (bus_for_each_dev) from [<c047f798>] (bus_add_driver+0xcc/0x1e8) [
>>>> 10.118988] [<c047f798>] (bus_add_driver) from [<c0481228>]
>>>> (driver_register+0xac/0xf4) [ 10.118988] [<c0481228>] (driver_register)
>>>> from [<c010192c>] (do_one_initcall+0xac/0x154) [ 10.119018]
>>>> [<c010192c>]
>>>> (do_one_initcall) from [<c02015bc>] (do_init_module+0x58/0x39c) [
>>>> 10.119049] [<c02015bc>] (do_init_module) from [<c01bd314>]
>>>> (load_module+0xe5c/0x1004) [ 10.119049] [<c01bd314>] (load_module) from
>>>> [<c01bd68c>] (SyS_finit_module+0x88/0x90) [ 10.119079] [<c01bd68c>]
>>>> (SyS_finit_module) from [<c0107040>] (ret_fast_syscall+0x0/0x1c)
>>>>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/media/platform/omap3isp/isp.c | 3 +--
>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/omap3isp/isp.c
>>>> b/drivers/media/platform/omap3isp/isp.c index 5d54e2c..6e2624e 100644
>>>> --- a/drivers/media/platform/omap3isp/isp.c
>>>> +++ b/drivers/media/platform/omap3isp/isp.c
>>>> @@ -2114,7 +2114,6 @@ static int isp_of_parse_nodes(struct device *dev,
>>>>
>>>> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
>>>> if (!isd) {
>>>> - of_node_put(node);
>>>
>>> I don't think this one is correct. Looking at the context
>>>
>>> while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
>>> (node = of_graph_get_next_endpoint(dev->of_node, node))) {
>>> struct isp_async_subdev *isd;
>>>
>>> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
>>> if (!isd) {
>>> of_node_put(node);
>>> return -ENOMEM;
>>> }
>>>
>>> notifier->subdevs[notifier->num_subdevs] = &isd->asd;
>>>
>>> if (isp_of_parse_node(dev, node, isd)) {
>>> of_node_put(node);
>>> return -EINVAL;
>>> }
>>>
>>> isd->asd.match.of.node =
>>> of_graph_get_remote_port_parent(node);
>>> of_node_put(node);
>>>
>>> if (!isd->asd.match.of.node) {
>>> dev_warn(dev, "bad remote port parent\n");
>>> return -EINVAL;
>>> }
>>>
>>> isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
>>> notifier->num_subdevs++;
>>> }
>>>
>>> of_graph_get_next_endpoint() increases the reference count of the node it
>>> returns, which needs a corresponding of_node_put() in the error paths. It
>>> thus look like to me that the function isn't correct in that the
>>> devm_kzalloc() and !isd->asd.match.of.node error paths.
>>
>> Ah ok, the of_node_put() is not always wrong.
>>
>>>> return -ENOMEM;
>>>> }
>>>>
>>>> @@ -2126,7 +2125,7 @@ static int isp_of_parse_nodes(struct device *dev,
>>>> }
>>>>
>>>> isd->asd.match.of.node =
>>>> of_graph_get_remote_port_parent(node);
>>>> - of_node_put(node);
>>>> +
>>>
>>> This change is correct not because of_graph_get_next_endpoint() has called
>>> of_node_put() but because it *will* call it on the next iteration.
>>>
>>> How about the following patch instead ?
>>>
>>> commit 4ed9893bf52c90181c8d5b1ae29a37556b89f1da
>>> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Date: Thu Sep 29 11:41:24 2016 +0300
>>>
>>> omap3isp: Fix OF node double put when parsing OF graph
>>>
>>> When parsing the graph the driver loops over all endpoints using
>>> of_graph_get_next_endpoint(). The function handles reference counting
>>> of the passed and returned nodes, so the returned node's reference
>>> count must not be decreased manually in the normal path.
>>>
>>> Move the offending of_node_put() call to the error path that requires
>>> manual reference count handling.
>>>
>>> Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>> diff --git a/drivers/media/platform/omap3isp/isp.c
>>> b/drivers/media/platform/omap3isp/isp.c index 5e212668f726..f8b437cc8943
>>> 100644
>>> --- a/drivers/media/platform/omap3isp/isp.c
>>> +++ b/drivers/media/platform/omap3isp/isp.c
>>> @@ -2131,23 +2131,18 @@ static int isp_of_parse_nodes(struct device *dev,
>>> struct isp_async_subdev *isd;
>>>
>>> isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
>>> - if (!isd) {
>>> - of_node_put(node);
>>> - return -ENOMEM;
>>> - }
>>> + if (!isd)
>>> + goto error;
>>>
>>> notifier->subdevs[notifier->num_subdevs] = &isd->asd;
>>>
>>> - if (isp_of_parse_node(dev, node, isd)) {
>>> - of_node_put(node);
>>> - return -EINVAL;
>>> - }
>>> + if (isp_of_parse_node(dev, node, isd))
>>> + goto error;
>>>
>>> isd->asd.match.of.node =
>>> of_graph_get_remote_port_parent(node);
>>> - of_node_put(node);
>>> if (!isd->asd.match.of.node) {
>>> dev_warn(dev, "bad remote port parent\n");
>>> - return -EINVAL;
>>> + goto error;
>>> }
>>>
>>> isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
>>> @@ -2155,6 +2150,10 @@ static int isp_of_parse_nodes(struct device *dev,
>>> }
>>>
>>> return notifier->num_subdevs;
>>> +
>>> +error:
>>> + of_node_put(node);
>>> + return -EINVAL;
>>> }
>>>
>>> static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
>>
>> Seems to fix the reported warning, but obviously I can't test the error
>> paths.
>
> Can I add
>
> Tested-by: H. Nikolaus Schaller <hns@goldelico.com>
>
> to the patch ?
>
>> So let's go this way (until someone shows a bug in the error path).
>
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-25 14:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-08 15:48 [PATCH] [media] omap3isp: don't call of_node_put H. Nikolaus Schaller
[not found] ` <D55EF0EA-F7B8-4DA7-8F2F-BCC6650D194F@goldelico.com>
2016-09-29 6:59 ` H. Nikolaus Schaller
2016-09-29 8:54 ` Laurent Pinchart
2016-09-30 8:28 ` H. Nikolaus Schaller
2016-09-30 8:52 ` Laurent Pinchart
2016-10-25 14:32 ` H. Nikolaus Schaller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).