i.MX6: WEIM: put node reference on early loop exit
PATCH 1/1 i.MX6: WEIM: put node reference on early loop exit
From: Alison Chaiken <[email protected]>
--- drivers/bus/imx-weim.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/bus/imx-weim.c +9 -2
@@ -158,7 +158,7 @@ static int __init weim_parse_dt(struct platform_device *pdev,if (ret) {dev_err (&pdev->dev,"%s set timing failed.\n" , child->full_name); -return ret; + goto put_node; } }
@@ -166,7 +166,14 @@ static int __init weim_parse_dt(struct platform_device *pdev, of_default_bus_match_table, NULL, &pdev->dev);if (ret) -dev_err (&pdev->dev,"%s fail to create devices.\n" , + goto out; + +return 0 ; + +put_node: +of_node_put (child); +out: +dev_err (&pdev->dev,"%s fail to create devices.\n" , pdev->dev.of_node->full_name);return ret; }
Signature · 1 line
2.1.4
Sascha Hauer
maintainerPATCH 1/1 i.MX6: WEIM: put node reference on early loop exit
Quoted 3 lines · alison at she-devel.com
From: Alison Chaiken <[email protected]>
Signed-off-by Alison Chaiken <[email protected]>
Looking at the code I get the question why the current code bails out on error. Wouldn't it be better to just skip erroneous child devices and issue a warning?
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH 1/1 i.MX6: WEIM: put node reference on early loop exit
Sascha Hauer <[email protected]>
Quoted 3 lines
Looking at the code I get the question why the current code bails out on error. Wouldn't it be better to just skip erroneous child devices and issue a warning?
The WEIM connects only one child at any given time. Since it is a platform device, any child is cold-plugged. If the probe of the child that is marked 'status = "okay"' in the device-tree fails, then the WEIM itself has no function, which sounds more like an error than a warning state.
-- Alison
PATCH 1/1 i.MX6: WEIM: put node reference on early loop exit
Quoted 7 lines · Alison
Sascha Hauer <[email protected]>
Quoted 3 lines
Looking at the code I get the question why the current code bails out on error. Wouldn't it be better to just skip erroneous child devices and issue a warning?
The WEIM connects only one child at any given time. Since it is a platform
Not true. WEIM is designed to connect to up to 4 children each with its own chipselect and io memory area.
Quoted 3 lines
device, any child is cold-plugged. If the probe of the child that is
marked 'status = "okay"' in the device-tree fails, then the WEIM itself has no function, which sounds more like an error than a warning state.
Is it possible to set 'status = disabled' per child if the call to weim_timing_setup() fails, or something like that, so that of_platform_populate() will skip it?
Quoted 7 lines
-- Alison
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 7 lines
Joshua Clayton Software Engineer UniWest 122 S. 4th Avenue Pasco, WA 99301 Ph: (509) 544-0720 Fx: (509) 544-0868
PATCH 1/1 i.MX6: WEIM: improve error handling upon child probe-failure
From: Alison Chaiken <[email protected]>
Probe all children of the WEIM node, reporting any failures. Report failure from parsing of WEIM node itself if probes of all children fail.
--- drivers/bus/imx-weim.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/bus/imx-weim.c +12 -8
@@ -142,7 +142,7 @@ static int __init weim_parse_dt(struct platform_device *pdev, &pdev->dev);const struct imx_weim_devtype *devtype = of_id->data;struct device_node *child; -int ret; +int ret, have_child;if (devtype == &imx50_weim_devtype) { ret =imx_weim_gpr_setup (pdev);
@@ -155,20 +155,24 @@ static int __init weim_parse_dt(struct platform_device *pdev,continue ; ret =weim_timing_setup (child, base, devtype); -if (ret) { -dev_err (&pdev->dev,"%s set timing failed.\n" , +if (ret) +dev_warn (&pdev->dev,"%s set timing failed.\n" , child->full_name); -return ret; - } +else + have_child =1 ; } - ret =of_platform_populate (pdev->dev.of_node, +if (have_child) + ret =of_platform_populate (pdev->dev.of_node, of_default_bus_match_table,NULL , &pdev->dev); -if (ret) +if (ret) {dev_err (&pdev->dev,"%s fail to create devices.\n" , pdev->dev.of_node->full_name); -return ret; +return ret; + } + +return 0 ; }static int __init weim_probe (struct platform_device *pdev)
Signature · 1 line
2.1.4
PATCH 1/1 i.MX6: WEIM: improve error handling upon child probe-failure
Hi Alison.
I'm assuming this is a v2 of your previously posted patch. Bear in mind I am not super familiar with refcount issues in devicetree. There is no explicit of_node_get(), at least at this level, so the need for the original patch is not entirely clear to me.
Quoted 15 lines · alison at she-devel.com
From: Alison Chaiken <[email protected]>
Probe all children of the WEIM node, reporting any failures. Report failure from parsing of WEIM node itself if probes of all children fail.
Signed-off-by Alison Chaiken <[email protected]>--- drivers/bus/imx-weim.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)drivers/bus/imx-weim.c +0
-0
drivers/bus/imx-weim.c static int __init weim_parse_dt(struct platform_device · line 145
Quoted 5 lines
drivers/bus/imx-weim.c +1
-1@@ -142,7 +142,7 @@ static int __init weim_parse_dt(struct platform_device *pdev, &pdev->dev);const struct imx_weim_devtype *devtype = of_id->data;struct device_node *child; -int ret; +int ret, have_child;
You need to initialize have_child to 0.
Quoted 13 lines
drivers/bus/imx-weim.c +2
-3if (devtype == &imx50_weim_devtype) { ret =imx_weim_gpr_setup (pdev);@@ -155,20 +155,24 @@ static int __init weim_parse_dt(struct platform_device *pdev, continue; ret =weim_timing_setup (child, base, devtype); -if (ret) { -dev_err (&pdev->dev,"%s set timing failed.\n" , +if (ret) +dev_warn (&pdev->dev,"%s set timing failed.\n" , child->full_name); -return ret;
drivers/bus/imx-weim.c static int __init weim_parse_dt(struct platform_device · line 168
Quoted 10 lines
drivers/bus/imx-weim.c +4
-2- } +else + have_child =1 ; } - ret =of_platform_populate (pdev->dev.of_node, +if (have_child) + ret =of_platform_populate (pdev->dev.of_node, of_default_bus_match_table,NULL , &pdev->dev);
Where did the need for of_node_put() in the original patch come from? Is it absolved by calling platform_populate? If so, should of_node_put() be called for each child in the !have_child case?
drivers/bus/imx-weim.c static int __init weim_parse_dt(struct platform_device *pdev, · line 176
Quoted 10 lines
drivers/bus/imx-weim.c +5
-2-if (ret) +if (ret) {dev_err (&pdev->dev,"%s fail to create devices.\n" , pdev->dev.of_node->full_name); -return ret; +return ret; + } + +return 0 ; }
This last change doesn't... err ... change. drop it.
Quoted 10 lines
drivers/bus/imx-weim.c +0
-1static int __init weim_probe (struct platform_device *pdev) --2.1.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 7 lines
Joshua Clayton Software Engineer UniWest 122 S. 4th Avenue Pasco, WA 99301 Ph: (509) 544-0720 Fx: (509) 544-0868
v3 cover i.MX6: WEIM: improve error handling upon child probe-failure
In reply to Joshua ClaytonFrom: Alison Chaiken <[email protected]>
Here is a third version which incorporates Joshua Clayton's suggestions. The boolean value have_child is initialized to zero and the redundant return statement is removed.
Joshua inquires:
Quoted 2 lines
There is no explicit
of_node_get(), at least at this level, so the need for the original patch is not entirely clear to me.
for_each_child_of_node() calls of_get_next_child() which in turn calls of_node_get(). If the for_each_child_of_node() iterator runs to the end of the list, the last call of of_get_next_child() puts the final open reference. If the iterator loop is exited early due to an error,the final reference needs to be cleaned up manually. Since, in the last two versions, failure of a child probe results in a warning rather than a loop exit, there is no longer a dangling reference.
Alison Chaiken (1):
i.MX6: WEIM: improve error handling upon child probe-failure
drivers/bus/imx-weim.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
Signature · 1 line
2.1.4
PATCH v3 1/1 i.MX6: WEIM: improve error handling upon child probe-failure
In reply to Joshua ClaytonFrom: Alison Chaiken <[email protected]>
Probe all children of the WEIM node, reporting any failures. Report failure from parsing of WEIM node itself if probes of all children fail.
--- drivers/bus/imx-weim.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/bus/imx-weim.c +7 -6
@@ -142,7 +142,7 @@ static int __init weim_parse_dt(struct platform_device *pdev, &pdev->dev);const struct imx_weim_devtype *devtype = of_id->data;struct device_node *child; -int ret; +int ret, have_child =0 ;if (devtype == &imx50_weim_devtype) { ret =imx_weim_gpr_setup (pdev);
@@ -155,14 +155,15 @@ static int __init weim_parse_dt(struct platform_device *pdev,continue ; ret =weim_timing_setup (child, base, devtype); -if (ret) { -dev_err (&pdev->dev,"%s set timing failed.\n" , +if (ret) +dev_warn (&pdev->dev,"%s set timing failed.\n" , child->full_name); -return ret; - } +else + have_child =1 ; } - ret =of_platform_populate (pdev->dev.of_node, +if (have_child) + ret =of_platform_populate (pdev->dev.of_node, of_default_bus_match_table,NULL , &pdev->dev);if (ret)
Signature · 1 line
2.1.4
Sascha Hauer
maintainerPATCH v3 1/1 i.MX6: WEIM: improve error handling upon child probe-failure
Quoted 6 lines · alison at she-devel.com
From: Alison Chaiken <[email protected]>
Probe all children of the WEIM node, reporting any failures. Report failure from parsing of WEIM node itself if probes of all children fail.
Signed-off-by Alison Chaiken <[email protected]>
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH v3 1/1 i.MX6: WEIM: improve error handling upon child probe-failure
In reply to alison at she-devel.comQuoted 6 lines · alison at she-devel.com
From: Alison Chaiken <[email protected]>
Probe all children of the WEIM node, reporting any failures. Report failure from parsing of WEIM node itself if probes of all children fail.
Signed-off-by Alison Chaiken <[email protected]>
Applied after fixing the patch prefix to be 'bus: imx-weim: '.
Shawn
Quoted 41 lines
--- drivers/bus/imx-weim.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)drivers/bus/imx-weim.c +7
-6@@ -142,7 +142,7 @@ static int __init weim_parse_dt(struct platform_device *pdev, &pdev->dev);const struct imx_weim_devtype *devtype = of_id->data;struct device_node *child; -int ret; +int ret, have_child =0 ;if (devtype == &imx50_weim_devtype) { ret =imx_weim_gpr_setup (pdev);@@ -155,14 +155,15 @@ static int __init weim_parse_dt(struct platform_device *pdev,continue ; ret =weim_timing_setup (child, base, devtype); -if (ret) { -dev_err (&pdev->dev,"%s set timing failed.\n" , +if (ret) +dev_warn (&pdev->dev,"%s set timing failed.\n" , child->full_name); -return ret; - } +else + have_child =1 ; } - ret =of_platform_populate (pdev->dev.of_node, +if (have_child) + ret =of_platform_populate (pdev->dev.of_node, of_default_bus_match_table,NULL , &pdev->dev);if (ret)Signature · 1 line
2.1.4