[PATCH 1/2][OF] Add of_device_is_disabled function

IEEE 1275 defined a standard "status" property to indicate the operational
status of a device.  The property has four possible values: okay, disabled,
fail, fail-xxx.  The absence of this property means the operational status
of the device is unknown or okay.

This adds a function called of_device_is_disabled that checks to see if a
node has the status property set to "disabled".  This can be quite useful
for devices that may be present but disabled due to pin sharing, etc.

Signed-off-by: Josh Boyer <redacted>

---
 drivers/of/base.c  |   18 ++++++++++++++++++
 include/linux/of.h |    1 +
 2 files changed, 19 insertions(+)

--- linux-2.6.orig/drivers/of/base.c
+++ linux-2.6/drivers/of/base.c

@@ -117,6 +117,24 @@ int of_device_is_compatible(const struct
 EXPORT_SYMBOL(of_device_is_compatible);
 
 /**
+ *  of_device_is_disabled - Check if a device's status is disabled
+ *  @device: device node to check
+ *
+ *  Returns true or false depending on the value of the staus property
+ */
+int of_device_is_disabled(const struct device_node *device)
+{
+	const char *status;
+
+	status = of_get_property(device, "status", NULL);
+	if (status == NULL)
+		return 0;
+
+	return !(strcmp(status, "disabled"));
+}
+EXPORT_SYMBOL(of_device_is_disabled);
+
+/**
  *	of_get_parent - Get a node's parent if any
  *	@node:	Node to get parent
  *
--- linux-2.6.orig/include/linux/of.h
+++ linux-2.6/include/linux/of.h

@@ -62,6 +62,7 @@ extern struct property *of_find_property
 					 int *lenp);
 extern int of_device_is_compatible(const struct device_node *device,
 				   const char *);
+extern int of_device_is_disabled(const struct device_node *device);
 extern const void *of_get_property(const struct device_node *node,
 				const char *name,
 				int *lenp);

Some SoC chips have multiple serial ports on board.  The usability of these
ports can rely on various factors, ranging from pin sharing to unpopulated
connectors.  This uses the new of_device_is_disabled function to check for
and ignore disabled UARTs.

Signed-off-by: Josh Boyer <redacted>

---
 arch/powerpc/kernel/legacy_serial.c |    4 ++++
 drivers/serial/of_serial.c          |    5 +++++
 2 files changed, 9 insertions(+)

--- linux-2.6.orig/drivers/serial/of_serial.c
+++ linux-2.6/drivers/serial/of_serial.c

@@ -72,6 +72,11 @@ static int __devinit of_platform_serial_
 	int port_type;
 	int ret;
 
+	if (of_device_is_disabled(ofdev->node)) {
+		dev_info(&ofdev->dev, "Disabled serial port.  Ignored\n");
+		return -ENODEV;
+	}
+
 	if (of_find_property(ofdev->node, "used-by-rtas", NULL))
 		return -EBUSY;
 
--- linux-2.6.orig/arch/powerpc/kernel/legacy_serial.c
+++ linux-2.6/arch/powerpc/kernel/legacy_serial.c

@@ -54,6 +54,10 @@ static int __init add_legacy_port(struct
 	u32 clock = BASE_BAUD * 16;
 	int index;
 
+	/* Check the status property if present.  Ignore disabled devices */
+	if (of_device_is_disabled(np))
+		return -1;
+
 	/* get clock freq. if present */
 	clk = of_get_property(np, "clock-frequency", NULL);
 	if (clk && *clk)

On Sat, 23 Feb 2008 15:58:23 -0600
Josh Boyer [off-list ref] wrote:

IEEE 1275 defined a standard "status" property to indicate the operational
status of a device.  The property has four possible values: okay, disabled,
fail, fail-xxx.  The absence of this property means the operational status
of the device is unknown or okay.

This adds a function called of_device_is_disabled that checks to see if a
node has the status property set to "disabled".  This can be quite useful
for devices that may be present but disabled due to pin sharing, etc.

Signed-off-by: Josh Boyer <redacted>

Talking with Ben H a bit, he suggested to reverse this API.  Basically,
create an of_device_is_available that returns 1 if the status property
is completely missing, or if it's set to "okay" or "ok".  The latter is
to cope with some broken firmwares.

I can do either really.  Eventually you could embed the is_available
check in the of_platform code so that devices don't even get presented
to drivers if they aren't available.

Dave, I'm not sure how applicable this all is to sparc.  But for some
of the "newer" embedded ports that are coming into powerpc I can see it
being very useful.

Thoughts?

josh

On Sat, 23 Feb 2008 18:59:04 -0600
Josh Boyer [off-list ref] wrote:

On Sat, 23 Feb 2008 15:58:23 -0600
Josh Boyer [off-list ref] wrote:

quoted

IEEE 1275 defined a standard "status" property to indicate the operational
status of a device.  The property has four possible values: okay, disabled,
fail, fail-xxx.  The absence of this property means the operational status
of the device is unknown or okay.

This adds a function called of_device_is_disabled that checks to see if a
node has the status property set to "disabled".  This can be quite useful
for devices that may be present but disabled due to pin sharing, etc.

Signed-off-by: Josh Boyer <redacted>

Talking with Ben H a bit, he suggested to reverse this API.  Basically,
create an of_device_is_available that returns 1 if the status property
is completely missing, or if it's set to "okay" or "ok".  The latter is
to cope with some broken firmwares.

And since I seem to be talking to myself and have nothing better to do
on a Saturday evening, here's the code.

josh


IEEE 1275 defined a standard "status" property to indicate the operational
status of a device.  The property has four possible values: okay, disabled,
fail, fail-xxx.  The absence of this property means the operational status
of the device is unknown or okay.

This adds a function called of_device_is_available that checks the state
of the status property of a device.  If the property is absent or set to
either "okay" or "ok", it returns 1.  Otherwise it returns 0.

Signed-off-by: Josh Boyer <redacted>

---
 drivers/of/base.c  |   23 +++++++++++++++++++++++
 include/linux/of.h |    1 +
 2 files changed, 24 insertions(+)

--- linux-2.6.orig/drivers/of/base.c
+++ linux-2.6/drivers/of/base.c

@@ -117,6 +117,29 @@ int of_device_is_compatible(const struct
 EXPORT_SYMBOL(of_device_is_compatible);
 
 /**
+ *  of_device_is_available - check if a device is available for use
+ *
+ *  @device: Node to check for availability
+ *
+ *  Returns 1 if the status property is absent or set to "okay" or "ok",
+ *  0 otherwise
+ */
+int of_device_is_available(const struct device_node *device)
+{
+	const char *status;
+
+	status = of_get_property(device, "status", NULL);
+	if (status == NULL)
+		return 1;
+
+	if (!strcmp(status, "okay") || !strcmp(status, "ok"))
+		return 1;
+
+	return 0;
+}
+EXPORT_SYMBOL(of_device_is_available);
+
+/**
  *	of_get_parent - Get a node's parent if any
  *	@node:	Node to get parent
  *
--- linux-2.6.orig/include/linux/of.h
+++ linux-2.6/include/linux/of.h

@@ -62,6 +62,7 @@ extern struct property *of_find_property
 					 int *lenp);
 extern int of_device_is_compatible(const struct device_node *device,
 				   const char *);
+extern int of_device_is_available(const struct device_node *device);
 extern const void *of_get_property(const struct device_node *node,
 				const char *name,
 				int *lenp);

From: Josh Boyer <redacted>
Date: Sat, 23 Feb 2008 18:59:04 -0600

Dave, I'm not sure how applicable this all is to sparc.  But for some
of the "newer" embedded ports that are coming into powerpc I can see it
being very useful.

I think I've seen this property on sparc boxes too, I'm fine
with whatever you guys come up with.

Josh Boyer wrote:

On Sat, 23 Feb 2008 15:58:23 -0600
Josh Boyer [off-list ref] wrote:

quoted

IEEE 1275 defined a standard "status" property to indicate the operational
status of a device.  The property has four possible values: okay, disabled,
fail, fail-xxx.  The absence of this property means the operational status
of the device is unknown or okay.

This adds a function called of_device_is_disabled that checks to see if a
node has the status property set to "disabled".  This can be quite useful
for devices that may be present but disabled due to pin sharing, etc.

Talking with Ben H a bit, he suggested to reverse this API.  Basically,
create an of_device_is_available that returns 1 if the status property
is completely missing, or if it's set to "okay" or "ok".  The latter is
to cope with some broken firmwares.

I agree with Ben's suggestion.  The rtas_pci and eeh code could be
converted to use this, which gives a net savings of a few bytes with
ppc64_defconfig:

diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
index 433a0a0..39a752b 100644
--- a/arch/powerpc/kernel/rtas_pci.c
+++ b/arch/powerpc/kernel/rtas_pci.c

@@ -56,21 +56,6 @@ static inline int config_access_valid(struct pci_dn *dn, int where)
 	return 0;
 }
 
-static int of_device_available(struct device_node * dn)
-{
-        const char *status;
-
-        status = of_get_property(dn, "status", NULL);
-
-        if (!status)
-                return 1;
-
-        if (!strcmp(status, "okay"))
-                return 1;
-
-        return 0;
-}
-
 int rtas_read_config(struct pci_dn *pdn, int where, int size, u32 *val)
 {
 	int returnval = -1;

@@ -117,7 +102,7 @@ static int rtas_pci_read_config(struct pci_bus *bus,
 	for (dn = busdn->child; dn; dn = dn->sibling) {
 		struct pci_dn *pdn = PCI_DN(dn);
 		if (pdn && pdn->devfn == devfn
-		    && of_device_available(dn))
+		    && of_device_is_available(dn))
 			return rtas_read_config(pdn, where, size, val);
 	}
 

@@ -164,7 +149,7 @@ static int rtas_pci_write_config(struct pci_bus *bus,
 	for (dn = busdn->child; dn; dn = dn->sibling) {
 		struct pci_dn *pdn = PCI_DN(dn);
 		if (pdn && pdn->devfn == devfn
-		    && of_device_available(dn))
+		    && of_device_is_available(dn))
 			return rtas_write_config(pdn, where, size, val);
 	}
 	return PCIBIOS_DEVICE_NOT_FOUND;

diff --git a/arch/powerpc/platforms/pseries/eeh.c b/arch/powerpc/platforms/pseries/eeh.c
index 9eb539e..550b2f7 100644
--- a/arch/powerpc/platforms/pseries/eeh.c
+++ b/arch/powerpc/platforms/pseries/eeh.c

@@ -945,7 +945,6 @@ static void *early_enable_eeh(struct device_node *dn, void *data)
 	unsigned int rets[3];
 	struct eeh_early_enable_info *info = data;
 	int ret;
-	const char *status = of_get_property(dn, "status", NULL);
 	const u32 *class_code = of_get_property(dn, "class-code", NULL);
 	const u32 *vendor_id = of_get_property(dn, "vendor-id", NULL);
 	const u32 *device_id = of_get_property(dn, "device-id", NULL);

@@ -959,8 +958,8 @@ static void *early_enable_eeh(struct device_node *dn, void *data)
 	pdn->eeh_freeze_count = 0;
 	pdn->eeh_false_positives = 0;
 
-	if (status && strncmp(status, "ok", 2) != 0)
-		return NULL;	/* ignore devices with bad status */
+	if (!of_device_is_available(dn))
+		return NULL;
 
 	/* Ignore bad nodes. */
 	if (!class_code || !vendor_id || !device_id)

Josh Boyer writes:

+	status = of_get_property(device, "status", NULL);
+	if (status == NULL)
+		return 1;
+
+	if (!strcmp(status, "okay") || !strcmp(status, "ok"))

It would probably be good to defend against the possibility that the
property isn't null-terminated (for example if its length is zero).

Paul.

From: Paul Mackerras <redacted>
Date: Tue, 26 Feb 2008 20:04:12 +1100

It would probably be good to defend against the possibility that the
property isn't null-terminated (for example if its length is zero).

FWIW, when I pull in the device tree on sparc I eliminate any need for
those kinds of checks by putting a '\0' at the end of every property
blob.

I copied a lot of this code from you guys, so it wouldn't surprise
me if ppc does this too.  :-)

On Tue, 2008-02-26 at 13:34 -0800, David Miller wrote:

From: Paul Mackerras <redacted>
Date: Tue, 26 Feb 2008 20:04:12 +1100

quoted

It would probably be good to defend against the possibility that the
property isn't null-terminated (for example if its length is zero).

FWIW, when I pull in the device tree on sparc I eliminate any need for
those kinds of checks by putting a '\0' at the end of every property
blob.

I copied a lot of this code from you guys, so it wouldn't surprise
me if ppc does this too.  :-)

I doubt we do that. Properties that contain things like ranges, or "reg"
properties are expected to be of a size that is a multiple of
#size-cells/#address-cells and I'm not sure that won't break things here
or there if they suddenly get one more byte..

Or do you mean you/we are appending that-without- changing the length
field ?

Ben.

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed, 27 Feb 2008 08:45:37 +1100

I doubt we do that. Properties that contain things like ranges, or "reg"
properties are expected to be of a size that is a multiple of
#size-cells/#address-cells and I'm not sure that won't break things here
or there if they suddenly get one more byte..

Or do you mean you/we are appending that-without- changing the length
field ?

Right, simply don't change the length field.  Put the zero byte
at offset "length + 1"

It's stupid to validate NULL termination everywhere when we
can make it an invariant in one spot.

On Tue, 2008-02-26 at 14:44 -0800, David Miller wrote:

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed, 27 Feb 2008 08:45:37 +1100

quoted

I doubt we do that. Properties that contain things like ranges, or "reg"
properties are expected to be of a size that is a multiple of
#size-cells/#address-cells and I'm not sure that won't break things here
or there if they suddenly get one more byte..

Or do you mean you/we are appending that-without- changing the length
field ?

Right, simply don't change the length field.  Put the zero byte
at offset "length + 1"

It's stupid to validate NULL termination everywhere when we
can make it an invariant in one spot.

Fair enough.

Ben.

On Tue, 26 Feb 2008 14:44:23 -0800 (PST)
David Miller [off-list ref] wrote:

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed, 27 Feb 2008 08:45:37 +1100

quoted

I doubt we do that. Properties that contain things like ranges, or "reg"
properties are expected to be of a size that is a multiple of
#size-cells/#address-cells and I'm not sure that won't break things here
or there if they suddenly get one more byte..

Or do you mean you/we are appending that-without- changing the length
field ?

Right, simply don't change the length field.  Put the zero byte
at offset "length + 1"

It's stupid to validate NULL termination everywhere when we
can make it an invariant in one spot.

I don't mind fixing up the function to use strncmp and checking for a 0
length from of_get_property.  However, I'm almost certain that other
places in the code have the same issue so what you're saying here seems
to make sense.

josh