[PATCH] i.MX6: WEIM: put node reference on early loop exit

[PATCH] i.MX6: WEIM: put node reference on early loop exit

[alison at she-devel.com]

From: Alison Chaiken <alison_chaiken@mentor.com>

Signed-off-by: Alison Chaiken <alison_chaiken@mentor.com>
---
 drivers/bus/imx-weim.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 0958b69..11f2c8b 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -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;
 }
-- 
2.1.4

[PATCH] i.MX6: WEIM: put node reference on early loop exit

[Sascha Hauer]

On Fri, Feb 13, 2015 at 09:33:04AM -0800, alison at she-devel.com wrote:
> From: Alison Chaiken <alison_chaiken@mentor.com>
> 
> Signed-off-by: Alison Chaiken <alison_chaiken@mentor.com>

Acked-by: Sascha Hauer <s.hauer@pengutronix.de>

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

-- 
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] i.MX6: WEIM: put node reference on early loop exit

[Chaiken, Alison]

Sascha Hauer <s.hauer@pengutronix.de>
>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] i.MX6: WEIM: put node reference on early loop exit

[Joshua Clayton]

On Tuesday, February 17, 2015 04:28:28 PM Chaiken, Alison wrote:
> Sascha Hauer <s.hauer@pengutronix.de>
> 
> >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.

> 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? 

> 
> -- Alison
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Joshua Clayton
Software Engineer
UniWest
122 S. 4th Avenue
Pasco, WA 99301
Ph: (509) 544-0720
Fx: (509) 544-0868 

[PATCH] i.MX6: WEIM: improve error handling upon child probe-failure

[alison at she-devel.com]

From: Alison Chaiken <alison_chaiken@mentor.com>

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 <alison_chaiken@mentor.com>
---
 drivers/bus/imx-weim.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 0958b69..98e04bf 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -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)
-- 
2.1.4

[PATCH] i.MX6: WEIM: improve error handling upon child probe-failure

[Joshua Clayton]

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.

On Wednesday, February 18, 2015 12:37:21 AM alison at she-devel.com wrote:
> From: Alison Chaiken <alison_chaiken@mentor.com>
> 
> 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 <alison_chaiken@mentor.com>
> ---
>  drivers/bus/imx-weim.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> index 0958b69..98e04bf 100644
> --- a/drivers/bus/imx-weim.c
> +++ b/drivers/bus/imx-weim.c
> @@ -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.

> 
>  	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);
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?

> -	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.
> 
>  static 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

-- 
Joshua Clayton
Software Engineer
UniWest
122 S. 4th Avenue
Pasco, WA 99301
Ph: (509) 544-0720
Fx: (509) 544-0868 

[PATCH v3 0/1] i.MX6: WEIM: improve error handling upon child probe-failure

[alison at she-devel.com]

From: Alison Chaiken <alison_chaiken@mentor.com>

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:
>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(-)

-- 
2.1.4

[PATCH v3 1/1] i.MX6: WEIM: improve error handling upon child probe-failure

[alison at she-devel.com]

From: Alison Chaiken <alison_chaiken@mentor.com>

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 <alison_chaiken@mentor.com>
---
 drivers/bus/imx-weim.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 0958b69..e98d15e 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -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)
-- 
2.1.4

[PATCH v3 1/1] i.MX6: WEIM: improve error handling upon child probe-failure

[Sascha Hauer]

On Wed, Feb 18, 2015 at 11:24:10PM -0800, alison at she-devel.com wrote:
> From: Alison Chaiken <alison_chaiken@mentor.com>
> 
> 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 <alison_chaiken@mentor.com>

Acked-by: Sascha Hauer <s.hauer@pengutronix.de>

-- 
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

[Shawn Guo]

On Wed, Feb 18, 2015 at 11:24:10PM -0800, alison at she-devel.com wrote:
> From: Alison Chaiken <alison_chaiken@mentor.com>
> 
> 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 <alison_chaiken@mentor.com>

Applied after fixing the patch prefix to be 'bus: imx-weim: '.

Shawn

> ---
>  drivers/bus/imx-weim.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> index 0958b69..e98d15e 100644
> --- a/drivers/bus/imx-weim.c
> +++ b/drivers/bus/imx-weim.c
> @@ -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)
> -- 
> 2.1.4
>