[PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Johan Hovold]

A helper purported to look up a child node based on its name was using
the wrong of-helper and ended up prematurely freeing the parent of-node
while searching the whole device tree depth-first starting at the parent
node.

Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
Cc: stable <stable@vger.kernel.org>     # 3.7
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: Marek Belisko <marek@goldelico.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/misc/twl4030-vibra.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
index 6c51d404874b..c37aea9ac272 100644
--- a/drivers/input/misc/twl4030-vibra.c
+++ b/drivers/input/misc/twl4030-vibra.c
@@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
 			 twl4030_vibra_suspend, twl4030_vibra_resume);
 
 static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
-			      struct device_node *node)
+			      struct device_node *parent)
 {
+	struct device_node *node;
+
 	if (pdata && pdata->coexist)
 		return true;
 
-	node = of_find_node_by_name(node, "codec");
+	node = of_get_child_by_name(parent, "codec");
 	if (node) {
 		of_node_put(node);
 		return true;
-- 
2.15.0

[PATCH 2/3] Input: twl6040-vibra: fix child-node lookup

[Johan Hovold]

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

Later sanity checks on node properties (which would likely be missing)
should prevent this from causing much trouble however, especially as the
original premature free of the parent node has already been fixed
separately (but that "fix" was apparently never backported to stable).

Fixes: e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
Fixes: c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")
Cc: stable <stable@vger.kernel.org>     # 3.6
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/misc/twl6040-vibra.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
index 5690eb7ff954..15e0d352c4cc 100644
--- a/drivers/input/misc/twl6040-vibra.c
+++ b/drivers/input/misc/twl6040-vibra.c
@@ -248,8 +248,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
 	int vddvibr_uV = 0;
 	int error;
 
-	of_node_get(twl6040_core_dev->of_node);
-	twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node,
+	twl6040_core_node = of_get_child_by_name(twl6040_core_dev->of_node,
 						 "vibra");
 	if (!twl6040_core_node) {
 		dev_err(&pdev->dev, "parent of node is missing?\n");
-- 
2.15.0

[PATCH 3/3] Input: 88pm860x-ts: fix child-node lookup

[Johan Hovold]

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

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

Fixes: 2e57d56747e6 ("mfd: 88pm860x: Device tree support")
Cc: stable <stable@vger.kernel.org>     # 3.7
Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/input/touchscreen/88pm860x-ts.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/88pm860x-ts.c b/drivers/input/touchscreen/88pm860x-ts.c
index 7ed828a51f4c..3486d9403805 100644
--- a/drivers/input/touchscreen/88pm860x-ts.c
+++ b/drivers/input/touchscreen/88pm860x-ts.c
@@ -126,7 +126,7 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
 	int data, n, ret;
 	if (!np)
 		return -ENODEV;
-	np = of_find_node_by_name(np, "touch");
+	np = of_get_child_by_name(np, "touch");
 	if (!np) {
 		dev_err(&pdev->dev, "Can't find touch node\n");
 		return -EINVAL;
@@ -144,13 +144,13 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
 	if (data) {
 		ret = pm860x_reg_write(i2c, PM8607_GPADC_MISC1, data);
 		if (ret < 0)
-			return -EINVAL;
+			goto err_put_node;
 	}
 	/* set tsi prebias time */
 	if (!of_property_read_u32(np, "marvell,88pm860x-tsi-prebias", &data)) {
 		ret = pm860x_reg_write(i2c, PM8607_TSI_PREBIAS, data);
 		if (ret < 0)
-			return -EINVAL;
+			goto err_put_node;
 	}
 	/* set prebias & prechg time of pen detect */
 	data = 0;
@@ -161,10 +161,18 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
 	if (data) {
 		ret = pm860x_reg_write(i2c, PM8607_PD_PREBIAS, data);
 		if (ret < 0)
-			return -EINVAL;
+			goto err_put_node;
 	}
 	of_property_read_u32(np, "marvell,88pm860x-resistor-X", res_x);
+
+	of_node_put(np);
+
 	return 0;
+
+err_put_node:
+	of_node_put(np);
+
+	return -EINVAL;
 }
 #else
 #define pm860x_touch_dt_init(x, y, z)	(-1)
-- 
2.15.0

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Dmitry Torokhov]

On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> A helper purported to look up a child node based on its name was using
> the wrong of-helper and ended up prematurely freeing the parent of-node
> while searching the whole device tree depth-first starting at the parent
> node.

Ugh, this all is pretty ugly business. Can we teach MFD to allow
specifying firmware node to be attached to the platform devices it
creates in mfd_add_device() so that the leaf drivers simply call
device_property_read_XXX() on their own device and not be bothered with
weird OF refcount issues or what node they need to locate and parse?

Thanks!

> 
> Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
> Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
> Cc: stable <stable@vger.kernel.org>     # 3.7
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Marek Belisko <marek@goldelico.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/input/misc/twl4030-vibra.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
> index 6c51d404874b..c37aea9ac272 100644
> --- a/drivers/input/misc/twl4030-vibra.c
> +++ b/drivers/input/misc/twl4030-vibra.c
> @@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
>  			 twl4030_vibra_suspend, twl4030_vibra_resume);
>  
>  static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
> -			      struct device_node *node)
> +			      struct device_node *parent)
>  {
> +	struct device_node *node;
> +
>  	if (pdata && pdata->coexist)
>  		return true;
>  
> -	node = of_find_node_by_name(node, "codec");
> +	node = of_get_child_by_name(parent, "codec");
>  	if (node) {
>  		of_node_put(node);
>  		return true;
> -- 
> 2.15.0
> 

-- 
Dmitry

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Johan Hovold]

[ +CC: Lee, Rob and device-tree list ]

On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > A helper purported to look up a child node based on its name was using
> > the wrong of-helper and ended up prematurely freeing the parent of-node
> > while searching the whole device tree depth-first starting at the parent
> > node.
> 
> Ugh, this all is pretty ugly business. Can we teach MFD to allow
> specifying firmware node to be attached to the platform devices it
> creates in mfd_add_device() so that the leaf drivers simply call
> device_property_read_XXX() on their own device and not be bothered with
> weird OF refcount issues or what node they need to locate and parse?

Yeah, that may have helped. You can actually specify a compatible string
in struct mfd_cell today which does make mfd_add_device() associate a
matching child node.

Some best practice regarding how to deal with MFD and device tree would
be good to determine and document too. For example, when should
of_platform_populate() be used in favour of mfd_add_device()?
And how best to deal with sibling nodes, which is part of the problem
here (I think the mfd should have provided a flag rather than having
subdrivers deal with sibling nodes, for example).

That said, driver authors using the wrong of-helper could possibly have
been avoided by amending the kernel docs (I'll do that as a follow up),
but once these incorrect usages get in, only review can prevent them
from being reproduced through copy-paste coding.

Johan

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Peter Ujfalusi]

On 2017-11-11 17:43, Johan Hovold wrote:
> A helper purported to look up a child node based on its name was using
> the wrong of-helper and ended up prematurely freeing the parent of-node
> while searching the whole device tree depth-first starting at the parent
> node.
> 
> Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
> Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
> Cc: stable <stable@vger.kernel.org>     # 3.7
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Marek Belisko <marek@goldelico.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> ---
>  drivers/input/misc/twl4030-vibra.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
> index 6c51d404874b..c37aea9ac272 100644
> --- a/drivers/input/misc/twl4030-vibra.c
> +++ b/drivers/input/misc/twl4030-vibra.c
> @@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
>  			 twl4030_vibra_suspend, twl4030_vibra_resume);
>  
>  static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
> -			      struct device_node *node)
> +			      struct device_node *parent)
>  {
> +	struct device_node *node;
> +
>  	if (pdata && pdata->coexist)
>  		return true;
>  
> -	node = of_find_node_by_name(node, "codec");
> +	node = of_get_child_by_name(parent, "codec");
>  	if (node) {
>  		of_node_put(node);
>  		return true;
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] Input: twl6040-vibra: fix child-node lookup

[Peter Ujfalusi]

On 2017-11-11 17:43, Johan Hovold wrote:
> Fix child-node lookup during probe, which ended up searching the whole
> device tree depth-first starting at parent rather than just matching on
> its children.
> 
> Later sanity checks on node properties (which would likely be missing)
> should prevent this from causing much trouble however, especially as the
> original premature free of the parent node has already been fixed
> separately (but that "fix" was apparently never backported to stable).
> 
> Fixes: e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
> Fixes: c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")
> Cc: stable <stable@vger.kernel.org>     # 3.6
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> ---
>  drivers/input/misc/twl6040-vibra.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
> index 5690eb7ff954..15e0d352c4cc 100644
> --- a/drivers/input/misc/twl6040-vibra.c
> +++ b/drivers/input/misc/twl6040-vibra.c
> @@ -248,8 +248,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
>  	int vddvibr_uV = 0;
>  	int error;
>  
> -	of_node_get(twl6040_core_dev->of_node);
> -	twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node,
> +	twl6040_core_node = of_get_child_by_name(twl6040_core_dev->of_node,
>  						 "vibra");
>  	if (!twl6040_core_node) {
>  		dev_err(&pdev->dev, "parent of node is missing?\n");
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Lee Jones]

On Sun, 12 Nov 2017, Johan Hovold wrote:

> [ +CC: Lee, Rob and device-tree list ]
> 
> On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > A helper purported to look up a child node based on its name was using
> > > the wrong of-helper and ended up prematurely freeing the parent of-node
> > > while searching the whole device tree depth-first starting at the parent
> > > node.
> > 
> > Ugh, this all is pretty ugly business. Can we teach MFD to allow
> > specifying firmware node to be attached to the platform devices it
> > creates in mfd_add_device() so that the leaf drivers simply call
> > device_property_read_XXX() on their own device and not be bothered with
> > weird OF refcount issues or what node they need to locate and parse?

If a child compatible is provided, we already set the child's
of_node.  It's then up to the driver (set) author(s) to use it in the
correct manner. 

> Yeah, that may have helped. You can actually specify a compatible string
> in struct mfd_cell today which does make mfd_add_device() associate a
> matching child node.
> 
> Some best practice regarding how to deal with MFD and device tree would
> be good to determine and document too. For example, when should
> of_platform_populate() be used in favour of mfd_add_device()?

When the device supports DT and its entire hierarchical layout, along
with all of its attributes can be expressed in DT.

> And how best to deal with sibling nodes, which is part of the problem
> here (I think the mfd should have provided a flag rather than having
> subdrivers deal with sibling nodes, for example).

I disagree.  The only properties the MFD (parent) driver is interested
in is ones which are shared across multiple child devices.
*Everything* which pertains to only a single child device should be
handled by its accompanying driver. 

> That said, driver authors using the wrong of-helper could possibly have
> been avoided by amending the kernel docs (I'll do that as a follow up),
> but once these incorrect usages get in, only review can prevent them
> from being reproduced through copy-paste coding.

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

Re: [PATCH 2/3] Input: twl6040-vibra: fix child-node lookup

[Johan Hovold]

On Mon, Nov 13, 2017 at 08:30:53AM +0100, H. Nikolaus Schaller wrote:
> Hi Johan,
> 
> > Am 11.11.2017 um 16:43 schrieb Johan Hovold <johan@kernel.org>:
> > 
> > Fix child-node lookup during probe, which ended up searching the whole
> > device tree depth-first starting at parent rather than just matching on
> > its children.
> > 
> > Later sanity checks on node properties (which would likely be missing)
> > should prevent this from causing much trouble however, especially as the
> > original premature free of the parent node has already been fixed
> > separately (but that "fix" was apparently never backported to stable).
> > 
> > Fixes: e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
> > Fixes: c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")
> > Cc: stable <stable@vger.kernel.org>     # 3.6
> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > Cc: H. Nikolaus Schaller <hns@goldelico.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> Tested-By: H. Nikolaus Schaller <hns@goldelico.com>	(on Pyra OMAP5 hardware)
> 
> I also tested your twl4030-vibra patch on GTA04.
> 
> No regressions observed.

Thanks for testing.

Johan

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Johan Hovold]

On Mon, Nov 13, 2017 at 09:11:44AM +0000, Lee Jones wrote:
> On Sun, 12 Nov 2017, Johan Hovold wrote:
> 
> > [ +CC: Lee, Rob and device-tree list ]
> > 
> > On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> > > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > > A helper purported to look up a child node based on its name was using
> > > > the wrong of-helper and ended up prematurely freeing the parent of-node
> > > > while searching the whole device tree depth-first starting at the parent
> > > > node.
> > > 
> > > Ugh, this all is pretty ugly business. Can we teach MFD to allow
> > > specifying firmware node to be attached to the platform devices it
> > > creates in mfd_add_device() so that the leaf drivers simply call
> > > device_property_read_XXX() on their own device and not be bothered with
> > > weird OF refcount issues or what node they need to locate and parse?
> 
> If a child compatible is provided, we already set the child's
> of_node.  It's then up to the driver (set) author(s) to use it in the
> correct manner. 
> 
> > Yeah, that may have helped. You can actually specify a compatible string
> > in struct mfd_cell today which does make mfd_add_device() associate a
> > matching child node.
> > 
> > Some best practice regarding how to deal with MFD and device tree would
> > be good to determine and document too. For example, when should
> > of_platform_populate() be used in favour of mfd_add_device()?
> 
> When the device supports DT and its entire hierarchical layout, along
> with all of its attributes can be expressed in DT.

Ok, a follow up: When there are different variants of an MFD and that
affects the child drivers, then that should be expressed in in the child
node compatibles rather than having the child match on the parent node?

I'm asking because this came up recently during review and their seems
to be no precedent for matching on the parent compatible in child
drivers:

	https://lkml.kernel.org/r/20171105154725.GA11226@localhost

> > And how best to deal with sibling nodes, which is part of the problem
> > here (I think the mfd should have provided a flag rather than having
> > subdrivers deal with sibling nodes, for example).
> 
> I disagree.  The only properties the MFD (parent) driver is interested
> in is ones which are shared across multiple child devices.
> *Everything* which pertains to only a single child device should be
> handled by its accompanying driver. 

Even if that means leaking details of one child driver into a sibling?
Isn't it then cleaner to use the parent MFD to coordinate between the
cells, just as we do for IO?

In this case a child driver looked up a sibling node based on name, but
that doesn't mean the node is active, that there's a driver bound, or
that the sibling node has some other random property for example. The
parent could be used for such coordination, if only to pass information
from one sibling to another.

Thanks,
Johan

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Lee Jones]

On Mon, 13 Nov 2017, Johan Hovold wrote:

> On Mon, Nov 13, 2017 at 09:11:44AM +0000, Lee Jones wrote:
> > On Sun, 12 Nov 2017, Johan Hovold wrote:
> > 
> > > [ +CC: Lee, Rob and device-tree list ]
> > > 
> > > On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> > > > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > > > A helper purported to look up a child node based on its name was using
> > > > > the wrong of-helper and ended up prematurely freeing the parent of-node
> > > > > while searching the whole device tree depth-first starting at the parent
> > > > > node.
> > > > 
> > > > Ugh, this all is pretty ugly business. Can we teach MFD to allow
> > > > specifying firmware node to be attached to the platform devices it
> > > > creates in mfd_add_device() so that the leaf drivers simply call
> > > > device_property_read_XXX() on their own device and not be bothered with
> > > > weird OF refcount issues or what node they need to locate and parse?
> > 
> > If a child compatible is provided, we already set the child's
> > of_node.  It's then up to the driver (set) author(s) to use it in the
> > correct manner. 
> > 
> > > Yeah, that may have helped. You can actually specify a compatible string
> > > in struct mfd_cell today which does make mfd_add_device() associate a
> > > matching child node.
> > > 
> > > Some best practice regarding how to deal with MFD and device tree would
> > > be good to determine and document too. For example, when should
> > > of_platform_populate() be used in favour of mfd_add_device()?
> > 
> > When the device supports DT and its entire hierarchical layout, along
> > with all of its attributes can be expressed in DT.
> 
> Ok, a follow up: When there are different variants of an MFD and that
> affects the child drivers, then that should be expressed in in the child
> node compatibles rather than having the child match on the parent node?
> 
> I'm asking because this came up recently during review and their seems
> to be no precedent for matching on the parent compatible in child
> drivers:
> 
> 	https://lkml.kernel.org/r/20171105154725.GA11226@localhost

Accessing the parent's of_device_id .data directly doesn't sit well
with me.  The parent driver should pass this type of configuration
though pdata IMHO.

> > > And how best to deal with sibling nodes, which is part of the problem
> > > here (I think the mfd should have provided a flag rather than having
> > > subdrivers deal with sibling nodes, for example).
> > 
> > I disagree.  The only properties the MFD (parent) driver is interested
> > in is ones which are shared across multiple child devices.
> > *Everything* which pertains to only a single child device should be
> > handled by its accompanying driver. 
> 
> Even if that means leaking details of one child driver into a sibling?

Not sure what you mean here.  Could you please elaborate or provide an
example?

> Isn't it then cleaner to use the parent MFD to coordinate between the
> cells, just as we do for IO?
> 
> In this case a child driver looked up a sibling node based on name, but

This should not be allowed.  If >1 sibling requires access to a
particular property, this is normally evidence enough that this
property should be shared and handled by the parent.

> that doesn't mean the node is active, that there's a driver bound, or
> that the sibling node has some other random property for example. The
> parent could be used for such coordination, if only to pass information
> from one sibling to another.

Right.

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

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Johan Hovold]

On Mon, Nov 13, 2017 at 10:20:28AM +0000, Lee Jones wrote:
> On Mon, 13 Nov 2017, Johan Hovold wrote:
> 
> > On Mon, Nov 13, 2017 at 09:11:44AM +0000, Lee Jones wrote:
> > > On Sun, 12 Nov 2017, Johan Hovold wrote:
> > > 
> > > > [ +CC: Lee, Rob and device-tree list ]
> > > > 
> > > > On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> > > > > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > > > > A helper purported to look up a child node based on its name was using
> > > > > > the wrong of-helper and ended up prematurely freeing the parent of-node
> > > > > > while searching the whole device tree depth-first starting at the parent
> > > > > > node.
> > > > > 
> > > > > Ugh, this all is pretty ugly business. Can we teach MFD to allow
> > > > > specifying firmware node to be attached to the platform devices it
> > > > > creates in mfd_add_device() so that the leaf drivers simply call
> > > > > device_property_read_XXX() on their own device and not be bothered with
> > > > > weird OF refcount issues or what node they need to locate and parse?
> > > 
> > > If a child compatible is provided, we already set the child's
> > > of_node.  It's then up to the driver (set) author(s) to use it in the
> > > correct manner. 
> > > 
> > > > Yeah, that may have helped. You can actually specify a compatible string
> > > > in struct mfd_cell today which does make mfd_add_device() associate a
> > > > matching child node.
> > > > 
> > > > Some best practice regarding how to deal with MFD and device tree would
> > > > be good to determine and document too. For example, when should
> > > > of_platform_populate() be used in favour of mfd_add_device()?
> > > 
> > > When the device supports DT and its entire hierarchical layout, along
> > > with all of its attributes can be expressed in DT.
> > 
> > Ok, a follow up: When there are different variants of an MFD and that
> > affects the child drivers, then that should be expressed in in the child
> > node compatibles rather than having the child match on the parent node?
> > 
> > I'm asking because this came up recently during review and their seems
> > to be no precedent for matching on the parent compatible in child
> > drivers:
> > 
> > 	https://lkml.kernel.org/r/20171105154725.GA11226@localhost
> 
> Accessing the parent's of_device_id .data directly doesn't sit well
> with me.  The parent driver should pass this type of configuration
> though pdata IMHO.

The child driver is only matching on the parent-node compatible string
IIRC, and therefore keeps its own table of all parent compatibles with
its own set of (child) private match data (i.e. the parent compatible
property is matched first by the parent driver, and then again by the
child).

Passing through pdata here is not possible since mfd_add_device() isn't
used, right? It could of course be described using properties of the
child node (e.g. by using different child compatible strings).

> > > > And how best to deal with sibling nodes, which is part of the problem
> > > > here (I think the mfd should have provided a flag rather than having
> > > > subdrivers deal with sibling nodes, for example).
> > > 
> > > I disagree.  The only properties the MFD (parent) driver is interested
> > > in is ones which are shared across multiple child devices.
> > > *Everything* which pertains to only a single child device should be
> > > handled by its accompanying driver. 
> > 
> > Even if that means leaking details of one child driver into a sibling?
> 
> Not sure what you mean here.  Could you please elaborate or provide an
> example?

I mean that the sibling node needs to be aware of the name, compatible
string, or other node properties of its sibling node to be able to parse
sibling nodes itself (rather than the sibling or parent doing so on its
behalf). But it seems you answer this below.

> > Isn't it then cleaner to use the parent MFD to coordinate between the
> > cells, just as we do for IO?
> > 
> > In this case a child driver looked up a sibling node based on name, but
> 
> This should not be allowed.  If >1 sibling requires access to a
> particular property, this is normally evidence enough that this
> property should be shared and handled by the parent.
> 
> > that doesn't mean the node is active, that there's a driver bound, or
> > that the sibling node has some other random property for example. The
> > parent could be used for such coordination, if only to pass information
> > from one sibling to another.
> 
> Right.

Ok, it seems we're in agreement here.

Given that MFD has evolved over time and device-tree support has been
added retroactively to some drivers, we've ended up with a multitude of
different ways of dealing with such issues. I think it may still be a
good idea to jot down some best practices for future driver developers.

Thanks,
Johan

Re: [PATCH 2/3] Input: twl6040-vibra: fix child-node lookup

[Peter Ujfalusi]

On 2017-11-11 17:43, Johan Hovold wrote:
> Fix child-node lookup during probe, which ended up searching the whole
> device tree depth-first starting at parent rather than just matching on
> its children.
> 
> Later sanity checks on node properties (which would likely be missing)
> should prevent this from causing much trouble however, especially as the
> original premature free of the parent node has already been fixed
> separately (but that "fix" was apparently never backported to stable).
> 
> Fixes: e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
> Fixes: c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")
> Cc: stable <stable@vger.kernel.org>     # 3.6

Note that of_get_child_by_name() is not present in 3.6, this was the
reason the original implementation was using of_find_node_by_name()

> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/input/misc/twl6040-vibra.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
> index 5690eb7ff954..15e0d352c4cc 100644
> --- a/drivers/input/misc/twl6040-vibra.c
> +++ b/drivers/input/misc/twl6040-vibra.c
> @@ -248,8 +248,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
>  	int vddvibr_uV = 0;
>  	int error;
>  
> -	of_node_get(twl6040_core_dev->of_node);
> -	twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node,
> +	twl6040_core_node = of_get_child_by_name(twl6040_core_dev->of_node,
>  						 "vibra");
>  	if (!twl6040_core_node) {
>  		dev_err(&pdev->dev, "parent of node is missing?\n");
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] Input: twl6040-vibra: fix child-node lookup

[Johan Hovold]

On Mon, Nov 13, 2017 at 04:10:52PM +0200, Peter Ujfalusi wrote:

> On 2017-11-11 17:43, Johan Hovold wrote:
> > Fix child-node lookup during probe, which ended up searching the whole
> > device tree depth-first starting at parent rather than just matching on
> > its children.
> > 
> > Later sanity checks on node properties (which would likely be missing)
> > should prevent this from causing much trouble however, especially as the
> > original premature free of the parent node has already been fixed
> > separately (but that "fix" was apparently never backported to stable).
> > 
> > Fixes: e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
> > Fixes: c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")
> > Cc: stable <stable@vger.kernel.org>     # 3.6
> 
> Note that of_get_child_by_name() is not present in 3.6, this was the
> reason the original implementation was using of_find_node_by_name()

Ah, ok. So the lookup would have needed to be open-coded using
for_each_child_of_node() back then, or if anyone wants to backport any
of these fixes to before v3.7.

Thanks,
Johan

Re: [PATCH 2/3] Input: twl6040-vibra: fix child-node lookup

[H. Nikolaus Schaller]

Hi,

> Am 13.11.2017 um 15:19 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Mon, Nov 13, 2017 at 04:10:52PM +0200, Peter Ujfalusi wrote:
> 
>> On 2017-11-11 17:43, Johan Hovold wrote:
>>> Fix child-node lookup during probe, which ended up searching the whole
>>> device tree depth-first starting at parent rather than just matching on
>>> its children.
>>> 
>>> Later sanity checks on node properties (which would likely be missing)
>>> should prevent this from causing much trouble however, especially as the
>>> original premature free of the parent node has already been fixed
>>> separately (but that "fix" was apparently never backported to stable).
>>> 
>>> Fixes: e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
>>> Fixes: c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")
>>> Cc: stable <stable@vger.kernel.org>     # 3.6
>> 
>> Note that of_get_child_by_name() is not present in 3.6, this was the
>> reason the original implementation was using of_find_node_by_name()
> 
> Ah, ok. So the lookup would have needed to be open-coded using
> for_each_child_of_node() back then, or if anyone wants to backport any
> of these fixes to before v3.7.

Just let me note that intensive DT support of OMAP4&5/ARM based boards didn't
start before ca. v3.13. So it is very unlikely that anyone is running a twl6040
based board with older kernels. It is different for twl4030/omap3 which was
used by several devices (N900, Pandora, GTA04) back in 2.6. So I don't
see a reason to backport this change to any kernel older than 3.16.

BR,
Nikolaus Schaller

Re: [PATCH 2/3] Input: twl6040-vibra: fix child-node lookup

[Johan Hovold]

On Mon, Nov 13, 2017 at 03:39:48PM +0100, H. Nikolaus Schaller wrote:
> Hi,
> 
> > Am 13.11.2017 um 15:19 schrieb Johan Hovold <johan@kernel.org>:
> > 
> > On Mon, Nov 13, 2017 at 04:10:52PM +0200, Peter Ujfalusi wrote:
> > 
> >> On 2017-11-11 17:43, Johan Hovold wrote:
> >>> Fix child-node lookup during probe, which ended up searching the whole
> >>> device tree depth-first starting at parent rather than just matching on
> >>> its children.
> >>> 
> >>> Later sanity checks on node properties (which would likely be missing)
> >>> should prevent this from causing much trouble however, especially as the
> >>> original premature free of the parent node has already been fixed
> >>> separately (but that "fix" was apparently never backported to stable).
> >>> 
> >>> Fixes: e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
> >>> Fixes: c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")
> >>> Cc: stable <stable@vger.kernel.org>     # 3.6
> >> 
> >> Note that of_get_child_by_name() is not present in 3.6, this was the
> >> reason the original implementation was using of_find_node_by_name()
> > 
> > Ah, ok. So the lookup would have needed to be open-coded using
> > for_each_child_of_node() back then, or if anyone wants to backport any
> > of these fixes to before v3.7.
> 
> Just let me note that intensive DT support of OMAP4&5/ARM based boards didn't
> start before ca. v3.13. So it is very unlikely that anyone is running a twl6040
> based board with older kernels. It is different for twl4030/omap3 which was
> used by several devices (N900, Pandora, GTA04) back in 2.6. So I don't
> see a reason to backport this change to any kernel older than 3.16.

Right, and we don't really have any active stable trees going that far
back either (possibly with the exception of 3.2, but those guys are on
their own ;) ).

Thanks,
Johan

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Dmitry Torokhov]

On Mon, Nov 13, 2017 at 12:51:02PM +0100, Johan Hovold wrote:
> On Mon, Nov 13, 2017 at 10:20:28AM +0000, Lee Jones wrote:
> > On Mon, 13 Nov 2017, Johan Hovold wrote:
> > 
> > > On Mon, Nov 13, 2017 at 09:11:44AM +0000, Lee Jones wrote:
> > > > On Sun, 12 Nov 2017, Johan Hovold wrote:
> > > > 
> > > > > [ +CC: Lee, Rob and device-tree list ]
> > > > > 
> > > > > On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> > > > > > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > > > > > A helper purported to look up a child node based on its name was using
> > > > > > > the wrong of-helper and ended up prematurely freeing the parent of-node
> > > > > > > while searching the whole device tree depth-first starting at the parent
> > > > > > > node.
> > > > > > 
> > > > > > Ugh, this all is pretty ugly business. Can we teach MFD to allow
> > > > > > specifying firmware node to be attached to the platform devices it
> > > > > > creates in mfd_add_device() so that the leaf drivers simply call
> > > > > > device_property_read_XXX() on their own device and not be bothered with
> > > > > > weird OF refcount issues or what node they need to locate and parse?
> > > > 
> > > > If a child compatible is provided, we already set the child's
> > > > of_node.  It's then up to the driver (set) author(s) to use it in the
> > > > correct manner. 
> > > > 
> > > > > Yeah, that may have helped. You can actually specify a compatible string
> > > > > in struct mfd_cell today which does make mfd_add_device() associate a
> > > > > matching child node.
> > > > > 
> > > > > Some best practice regarding how to deal with MFD and device tree would
> > > > > be good to determine and document too. For example, when should
> > > > > of_platform_populate() be used in favour of mfd_add_device()?
> > > > 
> > > > When the device supports DT and its entire hierarchical layout, along
> > > > with all of its attributes can be expressed in DT.
> > > 
> > > Ok, a follow up: When there are different variants of an MFD and that
> > > affects the child drivers, then that should be expressed in in the child
> > > node compatibles rather than having the child match on the parent node?
> > > 
> > > I'm asking because this came up recently during review and their seems
> > > to be no precedent for matching on the parent compatible in child
> > > drivers:
> > > 
> > > 	https://lkml.kernel.org/r/20171105154725.GA11226@localhost
> > 
> > Accessing the parent's of_device_id .data directly doesn't sit well
> > with me.  The parent driver should pass this type of configuration
> > though pdata IMHO.
> 
> The child driver is only matching on the parent-node compatible string
> IIRC, and therefore keeps its own table of all parent compatibles with
> its own set of (child) private match data (i.e. the parent compatible
> property is matched first by the parent driver, and then again by the
> child).
> 
> Passing through pdata here is not possible since mfd_add_device() isn't
> used, right? It could of course be described using properties of the
> child node (e.g. by using different child compatible strings).
> 
> > > > > And how best to deal with sibling nodes, which is part of the problem
> > > > > here (I think the mfd should have provided a flag rather than having
> > > > > subdrivers deal with sibling nodes, for example).
> > > > 
> > > > I disagree.  The only properties the MFD (parent) driver is interested
> > > > in is ones which are shared across multiple child devices.
> > > > *Everything* which pertains to only a single child device should be
> > > > handled by its accompanying driver. 
> > > 
> > > Even if that means leaking details of one child driver into a sibling?
> > 
> > Not sure what you mean here.  Could you please elaborate or provide an
> > example?
> 
> I mean that the sibling node needs to be aware of the name, compatible
> string, or other node properties of its sibling node to be able to parse
> sibling nodes itself (rather than the sibling or parent doing so on its
> behalf). But it seems you answer this below.
> 
> > > Isn't it then cleaner to use the parent MFD to coordinate between the
> > > cells, just as we do for IO?
> > > 
> > > In this case a child driver looked up a sibling node based on name, but
> > 
> > This should not be allowed.  If >1 sibling requires access to a
> > particular property, this is normally evidence enough that this
> > property should be shared and handled by the parent.
> > 
> > > that doesn't mean the node is active, that there's a driver bound, or
> > > that the sibling node has some other random property for example. The
> > > parent could be used for such coordination, if only to pass information
> > > from one sibling to another.
> > 
> > Right.
> 
> Ok, it seems we're in agreement here.
> 
> Given that MFD has evolved over time and device-tree support has been
> added retroactively to some drivers, we've ended up with a multitude of
> different ways of dealing with such issues. I think it may still be a
> good idea to jot down some best practices for future driver developers.

FWIW here is the patch allowing attaching fwnode to an MFD cell that is
not using of_compatible (because if historical reasons). Completely
untested as I do not have this hardware.

If this is somewhat acceptable I can untangle core from twl6040
changes.

Thanks.
> 
> Thanks,
> Johan

-- 
Dmitry


Attach fwnode to MFD cells

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/twl6040-vibra.c |   32 ++++++++++----------------------
 drivers/mfd/mfd-core.c             |    2 ++
 drivers/mfd/twl6040.c              |   14 +++++++-------
 include/linux/mfd/core.h           |    6 ++++++
 include/linux/mfd/twl6040.h        |    2 ++
 5 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
index 5690eb7ff954..bd9507cf5608 100644
--- a/drivers/input/misc/twl6040-vibra.c
+++ b/drivers/input/misc/twl6040-vibra.c
@@ -242,23 +242,13 @@ static SIMPLE_DEV_PM_OPS(twl6040_vibra_pm_ops, twl6040_vibra_suspend, NULL);
 static int twl6040_vibra_probe(struct platform_device *pdev)
 {
 	struct device *twl6040_core_dev = pdev->dev.parent;
-	struct device_node *twl6040_core_node;
 	struct vibra_info *info;
 	int vddvibl_uV = 0;
 	int vddvibr_uV = 0;
 	int error;
 
-	of_node_get(twl6040_core_dev->of_node);
-	twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node,
-						 "vibra");
-	if (!twl6040_core_node) {
-		dev_err(&pdev->dev, "parent of node is missing?\n");
-		return -EINVAL;
-	}
-
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info) {
-		of_node_put(twl6040_core_node);
 		dev_err(&pdev->dev, "couldn't allocate memory\n");
 		return -ENOMEM;
 	}
@@ -267,18 +257,16 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
 
 	info->twl6040 = dev_get_drvdata(pdev->dev.parent);
 
-	of_property_read_u32(twl6040_core_node, "ti,vibldrv-res",
-			     &info->vibldrv_res);
-	of_property_read_u32(twl6040_core_node, "ti,vibrdrv-res",
-			     &info->vibrdrv_res);
-	of_property_read_u32(twl6040_core_node, "ti,viblmotor-res",
-			     &info->viblmotor_res);
-	of_property_read_u32(twl6040_core_node, "ti,vibrmotor-res",
-			     &info->vibrmotor_res);
-	of_property_read_u32(twl6040_core_node, "ti,vddvibl-uV", &vddvibl_uV);
-	of_property_read_u32(twl6040_core_node, "ti,vddvibr-uV", &vddvibr_uV);
-
-	of_node_put(twl6040_core_node);
+	device_property_read_u32(&pdev->dev, "ti,vibldrv-res",
+				 &info->vibldrv_res);
+	device_property_read_u32(&pdev->dev, "ti,vibrdrv-res",
+				 &info->vibrdrv_res);
+	device_property_read_u32(&pdev->dev, "ti,viblmotor-res",
+				 &info->viblmotor_res);
+	device_property_read_u32(&pdev->dev, "ti,vibrmotor-res",
+				 &info->vibrmotor_res);
+	device_property_read_u32(&pdev->dev, "ti,vddvibl-uV", &vddvibl_uV);
+	device_property_read_u32(&pdev->dev, "ti,vddvibr-uV", &vddvibr_uV);
 
 	if ((!info->vibldrv_res && !info->viblmotor_res) ||
 	    (!info->vibrdrv_res && !info->vibrmotor_res)) {
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index c57e407020f1..2792fe40e2e4 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -186,6 +186,8 @@ static int mfd_add_device(struct device *parent, int id,
 
 	mfd_acpi_add_device(cell, pdev);
 
+	set_secondary_fwnode(&pdev->dev, cell->fwnode);
+
 	if (cell->pdata_size) {
 		ret = platform_device_add_data(pdev,
 					cell->platform_data, cell->pdata_size);
diff --git a/drivers/mfd/twl6040.c b/drivers/mfd/twl6040.c
index d66502d36ba0..2994a8885c7e 100644
--- a/drivers/mfd/twl6040.c
+++ b/drivers/mfd/twl6040.c
@@ -97,13 +97,9 @@ static struct reg_sequence twl6040_patch[] = {
 };
 
 
-static bool twl6040_has_vibra(struct device_node *node)
+static struct device_node *twl6040_get_vibra(struct device_node *node)
 {
-#ifdef CONFIG_OF
-	if (of_find_node_by_name(node, "vibra"))
-		return true;
-#endif
-	return false;
+	return node ? of_get_child_by_name(node, "vibra") : NULL;
 }
 
 int twl6040_reg_read(struct twl6040 *twl6040, unsigned int reg)
@@ -766,11 +762,13 @@ static int twl6040_probe(struct i2c_client *client,
 	children++;
 
 	/* Vibra input driver support */
-	if (twl6040_has_vibra(node)) {
+	twl6040->vibra_node = twl6040_get_vibra(node);
+	if (twl6040->vibra_node) {
 		irq = regmap_irq_get_virq(twl6040->irq_data, TWL6040_IRQ_VIB);
 
 		cell = &twl6040->cells[children];
 		cell->name = "twl6040-vibra";
+		cell->fwnode = &twl6040->vibra_node->fwnode;
 		twl6040_vibra_rsrc[0].start = irq;
 		twl6040_vibra_rsrc[0].end = irq;
 		cell->resources = twl6040_vibra_rsrc;
@@ -817,6 +815,8 @@ static int twl6040_remove(struct i2c_client *client)
 
 	mfd_remove_devices(&client->dev);
 
+	of_node_put(twl6040->vibra_node);
+
 	regulator_bulk_disable(TWL6040_NUM_SUPPLIES, twl6040->supplies);
 
 	return 0;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 99c0395fe1f9..452f9d4de98a 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -58,6 +58,12 @@ struct mfd_cell {
 	/* Matches ACPI */
 	const struct mfd_cell_acpi_match	*acpi_match;
 
+	/*
+	 * Secondary firmware node that should be associated with the
+	 * platform device corresponding to the cell.
+	 */
+	struct fwnode_handle	*fwnode;
+
 	/*
 	 * These resources can be specified relative to the parent device.
 	 * For accessing hardware you should use resources from the platform dev
diff --git a/include/linux/mfd/twl6040.h b/include/linux/mfd/twl6040.h
index a2e88761c09f..9eee44c4fc1f 100644
--- a/include/linux/mfd/twl6040.h
+++ b/include/linux/mfd/twl6040.h
@@ -232,6 +232,8 @@ struct twl6040 {
 	struct mfd_cell cells[TWL6040_CELLS];
 	struct completion ready;
 
+	struct device_node *vibra_node;
+
 	int audpwron;
 	int power_count;
 	int rev;

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Lee Jones]

On Mon, 13 Nov 2017, Dmitry Torokhov wrote:

> On Mon, Nov 13, 2017 at 12:51:02PM +0100, Johan Hovold wrote:
> > On Mon, Nov 13, 2017 at 10:20:28AM +0000, Lee Jones wrote:
> > > On Mon, 13 Nov 2017, Johan Hovold wrote:
> > > 
> > > > On Mon, Nov 13, 2017 at 09:11:44AM +0000, Lee Jones wrote:
> > > > > On Sun, 12 Nov 2017, Johan Hovold wrote:
> > > > > 
> > > > > > [ +CC: Lee, Rob and device-tree list ]
> > > > > > 
> > > > > > On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> > > > > > > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > > > > > > A helper purported to look up a child node based on its name was using
> > > > > > > > the wrong of-helper and ended up prematurely freeing the parent of-node
> > > > > > > > while searching the whole device tree depth-first starting at the parent
> > > > > > > > node.
> > > > > > > 
> > > > > > > Ugh, this all is pretty ugly business. Can we teach MFD to allow
> > > > > > > specifying firmware node to be attached to the platform devices it
> > > > > > > creates in mfd_add_device() so that the leaf drivers simply call
> > > > > > > device_property_read_XXX() on their own device and not be bothered with
> > > > > > > weird OF refcount issues or what node they need to locate and parse?
> > > > > 
> > > > > If a child compatible is provided, we already set the child's
> > > > > of_node.  It's then up to the driver (set) author(s) to use it in the
> > > > > correct manner. 
> > > > > 
> > > > > > Yeah, that may have helped. You can actually specify a compatible string
> > > > > > in struct mfd_cell today which does make mfd_add_device() associate a
> > > > > > matching child node.
> > > > > > 
> > > > > > Some best practice regarding how to deal with MFD and device tree would
> > > > > > be good to determine and document too. For example, when should
> > > > > > of_platform_populate() be used in favour of mfd_add_device()?
> > > > > 
> > > > > When the device supports DT and its entire hierarchical layout, along
> > > > > with all of its attributes can be expressed in DT.
> > > > 
> > > > Ok, a follow up: When there are different variants of an MFD and that
> > > > affects the child drivers, then that should be expressed in in the child
> > > > node compatibles rather than having the child match on the parent node?
> > > > 
> > > > I'm asking because this came up recently during review and their seems
> > > > to be no precedent for matching on the parent compatible in child
> > > > drivers:
> > > > 
> > > > 	https://lkml.kernel.org/r/20171105154725.GA11226@localhost
> > > 
> > > Accessing the parent's of_device_id .data directly doesn't sit well
> > > with me.  The parent driver should pass this type of configuration
> > > though pdata IMHO.
> > 
> > The child driver is only matching on the parent-node compatible string
> > IIRC, and therefore keeps its own table of all parent compatibles with
> > its own set of (child) private match data (i.e. the parent compatible
> > property is matched first by the parent driver, and then again by the
> > child).
> > 
> > Passing through pdata here is not possible since mfd_add_device() isn't
> > used, right? It could of course be described using properties of the
> > child node (e.g. by using different child compatible strings).
> > 
> > > > > > And how best to deal with sibling nodes, which is part of the problem
> > > > > > here (I think the mfd should have provided a flag rather than having
> > > > > > subdrivers deal with sibling nodes, for example).
> > > > > 
> > > > > I disagree.  The only properties the MFD (parent) driver is interested
> > > > > in is ones which are shared across multiple child devices.
> > > > > *Everything* which pertains to only a single child device should be
> > > > > handled by its accompanying driver. 
> > > > 
> > > > Even if that means leaking details of one child driver into a sibling?
> > > 
> > > Not sure what you mean here.  Could you please elaborate or provide an
> > > example?
> > 
> > I mean that the sibling node needs to be aware of the name, compatible
> > string, or other node properties of its sibling node to be able to parse
> > sibling nodes itself (rather than the sibling or parent doing so on its
> > behalf). But it seems you answer this below.
> > 
> > > > Isn't it then cleaner to use the parent MFD to coordinate between the
> > > > cells, just as we do for IO?
> > > > 
> > > > In this case a child driver looked up a sibling node based on name, but
> > > 
> > > This should not be allowed.  If >1 sibling requires access to a
> > > particular property, this is normally evidence enough that this
> > > property should be shared and handled by the parent.
> > > 
> > > > that doesn't mean the node is active, that there's a driver bound, or
> > > > that the sibling node has some other random property for example. The
> > > > parent could be used for such coordination, if only to pass information
> > > > from one sibling to another.
> > > 
> > > Right.
> > 
> > Ok, it seems we're in agreement here.
> > 
> > Given that MFD has evolved over time and device-tree support has been
> > added retroactively to some drivers, we've ended up with a multitude of
> > different ways of dealing with such issues. I think it may still be a
> > good idea to jot down some best practices for future driver developers.
> 
> FWIW here is the patch allowing attaching fwnode to an MFD cell that is
> not using of_compatible (because if historical reasons). Completely
> untested as I do not have this hardware.

I am not familiar with the device_* OF implementation, so find it
hard to provide a solid, knowledgeable review.  It looks okay in
principle.

I'd appreciate it if Rob or one of the other DT guys could cast an
eye though.

> If this is somewhat acceptable I can untangle core from twl6040
> changes.

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

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Johan Hovold]

On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> A helper purported to look up a child node based on its name was using
> the wrong of-helper and ended up prematurely freeing the parent of-node
> while searching the whole device tree depth-first starting at the parent
> node.
> 
> Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
> Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
> Cc: stable <stable@vger.kernel.org>     # 3.7
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Marek Belisko <marek@goldelico.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---

Just wanted to send a reminder about this series. I believe these bugs
need to be fixes regardless of whether MFD-core eventually provides some
means of avoiding such lookups in cell drivers (i.e. the discussion
which appears to have stalled).

Thanks,
Johan

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Johan Hovold]

On Mon, Dec 11, 2017 at 11:21:21AM +0100, Johan Hovold wrote:
> On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > A helper purported to look up a child node based on its name was using
> > the wrong of-helper and ended up prematurely freeing the parent of-node
> > while searching the whole device tree depth-first starting at the parent
> > node.
> > 
> > Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
> > Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
> > Cc: stable <stable@vger.kernel.org>     # 3.7
> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > Cc: Marek Belisko <marek@goldelico.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> 
> Just wanted to send a reminder about this series. I believe these bugs
> need to be fixes regardless of whether MFD-core eventually provides some
> means of avoiding such lookups in cell drivers (i.e. the discussion
> which appears to have stalled).

Another month, another reminder: Will you pick this series up for 4.16,
Dmitry?

Thanks,
Johan

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Dmitry Torokhov]

On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> A helper purported to look up a child node based on its name was using
> the wrong of-helper and ended up prematurely freeing the parent of-node
> while searching the whole device tree depth-first starting at the parent
> node.
> 
> Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
> Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
> Cc: stable <stable@vger.kernel.org>     # 3.7
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Marek Belisko <marek@goldelico.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Applied, thank you.

> ---
>  drivers/input/misc/twl4030-vibra.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
> index 6c51d404874b..c37aea9ac272 100644
> --- a/drivers/input/misc/twl4030-vibra.c
> +++ b/drivers/input/misc/twl4030-vibra.c
> @@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
>  			 twl4030_vibra_suspend, twl4030_vibra_resume);
>  
>  static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
> -			      struct device_node *node)
> +			      struct device_node *parent)
>  {
> +	struct device_node *node;
> +
>  	if (pdata && pdata->coexist)
>  		return true;
>  
> -	node = of_find_node_by_name(node, "codec");
> +	node = of_get_child_by_name(parent, "codec");
>  	if (node) {
>  		of_node_put(node);
>  		return true;
> -- 
> 2.15.0
> 

-- 
Dmitry

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Dmitry Torokhov]

On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> A helper purported to look up a child node based on its name was using
> the wrong of-helper and ended up prematurely freeing the parent of-node
> while searching the whole device tree depth-first starting at the parent
> node.
> 
> Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
> Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
> Cc: stable <stable@vger.kernel.org>     # 3.7
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Marek Belisko <marek@goldelico.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Applied, thank you.

> ---
>  drivers/input/misc/twl4030-vibra.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
> index 6c51d404874b..c37aea9ac272 100644
> --- a/drivers/input/misc/twl4030-vibra.c
> +++ b/drivers/input/misc/twl4030-vibra.c
> @@ -178,12 +178,14 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
>  			 twl4030_vibra_suspend, twl4030_vibra_resume);
>  
>  static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
> -			      struct device_node *node)
> +			      struct device_node *parent)
>  {
> +	struct device_node *node;
> +
>  	if (pdata && pdata->coexist)
>  		return true;
>  
> -	node = of_find_node_by_name(node, "codec");
> +	node = of_get_child_by_name(parent, "codec");
>  	if (node) {
>  		of_node_put(node);
>  		return true;
> -- 
> 2.15.0
> 

-- 
Dmitry

Re: [PATCH 2/3] Input: twl6040-vibra: fix child-node lookup

[Dmitry Torokhov]

On Sat, Nov 11, 2017 at 04:43:38PM +0100, Johan Hovold wrote:
> Fix child-node lookup during probe, which ended up searching the whole
> device tree depth-first starting at parent rather than just matching on
> its children.
> 
> Later sanity checks on node properties (which would likely be missing)
> should prevent this from causing much trouble however, especially as the
> original premature free of the parent node has already been fixed
> separately (but that "fix" was apparently never backported to stable).
> 
> Fixes: e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
> Fixes: c52c545ead97 ("Input: twl6040-vibra - fix DT node memory management")
> Cc: stable <stable@vger.kernel.org>     # 3.6
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Applied, thank you.

> ---
>  drivers/input/misc/twl6040-vibra.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
> index 5690eb7ff954..15e0d352c4cc 100644
> --- a/drivers/input/misc/twl6040-vibra.c
> +++ b/drivers/input/misc/twl6040-vibra.c
> @@ -248,8 +248,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
>  	int vddvibr_uV = 0;
>  	int error;
>  
> -	of_node_get(twl6040_core_dev->of_node);
> -	twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node,
> +	twl6040_core_node = of_get_child_by_name(twl6040_core_dev->of_node,
>  						 "vibra");
>  	if (!twl6040_core_node) {
>  		dev_err(&pdev->dev, "parent of node is missing?\n");
> -- 
> 2.15.0
> 

-- 
Dmitry

Re: [PATCH 3/3] Input: 88pm860x-ts: fix child-node lookup

[Dmitry Torokhov]

On Sat, Nov 11, 2017 at 04:43:39PM +0100, Johan Hovold wrote:
> Fix child node-lookup during probe, which ended up searching the whole
> device tree depth-first starting at parent rather than just matching on
> its children.
> 
> To make things worse, the parent node was prematurely freed, while the
> child node was leaked.
> 
> Fixes: 2e57d56747e6 ("mfd: 88pm860x: Device tree support")
> Cc: stable <stable@vger.kernel.org>     # 3.7
> Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Applied, thank you.

> ---
>  drivers/input/touchscreen/88pm860x-ts.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/88pm860x-ts.c b/drivers/input/touchscreen/88pm860x-ts.c
> index 7ed828a51f4c..3486d9403805 100644
> --- a/drivers/input/touchscreen/88pm860x-ts.c
> +++ b/drivers/input/touchscreen/88pm860x-ts.c
> @@ -126,7 +126,7 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
>  	int data, n, ret;
>  	if (!np)
>  		return -ENODEV;
> -	np = of_find_node_by_name(np, "touch");
> +	np = of_get_child_by_name(np, "touch");
>  	if (!np) {
>  		dev_err(&pdev->dev, "Can't find touch node\n");
>  		return -EINVAL;
> @@ -144,13 +144,13 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
>  	if (data) {
>  		ret = pm860x_reg_write(i2c, PM8607_GPADC_MISC1, data);
>  		if (ret < 0)
> -			return -EINVAL;
> +			goto err_put_node;
>  	}
>  	/* set tsi prebias time */
>  	if (!of_property_read_u32(np, "marvell,88pm860x-tsi-prebias", &data)) {
>  		ret = pm860x_reg_write(i2c, PM8607_TSI_PREBIAS, data);
>  		if (ret < 0)
> -			return -EINVAL;
> +			goto err_put_node;
>  	}
>  	/* set prebias & prechg time of pen detect */
>  	data = 0;
> @@ -161,10 +161,18 @@ static int pm860x_touch_dt_init(struct platform_device *pdev,
>  	if (data) {
>  		ret = pm860x_reg_write(i2c, PM8607_PD_PREBIAS, data);
>  		if (ret < 0)
> -			return -EINVAL;
> +			goto err_put_node;
>  	}
>  	of_property_read_u32(np, "marvell,88pm860x-resistor-X", res_x);
> +
> +	of_node_put(np);
> +
>  	return 0;
> +
> +err_put_node:
> +	of_node_put(np);
> +
> +	return -EINVAL;
>  }
>  #else
>  #define pm860x_touch_dt_init(x, y, z)	(-1)
> -- 
> 2.15.0
> 

-- 
Dmitry

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Dmitry Torokhov]

On Mon, Jan 08, 2018 at 02:55:02PM +0100, Johan Hovold wrote:
> On Mon, Dec 11, 2017 at 11:21:21AM +0100, Johan Hovold wrote:
> > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > A helper purported to look up a child node based on its name was using
> > > the wrong of-helper and ended up prematurely freeing the parent of-node
> > > while searching the whole device tree depth-first starting at the parent
> > > node.
> > > 
> > > Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
> > > Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
> > > Cc: stable <stable@vger.kernel.org>     # 3.7
> > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > > Cc: Marek Belisko <marek@goldelico.com>
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > 
> > Just wanted to send a reminder about this series. I believe these bugs
> > need to be fixes regardless of whether MFD-core eventually provides some
> > means of avoiding such lookups in cell drivers (i.e. the discussion
> > which appears to have stalled).
> 
> Another month, another reminder: Will you pick this series up for 4.16,
> Dmitry?

Sorry, I was hoping we'd have some movement in MFD... Applied all 3,
thank you.

-- 
Dmitry

Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup

[Johan Hovold]

On Mon, Jan 08, 2018 at 05:36:38PM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 08, 2018 at 02:55:02PM +0100, Johan Hovold wrote:
> > On Mon, Dec 11, 2017 at 11:21:21AM +0100, Johan Hovold wrote:
> > > On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > > > A helper purported to look up a child node based on its name was using
> > > > the wrong of-helper and ended up prematurely freeing the parent of-node
> > > > while searching the whole device tree depth-first starting at the parent
> > > > node.
> > > > 
> > > > Fixes: 64b9e4d803b1 ("input: twl4030-vibra: Support for DT booted kernel")
> > > > Fixes: e661d0a04462 ("Input: twl4030-vibra - fix ERROR: Bad of_node_put() warning")
> > > > Cc: stable <stable@vger.kernel.org>     # 3.7
> > > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > > > Cc: Marek Belisko <marek@goldelico.com>
> > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > ---
> > > 
> > > Just wanted to send a reminder about this series. I believe these bugs
> > > need to be fixes regardless of whether MFD-core eventually provides some
> > > means of avoiding such lookups in cell drivers (i.e. the discussion
> > > which appears to have stalled).
> > 
> > Another month, another reminder: Will you pick this series up for 4.16,
> > Dmitry?
> 
> Sorry, I was hoping we'd have some movement in MFD... Applied all 3,
> thank you.

No worries. The MFD changes can be done as follow-ons (when people can
find some spare cycles). I agree with the general direction.

Thanks,
Johan