[PATCH 1/5] of/device: Centralize checking of 'status' properties

[PATCH 1/5] of/device: Centralize checking of 'status' properties

[Deepak Saxena]

Don't call any of_platform_driver's probe() function if the device node is
not accessible (as denoted in the status property).

Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 arch/powerpc/platforms/83xx/suspend.c |    3 ---
 drivers/mmc/host/sdhci-of-core.c      |    3 ---
 drivers/net/gianfar.c                 |    2 +-
 drivers/net/ibm_newemac/core.c        |    2 +-
 drivers/of/platform.c                 |    7 +++++--
 5 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/suspend.c b/arch/powerpc/platforms/83xx/suspend.c
index 75ae77f..a5a54aa 100644
--- a/arch/powerpc/platforms/83xx/suspend.c
+++ b/arch/powerpc/platforms/83xx/suspend.c
@@ -326,9 +326,6 @@ static int pmc_probe(struct platform_device *ofdev,
 	struct pmc_type *type = match->data;
 	int ret = 0;
 
-	if (!of_device_is_available(np))
-		return -ENODEV;
-
 	has_deep_sleep = type->has_deep_sleep;
 	immrbase = get_immrbase();
 	pmc_dev = ofdev;
diff --git a/drivers/mmc/host/sdhci-of-core.c b/drivers/mmc/host/sdhci-of-core.c
index c51b711..51218d4 100644
--- a/drivers/mmc/host/sdhci-of-core.c
+++ b/drivers/mmc/host/sdhci-of-core.c
@@ -126,9 +126,6 @@ static int __devinit sdhci_of_probe(struct platform_device *ofdev,
 	int size;
 	int ret;
 
-	if (!of_device_is_available(np))
-		return -ENODEV;
-
 	host = sdhci_alloc_host(&ofdev->dev, sizeof(*of_host));
 	if (IS_ERR(host))
 		return -ENOMEM;
diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index d1bec62..9918914 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -620,7 +620,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 	unsigned int num_tx_qs, num_rx_qs;
 	u32 *tx_queues, *rx_queues;
 
-	if (!np || !of_device_is_available(np))
+	if (!np)
 		return -ENODEV;
 
 	/* parse the num of tx and rx queues */
diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index 06bb9b7..290fff2 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -2732,7 +2732,7 @@ static int __devinit emac_probe(struct platform_device *ofdev,
 	 * property here for now, but new flat device trees should set a
 	 * status property to "disabled" instead.
 	 */
-	if (of_get_property(np, "unused", NULL) || !of_device_is_available(np))
+	if (of_get_property(np, "unused", NULL))
 		return -ENODEV;
 
 	/* Find ourselves in the bootlist if we are there */
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 5b4a07f..14370a0 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -56,7 +56,10 @@ static int platform_driver_probe_shim(struct platform_device *pdev)
 	 * come up empty.  Return -EINVAL in this case so other drivers get
 	 * the chance to bind. */
 	match = of_match_device(pdev->dev.driver->of_match_table, &pdev->dev);
-	return match ? ofpdrv->probe(pdev, match) : -EINVAL;
+	if (match && of_device_is_available(pdev->dev.of_node))
+		return ofpdrv->probe(pdev, match);
+
+	return -EINVAL;
 }
 
 static void platform_driver_shutdown_shim(struct platform_device *pdev)
@@ -142,7 +145,7 @@ static int of_platform_device_probe(struct device *dev)
 	of_dev_get(of_dev);
 
 	match = of_match_device(drv->driver.of_match_table, dev);
-	if (match)
+	if (match && of_device_is_available(dev->of_node))
 		error = drv->probe(of_dev, match);
 	if (error)
 		of_dev_put(of_dev);
-- 
1.6.3.3

Re: [PATCH 1/5] of/device: Centralize checking of 'status' properties

[Grant Likely]

On Wed, Dec 08, 2010 at 11:29:13AM -0800, Deepak Saxena wrote:
> Don't call any of_platform_driver's probe() function if the device node is
> not accessible (as denoted in the status property).
> 
> Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com>
> Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com>
> ---
>  arch/powerpc/platforms/83xx/suspend.c |    3 ---
>  drivers/mmc/host/sdhci-of-core.c      |    3 ---
>  drivers/net/gianfar.c                 |    2 +-
>  drivers/net/ibm_newemac/core.c        |    2 +-
>  drivers/of/platform.c                 |    7 +++++--
>  5 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/83xx/suspend.c b/arch/powerpc/platforms/83xx/suspend.c
> index 75ae77f..a5a54aa 100644
> --- a/arch/powerpc/platforms/83xx/suspend.c
> +++ b/arch/powerpc/platforms/83xx/suspend.c
> @@ -326,9 +326,6 @@ static int pmc_probe(struct platform_device *ofdev,
>  	struct pmc_type *type = match->data;
>  	int ret = 0;
>  
> -	if (!of_device_is_available(np))
> -		return -ENODEV;
> -
>  	has_deep_sleep = type->has_deep_sleep;
>  	immrbase = get_immrbase();
>  	pmc_dev = ofdev;
> diff --git a/drivers/mmc/host/sdhci-of-core.c b/drivers/mmc/host/sdhci-of-core.c
> index c51b711..51218d4 100644
> --- a/drivers/mmc/host/sdhci-of-core.c
> +++ b/drivers/mmc/host/sdhci-of-core.c
> @@ -126,9 +126,6 @@ static int __devinit sdhci_of_probe(struct platform_device *ofdev,
>  	int size;
>  	int ret;
>  
> -	if (!of_device_is_available(np))
> -		return -ENODEV;
> -
>  	host = sdhci_alloc_host(&ofdev->dev, sizeof(*of_host));
>  	if (IS_ERR(host))
>  		return -ENOMEM;
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index d1bec62..9918914 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -620,7 +620,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
>  	unsigned int num_tx_qs, num_rx_qs;
>  	u32 *tx_queues, *rx_queues;
>  
> -	if (!np || !of_device_is_available(np))
> +	if (!np)
>  		return -ENODEV;
>  
>  	/* parse the num of tx and rx queues */
> diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
> index 06bb9b7..290fff2 100644
> --- a/drivers/net/ibm_newemac/core.c
> +++ b/drivers/net/ibm_newemac/core.c
> @@ -2732,7 +2732,7 @@ static int __devinit emac_probe(struct platform_device *ofdev,
>  	 * property here for now, but new flat device trees should set a
>  	 * status property to "disabled" instead.
>  	 */
> -	if (of_get_property(np, "unused", NULL) || !of_device_is_available(np))
> +	if (of_get_property(np, "unused", NULL))
>  		return -ENODEV;
>  
>  	/* Find ourselves in the bootlist if we are there */
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 5b4a07f..14370a0 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -56,7 +56,10 @@ static int platform_driver_probe_shim(struct platform_device *pdev)
>  	 * come up empty.  Return -EINVAL in this case so other drivers get
>  	 * the chance to bind. */
>  	match = of_match_device(pdev->dev.driver->of_match_table, &pdev->dev);
> -	return match ? ofpdrv->probe(pdev, match) : -EINVAL;
> +	if (match && of_device_is_available(pdev->dev.of_node))
> +		return ofpdrv->probe(pdev, match);
> +
> +	return -EINVAL;

Hi Deepak,

This works for drivers still using the deprecated of_platform_driver
structure, but all of those users are being converted into
platform_drivers which do not use this shim.  Fixing it here will not
work after conversion.

I think it would be better to inhibit the devices from getting registered in
the first place instead of short circuiting the probe path.

g.

>  }
>  
>  static void platform_driver_shutdown_shim(struct platform_device *pdev)
> @@ -142,7 +145,7 @@ static int of_platform_device_probe(struct device *dev)
>  	of_dev_get(of_dev);
>  
>  	match = of_match_device(drv->driver.of_match_table, dev);
> -	if (match)
> +	if (match && of_device_is_available(dev->of_node))
>  		error = drv->probe(of_dev, match);
>  	if (error)
>  		of_dev_put(of_dev);
> -- 
> 1.6.3.3
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 4/5] of/device: Create _of_get_next_child()

[Deepak Saxena]

There aren't many, but some of_get_next_child() callers do need an unabridged
view of the device tree so we provide a wrapper that can be used by them.

Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 drivers/of/base.c  |   26 ++++++++++++++++++++++++++
 include/linux/of.h |    2 ++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 81b2601..7bc1398 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -315,6 +315,32 @@ struct device_node *of_get_next_parent(struct device_node *node)
 }
 
 /**
+ *	_of_get_next_child - Iterate a node childs
+ *	@node:	parent node
+ *	@prev:	previous child of the parent node, or NULL to get first
+ *
+ *	Returns a node pointer with refcount incremented, use
+ *	of_node_put() on it when done.
+ *
+ *	Unlike most other node accessors, ignores status properties.
+ */
+struct device_node *_of_get_next_child(const struct device_node *node,
+	struct device_node *prev)
+{
+	struct device_node *next;
+
+	read_lock(&devtree_lock);
+	next = prev ? prev->sibling : node->child;
+	for (; next; next = next->sibling)
+		if (of_device_is_available(next) && of_node_get(next))
+			break;
+	of_node_put(prev);
+	read_unlock(&devtree_lock);
+	return next;
+}
+EXPORT_SYMBOL(_of_get_next_child);
+
+/**
  *	of_get_next_child - Iterate a node childs
  *	@node:	parent node
  *	@prev:	previous child of the parent node, or NULL to get first
diff --git a/include/linux/of.h b/include/linux/of.h
index cad7cf0..1fea01d 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -175,6 +175,8 @@ extern struct device_node *of_find_node_by_path(const char *path);
 extern struct device_node *of_find_node_by_phandle(phandle handle);
 extern struct device_node *of_get_parent(const struct device_node *node);
 extern struct device_node *of_get_next_parent(struct device_node *node);
+extern struct device_node *_of_get_next_child(const struct device_node *node,
+					     struct device_node *prev);
 extern struct device_node *of_get_next_child(const struct device_node *node,
 					     struct device_node *prev);
 #define for_each_child_of_node(parent, child) \
-- 
1.6.3.3

[PATCH 5/5] of/device: Show even unavailable nodes in procfs

[Deepak Saxena]

Use the new "raw" of_get_next_child() variant so all device tree nodes
will appear in procfs.

Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 fs/proc/proc_devtree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index d9396a4..aa914eb 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -188,7 +188,7 @@ void proc_device_tree_add_node(struct device_node *np,
 	const char *p;
 
 	set_node_proc_entry(np, de);
-	for (child = NULL; (child = of_get_next_child(np, child));) {
+	for (child = NULL; (child = _of_get_next_child(np, child));) {
 		/* Use everything after the last slash, or the full name */
 		p = strrchr(child->full_name, '/');
 		if (!p)
-- 
1.6.3.3

[PATCH 2/5] of/device: make for_each_node* check status properties

[Deepak Saxena]

Some early device drivers don't actually implement a struct of_platform_driver,
and instead use the for_each_node* iterators to search for matching device
nodes. Disabled device nodes will no longer be returned by these iterators.

Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 arch/powerpc/kernel/pci_of_scan.c         |    2 --
 arch/powerpc/platforms/83xx/usb.c         |    2 +-
 arch/powerpc/platforms/85xx/mpc85xx_mds.c |   13 ++-----------
 arch/powerpc/sysdev/fsl_pci.c             |    5 -----
 arch/powerpc/sysdev/ppc4xx_pci.c          |   15 ---------------
 drivers/of/base.c                         |   25 +++++++++++++++++++++++--
 6 files changed, 26 insertions(+), 36 deletions(-)

diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
index e751506..fa4f719 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -310,8 +310,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
 	/* Scan direct children */
 	for_each_child_of_node(node, child) {
 		pr_debug("  * %s\n", child->full_name);
-		if (!of_device_is_available(child))
-			continue;
 		reg = of_get_property(child, "reg", &reglen);
 		if (reg == NULL || reglen < 20)
 			continue;
diff --git a/arch/powerpc/platforms/83xx/usb.c b/arch/powerpc/platforms/83xx/usb.c
index 3ba4bb7..749c70b 100644
--- a/arch/powerpc/platforms/83xx/usb.c
+++ b/arch/powerpc/platforms/83xx/usb.c
@@ -211,7 +211,7 @@ int mpc837x_usb_cfg(void)
 	int ret = 0;
 
 	np = of_find_compatible_node(NULL, NULL, "fsl-usb2-dr");
-	if (!np || !of_device_is_available(np))
+	if (!np)
 		return -ENODEV;
 	prop = of_get_property(np, "phy_type", NULL);
 
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
index aa34cac..5d1a33f 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
@@ -170,10 +170,8 @@ static void __init mpc85xx_publish_qe_devices(void)
 	struct device_node *np;
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,qe");
-	if (!of_device_is_available(np)) {
-		of_node_put(np);
+	if (!np)
 		return;
-	}
 
 	of_platform_bus_probe(NULL, mpc85xx_qe_ids, NULL);
 }
@@ -267,11 +265,6 @@ static void __init mpc85xx_mds_qe_init(void)
 			return;
 	}
 
-	if (!of_device_is_available(np)) {
-		of_node_put(np);
-		return;
-	}
-
 	qe_reset();
 	of_node_put(np);
 
@@ -328,10 +321,8 @@ static void __init mpc85xx_mds_qeic_init(void)
 	struct device_node *np;
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,qe");
-	if (!of_device_is_available(np)) {
-		of_node_put(np);
+	if (!np)
 		return;
-	}
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
 	if (!np) {
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 818f7c6..fba4ec7 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -623,11 +623,6 @@ int __init mpc83xx_add_bridge(struct device_node *dev)
 
 	is_mpc83xx_pci = 1;
 
-	if (!of_device_is_available(dev)) {
-		pr_warning("%s: disabled by the firmware.\n",
-			   dev->full_name);
-		return -ENODEV;
-	}
 	pr_debug("Adding PCI host bridge %s\n", dev->full_name);
 
 	/* Fetch host bridge registers address */
diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
index 156aa7d..cf2adec 100644
--- a/arch/powerpc/sysdev/ppc4xx_pci.c
+++ b/arch/powerpc/sysdev/ppc4xx_pci.c
@@ -321,13 +321,6 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
 	const int *bus_range;
 	int primary = 0;
 
-	/* Check if device is enabled */
-	if (!of_device_is_available(np)) {
-		printk(KERN_INFO "%s: Port disabled via device-tree\n",
-		       np->full_name);
-		return;
-	}
-
 	/* Fetch config space registers address */
 	if (of_address_to_resource(np, 0, &rsrc_cfg)) {
 		printk(KERN_ERR "%s: Can't get PCI config register base !",
@@ -1890,14 +1883,6 @@ static void __init ppc4xx_probe_pciex_bridge(struct device_node *np)
 	port = &ppc4xx_pciex_ports[portno];
 	port->index = portno;
 
-	/*
-	 * Check if device is enabled
-	 */
-	if (!of_device_is_available(np)) {
-		printk(KERN_INFO "PCIE%d: Port disabled via device-tree\n", port->index);
-		return;
-	}
-
 	port->node = of_node_get(np);
 	pval = of_get_property(np, "sdr-base", NULL);
 	if (pval == NULL) {
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 710b53b..5d269a4 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -344,6 +344,8 @@ EXPORT_SYMBOL(of_get_next_child);
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
+ *
+ *	Does not return nodes marked unavailable by a status property.
  */
 struct device_node *of_find_node_by_path(const char *path)
 {
@@ -352,6 +354,7 @@ struct device_node *of_find_node_by_path(const char *path)
 	read_lock(&devtree_lock);
 	for (; np; np = np->allnext) {
 		if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
+			&& of_device_is_available(np)
 		    && of_node_get(np))
 			break;
 	}
@@ -370,6 +373,8 @@ EXPORT_SYMBOL(of_find_node_by_path);
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
+ *
+ *	Does not return nodes marked unavailable by a status property.
  */
 struct device_node *of_find_node_by_name(struct device_node *from,
 	const char *name)
@@ -380,6 +385,7 @@ struct device_node *of_find_node_by_name(struct device_node *from,
 	np = from ? from->allnext : allnodes;
 	for (; np; np = np->allnext)
 		if (np->name && (of_node_cmp(np->name, name) == 0)
+			&& of_device_is_available(np)
 		    && of_node_get(np))
 			break;
 	of_node_put(from);
@@ -399,6 +405,8 @@ EXPORT_SYMBOL(of_find_node_by_name);
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
+ *
+ *	Does not return nodes marked unavailable by a status property.
  */
 struct device_node *of_find_node_by_type(struct device_node *from,
 	const char *type)
@@ -409,6 +417,7 @@ struct device_node *of_find_node_by_type(struct device_node *from,
 	np = from ? from->allnext : allnodes;
 	for (; np; np = np->allnext)
 		if (np->type && (of_node_cmp(np->type, type) == 0)
+			&& of_device_is_available(np)
 		    && of_node_get(np))
 			break;
 	of_node_put(from);
@@ -430,6 +439,8 @@ EXPORT_SYMBOL(of_find_node_by_type);
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
+ *
+ *	Does not return nodes marked unavailable by a status property.
  */
 struct device_node *of_find_compatible_node(struct device_node *from,
 	const char *type, const char *compatible)
@@ -442,7 +453,9 @@ struct device_node *of_find_compatible_node(struct device_node *from,
 		if (type
 		    && !(np->type && (of_node_cmp(np->type, type) == 0)))
 			continue;
-		if (of_device_is_compatible(np, compatible) && of_node_get(np))
+		if (of_device_is_compatible(np, compatible)
+			&& of_device_is_available(np)
+			&& of_node_get(np))
 			break;
 	}
 	of_node_put(from);
@@ -462,6 +475,8 @@ EXPORT_SYMBOL(of_find_compatible_node);
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
+ *
+ *	Does not return nodes marked unavailable by a status property.
  */
 struct device_node *of_find_node_with_property(struct device_node *from,
 	const char *prop_name)
@@ -472,6 +487,8 @@ struct device_node *of_find_node_with_property(struct device_node *from,
 	read_lock(&devtree_lock);
 	np = from ? from->allnext : allnodes;
 	for (; np; np = np->allnext) {
+		if (!of_device_is_available(np))
+			continue;
 		for (pp = np->properties; pp != 0; pp = pp->next) {
 			if (of_prop_cmp(pp->name, prop_name) == 0) {
 				of_node_get(np);
@@ -526,6 +543,8 @@ EXPORT_SYMBOL(of_match_node);
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
+ *
+ *	Does not return nodes marked unavailable by a status property.
  */
 struct device_node *of_find_matching_node(struct device_node *from,
 					  const struct of_device_id *matches)
@@ -535,7 +554,9 @@ struct device_node *of_find_matching_node(struct device_node *from,
 	read_lock(&devtree_lock);
 	np = from ? from->allnext : allnodes;
 	for (; np; np = np->allnext) {
-		if (of_match_node(matches, np) && of_node_get(np))
+		if (of_match_node(matches, np)
+			&& of_device_is_available(np)
+			&& of_node_get(np))
 			break;
 	}
 	of_node_put(from);
-- 
1.6.3.3

Re: [PATCH 2/5] of/device: make for_each_node* check status properties

[Grant Likely]

On Wed, Dec 08, 2010 at 11:29:57AM -0800, Deepak Saxena wrote:
> Some early device drivers don't actually implement a struct of_platform_driver,
> and instead use the for_each_node* iterators to search for matching device
> nodes. Disabled device nodes will no longer be returned by these iterators.
> 
> Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>

As already discussed, the generic device tree iterator and search
functions should not be filtering the results as done here.

g.

> ---
>  arch/powerpc/kernel/pci_of_scan.c         |    2 --
>  arch/powerpc/platforms/83xx/usb.c         |    2 +-
>  arch/powerpc/platforms/85xx/mpc85xx_mds.c |   13 ++-----------
>  arch/powerpc/sysdev/fsl_pci.c             |    5 -----
>  arch/powerpc/sysdev/ppc4xx_pci.c          |   15 ---------------
>  drivers/of/base.c                         |   25 +++++++++++++++++++++++--
>  6 files changed, 26 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index e751506..fa4f719 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -310,8 +310,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
>  	/* Scan direct children */
>  	for_each_child_of_node(node, child) {
>  		pr_debug("  * %s\n", child->full_name);
> -		if (!of_device_is_available(child))
> -			continue;
>  		reg = of_get_property(child, "reg", &reglen);
>  		if (reg == NULL || reglen < 20)
>  			continue;
> diff --git a/arch/powerpc/platforms/83xx/usb.c b/arch/powerpc/platforms/83xx/usb.c
> index 3ba4bb7..749c70b 100644
> --- a/arch/powerpc/platforms/83xx/usb.c
> +++ b/arch/powerpc/platforms/83xx/usb.c
> @@ -211,7 +211,7 @@ int mpc837x_usb_cfg(void)
>  	int ret = 0;
>  
>  	np = of_find_compatible_node(NULL, NULL, "fsl-usb2-dr");
> -	if (!np || !of_device_is_available(np))
> +	if (!np)
>  		return -ENODEV;
>  	prop = of_get_property(np, "phy_type", NULL);
>  
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> index aa34cac..5d1a33f 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> @@ -170,10 +170,8 @@ static void __init mpc85xx_publish_qe_devices(void)
>  	struct device_node *np;
>  
>  	np = of_find_compatible_node(NULL, NULL, "fsl,qe");
> -	if (!of_device_is_available(np)) {
> -		of_node_put(np);
> +	if (!np)
>  		return;
> -	}
>  
>  	of_platform_bus_probe(NULL, mpc85xx_qe_ids, NULL);
>  }
> @@ -267,11 +265,6 @@ static void __init mpc85xx_mds_qe_init(void)
>  			return;
>  	}
>  
> -	if (!of_device_is_available(np)) {
> -		of_node_put(np);
> -		return;
> -	}
> -
>  	qe_reset();
>  	of_node_put(np);
>  
> @@ -328,10 +321,8 @@ static void __init mpc85xx_mds_qeic_init(void)
>  	struct device_node *np;
>  
>  	np = of_find_compatible_node(NULL, NULL, "fsl,qe");
> -	if (!of_device_is_available(np)) {
> -		of_node_put(np);
> +	if (!np)
>  		return;
> -	}
>  
>  	np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
>  	if (!np) {
> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
> index 818f7c6..fba4ec7 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -623,11 +623,6 @@ int __init mpc83xx_add_bridge(struct device_node *dev)
>  
>  	is_mpc83xx_pci = 1;
>  
> -	if (!of_device_is_available(dev)) {
> -		pr_warning("%s: disabled by the firmware.\n",
> -			   dev->full_name);
> -		return -ENODEV;
> -	}
>  	pr_debug("Adding PCI host bridge %s\n", dev->full_name);
>  
>  	/* Fetch host bridge registers address */
> diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
> index 156aa7d..cf2adec 100644
> --- a/arch/powerpc/sysdev/ppc4xx_pci.c
> +++ b/arch/powerpc/sysdev/ppc4xx_pci.c
> @@ -321,13 +321,6 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
>  	const int *bus_range;
>  	int primary = 0;
>  
> -	/* Check if device is enabled */
> -	if (!of_device_is_available(np)) {
> -		printk(KERN_INFO "%s: Port disabled via device-tree\n",
> -		       np->full_name);
> -		return;
> -	}
> -
>  	/* Fetch config space registers address */
>  	if (of_address_to_resource(np, 0, &rsrc_cfg)) {
>  		printk(KERN_ERR "%s: Can't get PCI config register base !",
> @@ -1890,14 +1883,6 @@ static void __init ppc4xx_probe_pciex_bridge(struct device_node *np)
>  	port = &ppc4xx_pciex_ports[portno];
>  	port->index = portno;
>  
> -	/*
> -	 * Check if device is enabled
> -	 */
> -	if (!of_device_is_available(np)) {
> -		printk(KERN_INFO "PCIE%d: Port disabled via device-tree\n", port->index);
> -		return;
> -	}
> -
>  	port->node = of_node_get(np);
>  	pval = of_get_property(np, "sdr-base", NULL);
>  	if (pval == NULL) {
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 710b53b..5d269a4 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -344,6 +344,8 @@ EXPORT_SYMBOL(of_get_next_child);
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
> + *
> + *	Does not return nodes marked unavailable by a status property.
>   */
>  struct device_node *of_find_node_by_path(const char *path)
>  {
> @@ -352,6 +354,7 @@ struct device_node *of_find_node_by_path(const char *path)
>  	read_lock(&devtree_lock);
>  	for (; np; np = np->allnext) {
>  		if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
> +			&& of_device_is_available(np)
>  		    && of_node_get(np))
>  			break;
>  	}
> @@ -370,6 +373,8 @@ EXPORT_SYMBOL(of_find_node_by_path);
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
> + *
> + *	Does not return nodes marked unavailable by a status property.
>   */
>  struct device_node *of_find_node_by_name(struct device_node *from,
>  	const char *name)
> @@ -380,6 +385,7 @@ struct device_node *of_find_node_by_name(struct device_node *from,
>  	np = from ? from->allnext : allnodes;
>  	for (; np; np = np->allnext)
>  		if (np->name && (of_node_cmp(np->name, name) == 0)
> +			&& of_device_is_available(np)
>  		    && of_node_get(np))
>  			break;
>  	of_node_put(from);
> @@ -399,6 +405,8 @@ EXPORT_SYMBOL(of_find_node_by_name);
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
> + *
> + *	Does not return nodes marked unavailable by a status property.
>   */
>  struct device_node *of_find_node_by_type(struct device_node *from,
>  	const char *type)
> @@ -409,6 +417,7 @@ struct device_node *of_find_node_by_type(struct device_node *from,
>  	np = from ? from->allnext : allnodes;
>  	for (; np; np = np->allnext)
>  		if (np->type && (of_node_cmp(np->type, type) == 0)
> +			&& of_device_is_available(np)
>  		    && of_node_get(np))
>  			break;
>  	of_node_put(from);
> @@ -430,6 +439,8 @@ EXPORT_SYMBOL(of_find_node_by_type);
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
> + *
> + *	Does not return nodes marked unavailable by a status property.
>   */
>  struct device_node *of_find_compatible_node(struct device_node *from,
>  	const char *type, const char *compatible)
> @@ -442,7 +453,9 @@ struct device_node *of_find_compatible_node(struct device_node *from,
>  		if (type
>  		    && !(np->type && (of_node_cmp(np->type, type) == 0)))
>  			continue;
> -		if (of_device_is_compatible(np, compatible) && of_node_get(np))
> +		if (of_device_is_compatible(np, compatible)
> +			&& of_device_is_available(np)
> +			&& of_node_get(np))
>  			break;
>  	}
>  	of_node_put(from);
> @@ -462,6 +475,8 @@ EXPORT_SYMBOL(of_find_compatible_node);
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
> + *
> + *	Does not return nodes marked unavailable by a status property.
>   */
>  struct device_node *of_find_node_with_property(struct device_node *from,
>  	const char *prop_name)
> @@ -472,6 +487,8 @@ struct device_node *of_find_node_with_property(struct device_node *from,
>  	read_lock(&devtree_lock);
>  	np = from ? from->allnext : allnodes;
>  	for (; np; np = np->allnext) {
> +		if (!of_device_is_available(np))
> +			continue;
>  		for (pp = np->properties; pp != 0; pp = pp->next) {
>  			if (of_prop_cmp(pp->name, prop_name) == 0) {
>  				of_node_get(np);
> @@ -526,6 +543,8 @@ EXPORT_SYMBOL(of_match_node);
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
> + *
> + *	Does not return nodes marked unavailable by a status property.
>   */
>  struct device_node *of_find_matching_node(struct device_node *from,
>  					  const struct of_device_id *matches)
> @@ -535,7 +554,9 @@ struct device_node *of_find_matching_node(struct device_node *from,
>  	read_lock(&devtree_lock);
>  	np = from ? from->allnext : allnodes;
>  	for (; np; np = np->allnext) {
> -		if (of_match_node(matches, np) && of_node_get(np))
> +		if (of_match_node(matches, np)
> +			&& of_device_is_available(np)
> +			&& of_node_get(np))
>  			break;
>  	}
>  	of_node_put(from);
> -- 
> 1.6.3.3
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 3/5] of/device: Make of_get_next_child() check status properties

[Deepak Saxena]

We only return the next child if the device is available.

Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com>
Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com>
---
 drivers/of/base.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5d269a4..81b2601 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
+ *
+ *	Does not return nodes marked unavailable by a status property.
  */
 struct device_node *of_get_next_child(const struct device_node *node,
 	struct device_node *prev)
@@ -330,7 +332,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
 	read_lock(&devtree_lock);
 	next = prev ? prev->sibling : node->child;
 	for (; next; next = next->sibling)
-		if (of_node_get(next))
+		if (of_device_is_available(next) && of_node_get(next))
 			break;
 	of_node_put(prev);
 	read_unlock(&devtree_lock);
-- 
1.6.3.3

Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties

[Scott Wood]

On Wed, 8 Dec 2010 11:29:44 -0800
Deepak Saxena <deepak_saxena@mentor.com> wrote:

> We only return the next child if the device is available.
> 
> Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com>
> Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com>
> ---
>  drivers/of/base.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5d269a4..81b2601 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
> + *
> + *	Does not return nodes marked unavailable by a status property.
>   */
>  struct device_node *of_get_next_child(const struct device_node *node,
>  	struct device_node *prev)
> @@ -330,7 +332,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
>  	read_lock(&devtree_lock);
>  	next = prev ? prev->sibling : node->child;
>  	for (; next; next = next->sibling)
> -		if (of_node_get(next))
> +		if (of_device_is_available(next) && of_node_get(next))
>  			break;
>  	of_node_put(prev);
>  	read_unlock(&devtree_lock);

This seems like too low-level a place to put this.  Some code may know
how to un-disable a device in certain situations, or it may be part of
debug code trying to dump the whole device tree, etc.  Looking
further[1], I see a raw version of this function, but not other things
like of_find_compatible_node.

Could this be done more othogonally, so that the caller specifies in a
parameter whether to skip based on status?

-Scott

[1] For some reason I received some of these patches from
linuxppc-dev, and others from devicetree-discuss.  I wish lists
wouldn't try to be "smart" about discarding duplicates -- it messes with
filters.

Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties

[Michael Ellerman]

[-- Attachment #1.1: Type: text/plain, Size: 2135 bytes --]

On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote:
> On Wed, 8 Dec 2010 11:29:44 -0800
> Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
> 
> > We only return the next child if the device is available.
> > 
> > Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/of/base.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 5d269a4..81b2601 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
> >   *
> >   *	Returns a node pointer with refcount incremented, use
> >   *	of_node_put() on it when done.
> > + *
> > + *	Does not return nodes marked unavailable by a status property.
> >   */
> >  struct device_node *of_get_next_child(const struct device_node *node,
> >  	struct device_node *prev)
> > @@ -330,7 +332,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
> >  	read_lock(&devtree_lock);
> >  	next = prev ? prev->sibling : node->child;
> >  	for (; next; next = next->sibling)
> > -		if (of_node_get(next))
> > +		if (of_device_is_available(next) && of_node_get(next))
> >  			break;
> >  	of_node_put(prev);
> >  	read_unlock(&devtree_lock);
> 
> This seems like too low-level a place to put this.  Some code may know
> how to un-disable a device in certain situations, or it may be part of
> debug code trying to dump the whole device tree, etc.  Looking
> further[1], I see a raw version of this function, but not other things
> like of_find_compatible_node.

Yeah I agree. I think we'll eventually end up with __ versions of all or
lots of them. Not to mention there might be cases you've missed where
code expects to see unavailable nodes. The right approach is to add
_new_ routines that don't return unavailable nodes, and convert code
that you know wants to use them.

cheers


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 2816 bytes --]

On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote:
> On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote:
> > On Wed, 8 Dec 2010 11:29:44 -0800
> > Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
> > 
> > > We only return the next child if the device is available.
> > > 
> > > Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  drivers/of/base.c |    4 +++-
> > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index 5d269a4..81b2601 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
> > >   *
> > >   *	Returns a node pointer with refcount incremented, use
> > >   *	of_node_put() on it when done.
> > > + *
> > > + *	Does not return nodes marked unavailable by a status property.
> > >   */
> > >  struct device_node *of_get_next_child(const struct device_node *node,
> > >  	struct device_node *prev)
> > > @@ -330,7 +332,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
> > >  	read_lock(&devtree_lock);
> > >  	next = prev ? prev->sibling : node->child;
> > >  	for (; next; next = next->sibling)
> > > -		if (of_node_get(next))
> > > +		if (of_device_is_available(next) && of_node_get(next))
> > >  			break;
> > >  	of_node_put(prev);
> > >  	read_unlock(&devtree_lock);
> > 
> > This seems like too low-level a place to put this.  Some code may know
> > how to un-disable a device in certain situations, or it may be part of
> > debug code trying to dump the whole device tree, etc.  Looking
> > further[1], I see a raw version of this function, but not other things
> > like of_find_compatible_node.
> 
> Yeah I agree. I think we'll eventually end up with __ versions of all or
> lots of them. Not to mention there might be cases you've missed where
> code expects to see unavailable nodes. The right approach is to add
> _new_ routines that don't return unavailable nodes, and convert code
> that you know wants to use them.

Actually, I don't think we really want these status-skipping
iterators at all.  The device tree iterators should give us the device
tree, as it is.  Those old-style drivers which seach for a node rather
than using the bus probing logic can keep individual checks of the
status property until they're converted to the new scheme.


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties

[Hollis Blanchard]

On Wed, Dec 8, 2010 at 7:09 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote:
>> On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote:
>> > On Wed, 8 Dec 2010 11:29:44 -0800
>> > Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> > > We only return the next child if the device is available.
>> > >
>> > > Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>> > > Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>> > > ---
>> > >  drivers/of/base.c |    4 +++-
>> > >  1 files changed, 3 insertions(+), 1 deletions(-)
>> > >
>> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
>> > > index 5d269a4..81b2601 100644
>> > > --- a/drivers/of/base.c
>> > > +++ b/drivers/of/base.c
>> > > @@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
>> > >   *
>> > >   *       Returns a node pointer with refcount incremented, use
>> > >   *       of_node_put() on it when done.
>> > > + *
>> > > + *       Does not return nodes marked unavailable by a status property.
>> > >   */
>> > >  struct device_node *of_get_next_child(const struct device_node *node,
>> > >   struct device_node *prev)
>> > > @@ -330,7 +332,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
>> > >   read_lock(&devtree_lock);
>> > >   next = prev ? prev->sibling : node->child;
>> > >   for (; next; next = next->sibling)
>> > > -         if (of_node_get(next))
>> > > +         if (of_device_is_available(next) && of_node_get(next))
>> > >                   break;
>> > >   of_node_put(prev);
>> > >   read_unlock(&devtree_lock);
>> >
>> > This seems like too low-level a place to put this.  Some code may know
>> > how to un-disable a device in certain situations, or it may be part of
>> > debug code trying to dump the whole device tree, etc.  Looking
>> > further[1], I see a raw version of this function, but not other things
>> > like of_find_compatible_node.
>>
>> Yeah I agree. I think we'll eventually end up with __ versions of all or
>> lots of them. Not to mention there might be cases you've missed where
>> code expects to see unavailable nodes. The right approach is to add
>> _new_ routines that don't return unavailable nodes, and convert code
>> that you know wants to use them.
>
> Actually, I don't think we really want these status-skipping
> iterators at all.  The device tree iterators should give us the device
> tree, as it is.  Those old-style drivers which seach for a node rather
> than using the bus probing logic can keep individual checks of the
> status property until they're converted to the new scheme.

So the patch should look something like this?

@@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct
device_node *node)
 *
 *     Returns a node pointer with refcount incremented, use
 *     of_node_put() on it when done.
+ *
+ *     Do not use this function.
 */
 struct device_node *of_get_next_child(const struct device_node *node,
       struct device_node *prev)

...

+ struct device_node *of_get_next_available_child(const struct
device_node *node,
+       struct device_node *prev)
+ ...
+ }

And then (almost) all the of_get_next_child() sites should be changed
to call the new function?

-Hollis

Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties

[Michael Ellerman]

[-- Attachment #1.1: Type: text/plain, Size: 3511 bytes --]

On Wed, 2010-12-15 at 10:35 -0800, Hollis Blanchard wrote:
> On Wed, Dec 8, 2010 at 7:09 PM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote:
> >> On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote:
> >> > On Wed, 8 Dec 2010 11:29:44 -0800
> >> > Deepak Saxena <deepak_saxena@mentor.com> wrote:
> >> >
> >> > > We only return the next child if the device is available.
> >> > >
> >> > > Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com>
> >> > > Signed-off-by: Deepak Saxena <deepak_saxena@mentor.com>
> >> > > ---
> >> > >  drivers/of/base.c |    4 +++-
> >> > >  1 files changed, 3 insertions(+), 1 deletions(-)
> >> > >
> >> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> >> > > index 5d269a4..81b2601 100644
> >> > > --- a/drivers/of/base.c
> >> > > +++ b/drivers/of/base.c
> >> > > @@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
> >> > >   *
> >> > >   *       Returns a node pointer with refcount incremented, use
> >> > >   *       of_node_put() on it when done.
> >> > > + *
> >> > > + *       Does not return nodes marked unavailable by a status property.
> >> > >   */
> >> > >  struct device_node *of_get_next_child(const struct device_node *node,
> >> > >   struct device_node *prev)
> >> > > @@ -330,7 +332,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
> >> > >   read_lock(&devtree_lock);
> >> > >   next = prev ? prev->sibling : node->child;
> >> > >   for (; next; next = next->sibling)
> >> > > -         if (of_node_get(next))
> >> > > +         if (of_device_is_available(next) && of_node_get(next))
> >> > >                   break;
> >> > >   of_node_put(prev);
> >> > >   read_unlock(&devtree_lock);
> >> >
> >> > This seems like too low-level a place to put this.  Some code may know
> >> > how to un-disable a device in certain situations, or it may be part of
> >> > debug code trying to dump the whole device tree, etc.  Looking
> >> > further[1], I see a raw version of this function, but not other things
> >> > like of_find_compatible_node.
> >>
> >> Yeah I agree. I think we'll eventually end up with __ versions of all or
> >> lots of them. Not to mention there might be cases you've missed where
> >> code expects to see unavailable nodes. The right approach is to add
> >> _new_ routines that don't return unavailable nodes, and convert code
> >> that you know wants to use them.
> >
> > Actually, I don't think we really want these status-skipping
> > iterators at all.  The device tree iterators should give us the device
> > tree, as it is.  Those old-style drivers which seach for a node rather
> > than using the bus probing logic can keep individual checks of the
> > status property until they're converted to the new scheme.
> 
> So the patch should look something like this?
> 
> @@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct
> device_node *node)
>  *
>  *     Returns a node pointer with refcount incremented, use
>  *     of_node_put() on it when done.
> + *
> + *     Do not use this function.
>  */
>  struct device_node *of_get_next_child(const struct device_node *node,
>        struct device_node *prev)

Haha. No it should say "this function doesn't lie to you".

And the patch should say "this patch _doesn't_ subtly change all callers
of of_get_next_child() without carefully auditing them".

cheers

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/5] of/device: Make of_get_next_child() check status properties

[Grant Likely]

On Thu, Dec 16, 2010 at 09:40:55AM +1100, Michael Ellerman wrote:
> On Wed, 2010-12-15 at 10:35 -0800, Hollis Blanchard wrote:
> > On Wed, Dec 8, 2010 at 7:09 PM, David Gibson
> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > On Thu, Dec 09, 2010 at 12:33:22PM +1100, Michael Ellerman wrote:
> > >> On Wed, 2010-12-08 at 15:01 -0600, Scott Wood wrote:
> > >> > On Wed, 8 Dec 2010 11:29:44 -0800
> > >> > Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
> > >> >
> > >> > > We only return the next child if the device is available.
> > >> > >
> > >> > > Signed-off-by: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> > >> > > Signed-off-by: Deepak Saxena <deepak_saxena-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> > >> > > ---
> > >> > >  drivers/of/base.c |    4 +++-
> > >> > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > >> > >
> > >> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > >> > > index 5d269a4..81b2601 100644
> > >> > > --- a/drivers/of/base.c
> > >> > > +++ b/drivers/of/base.c
> > >> > > @@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
> > >> > >   *
> > >> > >   *       Returns a node pointer with refcount incremented, use
> > >> > >   *       of_node_put() on it when done.
> > >> > > + *
> > >> > > + *       Does not return nodes marked unavailable by a status property.
> > >> > >   */
> > >> > >  struct device_node *of_get_next_child(const struct device_node *node,
> > >> > >   struct device_node *prev)
> > >> > > @@ -330,7 +332,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
> > >> > >   read_lock(&devtree_lock);
> > >> > >   next = prev ? prev->sibling : node->child;
> > >> > >   for (; next; next = next->sibling)
> > >> > > -         if (of_node_get(next))
> > >> > > +         if (of_device_is_available(next) && of_node_get(next))
> > >> > >                   break;
> > >> > >   of_node_put(prev);
> > >> > >   read_unlock(&devtree_lock);
> > >> >
> > >> > This seems like too low-level a place to put this.  Some code may know
> > >> > how to un-disable a device in certain situations, or it may be part of
> > >> > debug code trying to dump the whole device tree, etc.  Looking
> > >> > further[1], I see a raw version of this function, but not other things
> > >> > like of_find_compatible_node.
> > >>
> > >> Yeah I agree. I think we'll eventually end up with __ versions of all or
> > >> lots of them. Not to mention there might be cases you've missed where
> > >> code expects to see unavailable nodes. The right approach is to add
> > >> _new_ routines that don't return unavailable nodes, and convert code
> > >> that you know wants to use them.
> > >
> > > Actually, I don't think we really want these status-skipping
> > > iterators at all.  The device tree iterators should give us the device
> > > tree, as it is.  Those old-style drivers which seach for a node rather
> > > than using the bus probing logic can keep individual checks of the
> > > status property until they're converted to the new scheme.
> > 
> > So the patch should look something like this?
> > 
> > @@ -321,6 +321,8 @@ struct device_node *of_get_next_parent(struct
> > device_node *node)
> >  *
> >  *     Returns a node pointer with refcount incremented, use
> >  *     of_node_put() on it when done.
> > + *
> > + *     Do not use this function.
> >  */
> >  struct device_node *of_get_next_child(const struct device_node *node,
> >        struct device_node *prev)
> 
> Haha. No it should say "this function doesn't lie to you".
> 
> And the patch should say "this patch _doesn't_ subtly change all callers
> of of_get_next_child() without carefully auditing them".

Heh, Yes. The comments made on this patch are totally on-base.  Not
all nodes are devices, and not all callers will want to skip nodes;
regardless of the reason for skipping.  Case in point: the
/proc/device-tree support code.

If a caller needs a version of the function that skips unavailable
nodes, then that behaviour should be explicitly asked for.  In this
case it should be a new function with a new name.  Don't change the
behaviour out from under the existing users.

g.