[PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Johan Hovold]

Fix child-node lookup during probe, which ended up searching the whole
device tree depth-first starting at the parent rather than just matching
on its children.

To make things worse, the parent mfd node was also prematurely freed.

Note that the nodes returned from the two calls to of_parse_phandle()
are also leaking, but fixing that is a bit more involved as pointers to
node fields are being stored for later use.

Fixes: 59eb2b5e57ea ("drivers/video/backlight/as3711_bl.c: add OF support")
Cc: stable <stable@vger.kernel.org>     # 3.10
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/video/backlight/as3711_bl.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
index 734a9158946b..21ce56053c88 100644
--- a/drivers/video/backlight/as3711_bl.c
+++ b/drivers/video/backlight/as3711_bl.c
@@ -262,15 +262,16 @@ static int as3711_bl_register(struct platform_device *pdev,
 static int as3711_backlight_parse_dt(struct device *dev)
 {
 	struct as3711_bl_pdata *pdata = dev_get_platdata(dev);
-	struct device_node *bl =
-		of_find_node_by_name(dev->parent->of_node, "backlight"), *fb;
+	struct device_node *bl, *fb;
 	int ret;
 
+	bl = of_get_child_by_name(dev->parent->of_node, "backlight");
 	if (!bl) {
 		dev_dbg(dev, "backlight node not found\n");
 		return -ENODEV;
 	}
 
+	/* FIXME: need to drop reference to returned node */
 	fb = of_parse_phandle(bl, "su1-dev", 0);
 	if (fb) {
 		pdata->su1_fb = fb->full_name;
@@ -279,9 +280,10 @@ static int as3711_backlight_parse_dt(struct device *dev)
 		if (pdata->su1_max_uA <= 0)
 			ret = -EINVAL;
 		if (ret < 0)
-			return ret;
+			goto err_put_bl;
 	}
 
+	/* FIXME: need to drop reference to returned node */
 	fb = of_parse_phandle(bl, "su2-dev", 0);
 	if (fb) {
 		int count = 0;
@@ -292,7 +294,7 @@ static int as3711_backlight_parse_dt(struct device *dev)
 		if (pdata->su2_max_uA <= 0)
 			ret = -EINVAL;
 		if (ret < 0)
-			return ret;
+			goto err_put_bl;
 
 		if (of_find_property(bl, "su2-feedback-voltage", NULL)) {
 			pdata->su2_feedback = AS3711_SU2_VOLTAGE;
@@ -314,8 +316,10 @@ static int as3711_backlight_parse_dt(struct device *dev)
 			pdata->su2_feedback = AS3711_SU2_CURR_AUTO;
 			count++;
 		}
-		if (count != 1)
-			return -EINVAL;
+		if (count != 1) {
+			ret = -EINVAL;
+			goto err_put_bl;
+		}
 
 		count = 0;
 		if (of_find_property(bl, "su2-fbprot-lx-sd4", NULL)) {
@@ -334,8 +338,10 @@ static int as3711_backlight_parse_dt(struct device *dev)
 			pdata->su2_fbprot = AS3711_SU2_GPIO4;
 			count++;
 		}
-		if (count != 1)
-			return -EINVAL;
+		if (count != 1) {
+			ret = -EINVAL;
+			goto err_put_bl;
+		}
 
 		count = 0;
 		if (of_find_property(bl, "su2-auto-curr1", NULL)) {
@@ -355,11 +361,20 @@ static int as3711_backlight_parse_dt(struct device *dev)
 		 * At least one su2-auto-curr* must be specified iff
 		 * AS3711_SU2_CURR_AUTO is used
 		 */
-		if (!count ^ (pdata->su2_feedback != AS3711_SU2_CURR_AUTO))
-			return -EINVAL;
+		if (!count ^ (pdata->su2_feedback != AS3711_SU2_CURR_AUTO)) {
+			ret = -EINVAL;
+			goto err_put_bl;
+		}
 	}
 
+	of_node_put(bl);
+
 	return 0;
+
+err_put_bl:
+	of_node_put(bl);
+
+	return ret;
 }
 
 static int as3711_backlight_probe(struct platform_device *pdev)
-- 
2.15.0

[PATCH 2/3] backlight: max8925_bl: fix device-tree node lookup

[Johan Hovold]

Fix child-node lookup during probe, which ended up searching the whole
device tree depth-first starting at the parent rather than just matching
on its children.

To make things worse, the parent mfd node was also prematurely freed,
while the child backlight node was leaked.

Fixes: 47ec340cb8e2 ("mfd: max8925: Support dt for backlight")
Cc: stable <stable@vger.kernel.org>     # 3.9
Cc: Qing Xu <qingx@marvell.com>
Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/video/backlight/max8925_bl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/backlight/max8925_bl.c b/drivers/video/backlight/max8925_bl.c
index 7b738d60ecc2..f3aa6088f1d9 100644
--- a/drivers/video/backlight/max8925_bl.c
+++ b/drivers/video/backlight/max8925_bl.c
@@ -116,7 +116,7 @@ static void max8925_backlight_dt_init(struct platform_device *pdev)
 	if (!pdata)
 		return;
 
-	np = of_find_node_by_name(nproot, "backlight");
+	np = of_get_child_by_name(nproot, "backlight");
 	if (!np) {
 		dev_err(&pdev->dev, "failed to find backlight node\n");
 		return;
@@ -125,6 +125,8 @@ static void max8925_backlight_dt_init(struct platform_device *pdev)
 	if (!of_property_read_u32(np, "maxim,max8925-dual-string", &val))
 		pdata->dual_string = val;
 
+	of_node_put(np);
+
 	pdev->dev.platform_data = pdata;
 }
 
-- 
2.15.0

[PATCH 3/3] backlight: tps65217_bl: fix device-tree node lookup

[Johan Hovold]

Fix child-node lookup during probe, which ended up searching the whole
device tree depth-first starting at the parent rather than just matching
on its children.

This would only cause trouble if the child node is missing while there
is an unrelated node named "backlight" elsewhere in the tree.

Fixes: eebfdc17cc6c ("backlight: Add TPS65217 WLED driver")
Cc: stable <stable@vger.kernel.org>     # 3.7
Cc: Matthias Kaehlcke <matthias@kaehlcke.net>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/video/backlight/tps65217_bl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/tps65217_bl.c b/drivers/video/backlight/tps65217_bl.c
index 380917c86276..762e3feed097 100644
--- a/drivers/video/backlight/tps65217_bl.c
+++ b/drivers/video/backlight/tps65217_bl.c
@@ -184,11 +184,11 @@ static struct tps65217_bl_pdata *
 tps65217_bl_parse_dt(struct platform_device *pdev)
 {
 	struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
-	struct device_node *node = of_node_get(tps->dev->of_node);
+	struct device_node *node;
 	struct tps65217_bl_pdata *pdata, *err;
 	u32 val;
 
-	node = of_find_node_by_name(node, "backlight");
+	node = of_get_child_by_name(tps->dev->of_node, "backlight");
 	if (!node)
 		return ERR_PTR(-ENODEV);
 
-- 
2.15.0

Re: [PATCH 3/3] backlight: tps65217_bl: fix device-tree node lookup

[Daniel Thompson]

On 13/11/17 10:20, Johan Hovold wrote:
> Fix child-node lookup during probe, which ended up searching the whole
> device tree depth-first starting at the parent rather than just matching
> on its children.
> 
> This would only cause trouble if the child node is missing while there
> is an unrelated node named "backlight" elsewhere in the tree.
> 
> Fixes: eebfdc17cc6c ("backlight: Add TPS65217 WLED driver")
> Cc: stable <stable@vger.kernel.org>     # 3.7
> Cc: Matthias Kaehlcke <matthias@kaehlcke.net>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>   drivers/video/backlight/tps65217_bl.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/tps65217_bl.c b/drivers/video/backlight/tps65217_bl.c
> index 380917c86276..762e3feed097 100644
> --- a/drivers/video/backlight/tps65217_bl.c
> +++ b/drivers/video/backlight/tps65217_bl.c
> @@ -184,11 +184,11 @@ static struct tps65217_bl_pdata *
>   tps65217_bl_parse_dt(struct platform_device *pdev)
>   {
>   	struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
> -	struct device_node *node = of_node_get(tps->dev->of_node);
> +	struct device_node *node;
>   	struct tps65217_bl_pdata *pdata, *err;
>   	u32 val;
>   
> -	node = of_find_node_by_name(node, "backlight");
> +	node = of_get_child_by_name(tps->dev->of_node, "backlight");
>   	if (!node)
>   		return ERR_PTR(-ENODEV);
>   
> 

Re: [PATCH 2/3] backlight: max8925_bl: fix device-tree node lookup

[Daniel Thompson]

On 13/11/17 10:20, Johan Hovold wrote:
> Fix child-node lookup during probe, which ended up searching the whole
> device tree depth-first starting at the parent rather than just matching
> on its children.
> 
> To make things worse, the parent mfd node was also prematurely freed,
> while the child backlight node was leaked.
> 
> Fixes: 47ec340cb8e2 ("mfd: max8925: Support dt for backlight")
> Cc: stable <stable@vger.kernel.org>     # 3.9
> Cc: Qing Xu <qingx@marvell.com>
> Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>   drivers/video/backlight/max8925_bl.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/backlight/max8925_bl.c b/drivers/video/backlight/max8925_bl.c
> index 7b738d60ecc2..f3aa6088f1d9 100644
> --- a/drivers/video/backlight/max8925_bl.c
> +++ b/drivers/video/backlight/max8925_bl.c
> @@ -116,7 +116,7 @@ static void max8925_backlight_dt_init(struct platform_device *pdev)
>   	if (!pdata)
>   		return;
>   
> -	np = of_find_node_by_name(nproot, "backlight");
> +	np = of_get_child_by_name(nproot, "backlight");
>   	if (!np) {
>   		dev_err(&pdev->dev, "failed to find backlight node\n");
>   		return;
> @@ -125,6 +125,8 @@ static void max8925_backlight_dt_init(struct platform_device *pdev)
>   	if (!of_property_read_u32(np, "maxim,max8925-dual-string", &val))
>   		pdata->dual_string = val;
>   
> +	of_node_put(np);
> +
>   	pdev->dev.platform_data = pdata;
>   }
>   
> 

Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Daniel Thompson]

On 13/11/17 10:20, Johan Hovold wrote:
> Fix child-node lookup during probe, which ended up searching the whole
> device tree depth-first starting at the parent rather than just matching
> on its children.
> 
> To make things worse, the parent mfd node was also prematurely freed.
> 
> Note that the nodes returned from the two calls to of_parse_phandle()
> are also leaking, but fixing that is a bit more involved as pointers to
> node fields are being stored for later use.

Is using a devm_kstrdup() to remember the full_name sufficient so get 
each of the FIXMEs cleaned up as well?


Daniel.


> Fixes: 59eb2b5e57ea ("drivers/video/backlight/as3711_bl.c: add OF support")
> Cc: stable <stable@vger.kernel.org>     # 3.10
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/video/backlight/as3711_bl.c | 35 +++++++++++++++++++++++++----------
>   1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
> index 734a9158946b..21ce56053c88 100644
> --- a/drivers/video/backlight/as3711_bl.c
> +++ b/drivers/video/backlight/as3711_bl.c
> @@ -262,15 +262,16 @@ static int as3711_bl_register(struct platform_device *pdev,
>   static int as3711_backlight_parse_dt(struct device *dev)
>   {
>   	struct as3711_bl_pdata *pdata = dev_get_platdata(dev);
> -	struct device_node *bl =
> -		of_find_node_by_name(dev->parent->of_node, "backlight"), *fb;
> +	struct device_node *bl, *fb;
>   	int ret;
>   
> +	bl = of_get_child_by_name(dev->parent->of_node, "backlight");
>   	if (!bl) {
>   		dev_dbg(dev, "backlight node not found\n");
>   		return -ENODEV;
>   	}
>   
> +	/* FIXME: need to drop reference to returned node */
>   	fb = of_parse_phandle(bl, "su1-dev", 0);
>   	if (fb) {
>   		pdata->su1_fb = fb->full_name;
> @@ -279,9 +280,10 @@ static int as3711_backlight_parse_dt(struct device *dev)
>   		if (pdata->su1_max_uA <= 0)
>   			ret = -EINVAL;
>   		if (ret < 0)
> -			return ret;
> +			goto err_put_bl;
>   	}
>   
> +	/* FIXME: need to drop reference to returned node */
>   	fb = of_parse_phandle(bl, "su2-dev", 0);
>   	if (fb) {
>   		int count = 0;
> @@ -292,7 +294,7 @@ static int as3711_backlight_parse_dt(struct device *dev)
>   		if (pdata->su2_max_uA <= 0)
>   			ret = -EINVAL;
>   		if (ret < 0)
> -			return ret;
> +			goto err_put_bl;
>   
>   		if (of_find_property(bl, "su2-feedback-voltage", NULL)) {
>   			pdata->su2_feedback = AS3711_SU2_VOLTAGE;
> @@ -314,8 +316,10 @@ static int as3711_backlight_parse_dt(struct device *dev)
>   			pdata->su2_feedback = AS3711_SU2_CURR_AUTO;
>   			count++;
>   		}
> -		if (count != 1)
> -			return -EINVAL;
> +		if (count != 1) {
> +			ret = -EINVAL;
> +			goto err_put_bl;
> +		}
>   
>   		count = 0;
>   		if (of_find_property(bl, "su2-fbprot-lx-sd4", NULL)) {
> @@ -334,8 +338,10 @@ static int as3711_backlight_parse_dt(struct device *dev)
>   			pdata->su2_fbprot = AS3711_SU2_GPIO4;
>   			count++;
>   		}
> -		if (count != 1)
> -			return -EINVAL;
> +		if (count != 1) {
> +			ret = -EINVAL;
> +			goto err_put_bl;
> +		}
>   
>   		count = 0;
>   		if (of_find_property(bl, "su2-auto-curr1", NULL)) {
> @@ -355,11 +361,20 @@ static int as3711_backlight_parse_dt(struct device *dev)
>   		 * At least one su2-auto-curr* must be specified iff
>   		 * AS3711_SU2_CURR_AUTO is used
>   		 */
> -		if (!count ^ (pdata->su2_feedback != AS3711_SU2_CURR_AUTO))
> -			return -EINVAL;
> +		if (!count ^ (pdata->su2_feedback != AS3711_SU2_CURR_AUTO)) {
> +			ret = -EINVAL;
> +			goto err_put_bl;
> +		}
>   	}
>   
> +	of_node_put(bl);
> +
>   	return 0;
> +
> +err_put_bl:
> +	of_node_put(bl);
> +
> +	return ret;
>   }
>   
>   static int as3711_backlight_probe(struct platform_device *pdev)
> 

Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Johan Hovold]

On Mon, Nov 13, 2017 at 02:16:09PM +0000, Daniel Thompson wrote:
> On 13/11/17 10:20, Johan Hovold wrote:
> > Fix child-node lookup during probe, which ended up searching the whole
> > device tree depth-first starting at the parent rather than just matching
> > on its children.
> > 
> > To make things worse, the parent mfd node was also prematurely freed.
> > 
> > Note that the nodes returned from the two calls to of_parse_phandle()
> > are also leaking, but fixing that is a bit more involved as pointers to
> > node fields are being stored for later use.
> 
> Is using a devm_kstrdup() to remember the full_name sufficient so get 
> each of the FIXMEs cleaned up as well?

Yeah, that may be sufficient, but looking closer at this now, it seems
the name pointers (su1_fb and su2_fb) are only used as booleans, and the
fb_name pointer in struct as3711_bl_data is never used at all.

So cleaning that up somehow (e.g. and maybe even dropping non-dt
probing) would also work.

But since this is a separate, and less critical issue, I think it needs
to be done as a follow up to this one.

Thanks,
Johan

Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Daniel Thompson]

On 14/11/17 18:05, Johan Hovold wrote:
> On Mon, Nov 13, 2017 at 02:16:09PM +0000, Daniel Thompson wrote:
>> On 13/11/17 10:20, Johan Hovold wrote:
>>> Fix child-node lookup during probe, which ended up searching the whole
>>> device tree depth-first starting at the parent rather than just matching
>>> on its children.
>>>
>>> To make things worse, the parent mfd node was also prematurely freed.
>>>
>>> Note that the nodes returned from the two calls to of_parse_phandle()
>>> are also leaking, but fixing that is a bit more involved as pointers to
>>> node fields are being stored for later use.
>>
>> Is using a devm_kstrdup() to remember the full_name sufficient so get
>> each of the FIXMEs cleaned up as well?
> 
> Yeah, that may be sufficient, but looking closer at this now, it seems
> the name pointers (su1_fb and su2_fb) are only used as booleans, and the
> fb_name pointer in struct as3711_bl_data is never used at all.
> 
> So cleaning that up somehow (e.g. and maybe even dropping non-dt
> probing) would also work.
> 
> But since this is a separate, and less critical issue, I think it needs
> to be done as a follow up to this one.

To be honest it was adding the separate and less critical FIXMEs into 
the patches that attracted my attention in the first place. ;-)


Daniel.

Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Johan Hovold]

On Tue, Nov 14, 2017 at 07:48:09PM +0000, Daniel Thompson wrote:
> On 14/11/17 18:05, Johan Hovold wrote:
> > On Mon, Nov 13, 2017 at 02:16:09PM +0000, Daniel Thompson wrote:
> >> On 13/11/17 10:20, Johan Hovold wrote:
> >>> Fix child-node lookup during probe, which ended up searching the whole
> >>> device tree depth-first starting at the parent rather than just matching
> >>> on its children.
> >>>
> >>> To make things worse, the parent mfd node was also prematurely freed.
> >>>
> >>> Note that the nodes returned from the two calls to of_parse_phandle()
> >>> are also leaking, but fixing that is a bit more involved as pointers to
> >>> node fields are being stored for later use.
> >>
> >> Is using a devm_kstrdup() to remember the full_name sufficient so get
> >> each of the FIXMEs cleaned up as well?
> > 
> > Yeah, that may be sufficient, but looking closer at this now, it seems
> > the name pointers (su1_fb and su2_fb) are only used as booleans, and the
> > fb_name pointer in struct as3711_bl_data is never used at all.
> > 
> > So cleaning that up somehow (e.g. and maybe even dropping non-dt
> > probing) would also work.
> > 
> > But since this is a separate, and less critical issue, I think it needs
> > to be done as a follow up to this one.
> 
> To be honest it was adding the separate and less critical FIXMEs into 
> the patches that attracted my attention in the first place. ;-)

Heh. Since I was touching those error paths, I at least wanted to record
somehow there were further issues to be addressed. But feel free to drop
the FIXMEs if you prefer.

Thanks,
Johan

Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Lee Jones]

On Wed, 15 Nov 2017, Johan Hovold wrote:

> On Tue, Nov 14, 2017 at 07:48:09PM +0000, Daniel Thompson wrote:
> > On 14/11/17 18:05, Johan Hovold wrote:
> > > On Mon, Nov 13, 2017 at 02:16:09PM +0000, Daniel Thompson wrote:
> > >> On 13/11/17 10:20, Johan Hovold wrote:
> > >>> Fix child-node lookup during probe, which ended up searching the whole
> > >>> device tree depth-first starting at the parent rather than just matching
> > >>> on its children.
> > >>>
> > >>> To make things worse, the parent mfd node was also prematurely freed.
> > >>>
> > >>> Note that the nodes returned from the two calls to of_parse_phandle()
> > >>> are also leaking, but fixing that is a bit more involved as pointers to
> > >>> node fields are being stored for later use.
> > >>
> > >> Is using a devm_kstrdup() to remember the full_name sufficient so get
> > >> each of the FIXMEs cleaned up as well?
> > > 
> > > Yeah, that may be sufficient, but looking closer at this now, it seems
> > > the name pointers (su1_fb and su2_fb) are only used as booleans, and the
> > > fb_name pointer in struct as3711_bl_data is never used at all.
> > > 
> > > So cleaning that up somehow (e.g. and maybe even dropping non-dt
> > > probing) would also work.
> > > 
> > > But since this is a separate, and less critical issue, I think it needs
> > > to be done as a follow up to this one.
> > 
> > To be honest it was adding the separate and less critical FIXMEs into 
> > the patches that attracted my attention in the first place. ;-)
> 
> Heh. Since I was touching those error paths, I at least wanted to record
> somehow there were further issues to be addressed. But feel free to drop
> the FIXMEs if you prefer.

In my experience FIXME's tend not to get addressed:

$ git grep -i fixme | wc -l
4431

Submit patches instead. :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Johan Hovold]

On Wed, Nov 15, 2017 at 02:32:11PM +0000, Lee Jones wrote:
> On Wed, 15 Nov 2017, Johan Hovold wrote:
> 
> > On Tue, Nov 14, 2017 at 07:48:09PM +0000, Daniel Thompson wrote:
> > > On 14/11/17 18:05, Johan Hovold wrote:
> > > > On Mon, Nov 13, 2017 at 02:16:09PM +0000, Daniel Thompson wrote:
> > > >> On 13/11/17 10:20, Johan Hovold wrote:
> > > >>> Fix child-node lookup during probe, which ended up searching the whole
> > > >>> device tree depth-first starting at the parent rather than just matching
> > > >>> on its children.
> > > >>>
> > > >>> To make things worse, the parent mfd node was also prematurely freed.
> > > >>>
> > > >>> Note that the nodes returned from the two calls to of_parse_phandle()
> > > >>> are also leaking, but fixing that is a bit more involved as pointers to
> > > >>> node fields are being stored for later use.
> > > >>
> > > >> Is using a devm_kstrdup() to remember the full_name sufficient so get
> > > >> each of the FIXMEs cleaned up as well?
> > > > 
> > > > Yeah, that may be sufficient, but looking closer at this now, it seems
> > > > the name pointers (su1_fb and su2_fb) are only used as booleans, and the
> > > > fb_name pointer in struct as3711_bl_data is never used at all.
> > > > 
> > > > So cleaning that up somehow (e.g. and maybe even dropping non-dt
> > > > probing) would also work.
> > > > 
> > > > But since this is a separate, and less critical issue, I think it needs
> > > > to be done as a follow up to this one.
> > > 
> > > To be honest it was adding the separate and less critical FIXMEs into 
> > > the patches that attracted my attention in the first place. ;-)
> > 
> > Heh. Since I was touching those error paths, I at least wanted to record
> > somehow there were further issues to be addressed. But feel free to drop
> > the FIXMEs if you prefer.
> 
> In my experience FIXME's tend not to get addressed:
> 
> $ git grep -i fixme | wc -l
> 4431
> 
> Submit patches instead. :)

There may be some truth to that, but I still think it's better to mark
what is broken (especially since a leaked node is no big deal in this
case) than to just ignore and forget about it.

Johan

Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Johan Hovold]

On Wed, Nov 15, 2017 at 03:39:09PM +0100, Johan Hovold wrote:
> On Wed, Nov 15, 2017 at 02:32:11PM +0000, Lee Jones wrote:
> > On Wed, 15 Nov 2017, Johan Hovold wrote:
> > 
> > > On Tue, Nov 14, 2017 at 07:48:09PM +0000, Daniel Thompson wrote:
> > > > On 14/11/17 18:05, Johan Hovold wrote:
> > > > > On Mon, Nov 13, 2017 at 02:16:09PM +0000, Daniel Thompson wrote:
> > > > >> On 13/11/17 10:20, Johan Hovold wrote:
> > > > >>> Fix child-node lookup during probe, which ended up searching the whole
> > > > >>> device tree depth-first starting at the parent rather than just matching
> > > > >>> on its children.
> > > > >>>
> > > > >>> To make things worse, the parent mfd node was also prematurely freed.
> > > > >>>
> > > > >>> Note that the nodes returned from the two calls to of_parse_phandle()
> > > > >>> are also leaking, but fixing that is a bit more involved as pointers to
> > > > >>> node fields are being stored for later use.
> > > > >>
> > > > >> Is using a devm_kstrdup() to remember the full_name sufficient so get
> > > > >> each of the FIXMEs cleaned up as well?
> > > > > 
> > > > > Yeah, that may be sufficient, but looking closer at this now, it seems
> > > > > the name pointers (su1_fb and su2_fb) are only used as booleans, and the
> > > > > fb_name pointer in struct as3711_bl_data is never used at all.
> > > > > 
> > > > > So cleaning that up somehow (e.g. and maybe even dropping non-dt
> > > > > probing) would also work.
> > > > > 
> > > > > But since this is a separate, and less critical issue, I think it needs
> > > > > to be done as a follow up to this one.
> > > > 
> > > > To be honest it was adding the separate and less critical FIXMEs into 
> > > > the patches that attracted my attention in the first place. ;-)
> > > 
> > > Heh. Since I was touching those error paths, I at least wanted to record
> > > somehow there were further issues to be addressed. But feel free to drop
> > > the FIXMEs if you prefer.
> > 
> > In my experience FIXME's tend not to get addressed:
> > 
> > $ git grep -i fixme | wc -l
> > 4431
> > 
> > Submit patches instead. :)
> 
> There may be some truth to that, but I still think it's better to mark
> what is broken (especially since a leaked node is no big deal in this
> case) than to just ignore and forget about it.

I just sent a v2 including a new patch fixing these node leaks instead
of just flagging them. The driver really had no business storing those
node full_name fields in the first place.

Johan

Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

[Lee Jones]

On Mon, 20 Nov 2017, Johan Hovold wrote:

> On Wed, Nov 15, 2017 at 03:39:09PM +0100, Johan Hovold wrote:
> > On Wed, Nov 15, 2017 at 02:32:11PM +0000, Lee Jones wrote:
> > > On Wed, 15 Nov 2017, Johan Hovold wrote:
> > > 
> > > > On Tue, Nov 14, 2017 at 07:48:09PM +0000, Daniel Thompson wrote:
> > > > > On 14/11/17 18:05, Johan Hovold wrote:
> > > > > > On Mon, Nov 13, 2017 at 02:16:09PM +0000, Daniel Thompson wrote:
> > > > > >> On 13/11/17 10:20, Johan Hovold wrote:
> > > > > >>> Fix child-node lookup during probe, which ended up searching the whole
> > > > > >>> device tree depth-first starting at the parent rather than just matching
> > > > > >>> on its children.
> > > > > >>>
> > > > > >>> To make things worse, the parent mfd node was also prematurely freed.
> > > > > >>>
> > > > > >>> Note that the nodes returned from the two calls to of_parse_phandle()
> > > > > >>> are also leaking, but fixing that is a bit more involved as pointers to
> > > > > >>> node fields are being stored for later use.
> > > > > >>
> > > > > >> Is using a devm_kstrdup() to remember the full_name sufficient so get
> > > > > >> each of the FIXMEs cleaned up as well?
> > > > > > 
> > > > > > Yeah, that may be sufficient, but looking closer at this now, it seems
> > > > > > the name pointers (su1_fb and su2_fb) are only used as booleans, and the
> > > > > > fb_name pointer in struct as3711_bl_data is never used at all.
> > > > > > 
> > > > > > So cleaning that up somehow (e.g. and maybe even dropping non-dt
> > > > > > probing) would also work.
> > > > > > 
> > > > > > But since this is a separate, and less critical issue, I think it needs
> > > > > > to be done as a follow up to this one.
> > > > > 
> > > > > To be honest it was adding the separate and less critical FIXMEs into 
> > > > > the patches that attracted my attention in the first place. ;-)
> > > > 
> > > > Heh. Since I was touching those error paths, I at least wanted to record
> > > > somehow there were further issues to be addressed. But feel free to drop
> > > > the FIXMEs if you prefer.
> > > 
> > > In my experience FIXME's tend not to get addressed:
> > > 
> > > $ git grep -i fixme | wc -l
> > > 4431
> > > 
> > > Submit patches instead. :)
> > 
> > There may be some truth to that, but I still think it's better to mark
> > what is broken (especially since a leaked node is no big deal in this
> > case) than to just ignore and forget about it.
> 
> I just sent a v2 including a new patch fixing these node leaks instead
> of just flagging them. The driver really had no business storing those
> node full_name fields in the first place.

That's more like it. :)

You're a star, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog