Inconsistencies in OF_* functions return values

[Oleksandr Tymoshenko]

Hello,

I've been writing documentation for Open Firmware API
(OF_* functions) and found some inconsistencies.

As far as I understand OF_* functions are used to access device
tree in abstract way and mostly serve as wrappers to methods in
concrete implementations. There are two implementations at the
moment: standard Open Firmware and FDT. Nodes in device tree
represented by opaque integer type phandle_t. So whenever
function should return reference to the node it returns phandle_t
value. The problem is that error reporting is not consistent
across concrete implementations. Just as error checks in API
consumer code.

Standard Open Firmware implementations of tree navigations
functions OF_peer, OF_child, OF_parent return -1 in case of
internal error and just passes value returned by succefull call
to firmware.

FDT implementations return 0 to indicate both errors and absense
of requested node.

OF_finddevice in Standard OF acts like navigation functions:
-1 and pass through.

OF_finddevice in FDT returns -1 both on error and if path
can't be found.

API consumers of navigation functions are more or less
consistently check for 0. There are two code instances
that check for -1.

API consumers for OF_finddevice are all over the place.
Some check for 0, some for -1, some for both.

Also phandle_t is uint32_t so consumer code can't be just
converted to if (node > 0) ...

I didn't find any reserved values for phandle in FDT
specification [1]. The only requirement for it is to be unique
within device tree. So in theory 0 is also valid phandle_t.
In practice both GNU dtc and our own dtc start numbering
nodes from 1.

I think the right way to fix this is to consistently use 0
to indicate error or "no node" situation. I don't have
enough historical insight about OpenFirmware to claim
that this approach is compatible with older standards.
I'd appreciate input on this topic from people who actually
work with non-FDT implementation of OF_ API.

[1] https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2

[Nathan Whitehorn]

Post by Oleksandr Tymoshenko
Hello,
I've been writing documentation for Open Firmware API
(OF_* functions) and found some inconsistencies.

Thank you!

Post by Oleksandr Tymoshenko
As far as I understand OF_* functions are used to access device
tree in abstract way and mostly serve as wrappers to methods in
concrete implementations. There are two implementations at the
moment: standard Open Firmware and FDT. Nodes in device tree
represented by opaque integer type phandle_t. So whenever
function should return reference to the node it returns phandle_t
value. The problem is that error reporting is not consistent
across concrete implementations. Just as error checks in API
consumer code.

Some of the error checks are indeed a mess, but I think the
implementations are right. For reference, all of our OF_* functions are
supposed to match the IEEE 1275 CI specification (page 64 and on).
Insofar as that is different from FDT, we wrap the FDT conventions
(including the definition of phandle) to match the 1275 ones.

Post by Oleksandr Tymoshenko
Standard Open Firmware implementations of tree navigations
functions OF_peer, OF_child, OF_parent return -1 in case of
internal error and just passes value returned by succefull call
to firmware.
FDT implementations return 0 to indicate both errors and absense
of requested node.

OF_peer, child and, and parent are defined to return 0 on failure and a
non-zero number otherwise, so FDT is right. The standard does not allow
an "internal error" value. We should adjust ofw_standard and ofw_real to
return 0 if this occurs (which never happens in practice).

Post by Oleksandr Tymoshenko
-1 and pass through.
OF_finddevice in FDT returns -1 both on error and if path
can't be found.

These are the same correct behavior: OF_finddevice() is defined to
return -1 on failure of any kind. (On real OF, the firmware returns this
if the path cannot be found.)

Post by Oleksandr Tymoshenko
API consumers of navigation functions are more or less
consistently check for 0. There are two code instances
that check for -1.
API consumers for OF_finddevice are all over the place.
Some check for 0, some for -1, some for both.

I assume only very few check for zero? Those can't work at present.

Post by Oleksandr Tymoshenko
Also phandle_t is uint32_t so consumer code can't be just
converted to if (node > 0) ...

This can't be avoided, sadly. You have to compare to (phandle_t)-1 to be
compliant with the standard.

Post by Oleksandr Tymoshenko
I didn't find any reserved values for phandle in FDT
specification [1]. The only requirement for it is to be unique
within device tree. So in theory 0 is also valid phandle_t.
In practice both GNU dtc and our own dtc start numbering
nodes from 1.

FDT "phandles" are our "xref phandles" and have no constraints on
numeric value. Our phandles, which cannot be zero or -1, are the FDT
offsets shifted to make 0 an invalid value.

Post by Oleksandr Tymoshenko
I think the right way to fix this is to consistently use 0
to indicate error or "no node" situation. I don't have
enough historical insight about OpenFirmware to claim
that this approach is compatible with older standards.
I'd appreciate input on this topic from people who actually
work with non-FDT implementation of OF_ API.
[1] https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2

I think this is not the right approach and will break a lot of code. We
should keep using the 1275 CI values, which we almost entirely already are.
-Nathan

[Oleksandr Tymoshenko]

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
Hello,
I've been writing documentation for Open Firmware API
(OF_* functions) and found some inconsistencies.

Thank you!

Post by Oleksandr Tymoshenko
As far as I understand OF_* functions are used to access device
tree in abstract way and mostly serve as wrappers to methods in
concrete implementations. There are two implementations at the
moment: standard Open Firmware and FDT. Nodes in device tree
represented by opaque integer type phandle_t. So whenever
function should return reference to the node it returns phandle_t
value. The problem is that error reporting is not consistent
across concrete implementations. Just as error checks in API
consumer code.

Some of the error checks are indeed a mess, but I think the
implementations are right. For reference, all of our OF_* functions are
supposed to match the IEEE 1275 CI specification (page 64 and on).
Insofar as that is different from FDT, we wrap the FDT conventions
(including the definition of phandle) to match the 1275 ones.

Post by Oleksandr Tymoshenko
Standard Open Firmware implementations of tree navigations
functions OF_peer, OF_child, OF_parent return -1 in case of
internal error and just passes value returned by succefull call
to firmware.
FDT implementations return 0 to indicate both errors and absense
of requested node.

OF_peer, child and, and parent are defined to return 0 on failure and a
non-zero number otherwise, so FDT is right. The standard does not allow
an "internal error" value. We should adjust ofw_standard and ofw_real to
return 0 if this occurs (which never happens in practice).

Post by Oleksandr Tymoshenko
-1 and pass through.
OF_finddevice in FDT returns -1 both on error and if path
can't be found.

These are the same correct behavior: OF_finddevice() is defined to
return -1 on failure of any kind. (On real OF, the firmware returns this
if the path cannot be found.)

Post by Oleksandr Tymoshenko
API consumers of navigation functions are more or less
consistently check for 0. There are two code instances
that check for -1.
API consumers for OF_finddevice are all over the place.
Some check for 0, some for -1, some for both.

I assume only very few check for zero? Those can't work at present.

There are 14 instances of checks for 0 in sys/arm I could find with
grep. I believe they're mostly safeguards against invalid dtb
files so failure path never taken in practice.

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
Also phandle_t is uint32_t so consumer code can't be just
converted to if (node > 0) ...

This can't be avoided, sadly. You have to compare to (phandle_t)-1 to be
compliant with the standard.

In this case we need some kind of define, like INVALID_PHANDLE.
Or OF_valid() function. Because phandle_t is opaque type and
seeing -1 as a possible return value it's natural to assume
that it's signed and implement something like "if (node > 0)".

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
I didn't find any reserved values for phandle in FDT
specification [1]. The only requirement for it is to be unique
within device tree. So in theory 0 is also valid phandle_t.
In practice both GNU dtc and our own dtc start numbering
nodes from 1.

FDT "phandles" are our "xref phandles" and have no constraints on
numeric value. Our phandles, which cannot be zero or -1, are the FDT
offsets shifted to make 0 an invalid value.

Ah. It makes more sense now. Thanks. This should be documented too :)

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
I think the right way to fix this is to consistently use 0
to indicate error or "no node" situation. I don't have
enough historical insight about OpenFirmware to claim
that this approach is compatible with older standards.
I'd appreciate input on this topic from people who actually
work with non-FDT implementation of OF_ API.
[1] https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2

I think this is not the right approach and will break a lot of code. We
should keep using the 1275 CI values, which we almost entirely already are.

I agree. I didn't have information about IEEE 1275 standard.

[Nathan Whitehorn]

Post by Oleksandr Tymoshenko

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
Hello,
I've been writing documentation for Open Firmware API
(OF_* functions) and found some inconsistencies.

Thank you!

Post by Oleksandr Tymoshenko
As far as I understand OF_* functions are used to access device
tree in abstract way and mostly serve as wrappers to methods in
concrete implementations. There are two implementations at the
moment: standard Open Firmware and FDT. Nodes in device tree
represented by opaque integer type phandle_t. So whenever
function should return reference to the node it returns phandle_t
value. The problem is that error reporting is not consistent
across concrete implementations. Just as error checks in API
consumer code.

Some of the error checks are indeed a mess, but I think the
implementations are right. For reference, all of our OF_* functions are
supposed to match the IEEE 1275 CI specification (page 64 and on).
Insofar as that is different from FDT, we wrap the FDT conventions
(including the definition of phandle) to match the 1275 ones.

Post by Oleksandr Tymoshenko
Standard Open Firmware implementations of tree navigations
functions OF_peer, OF_child, OF_parent return -1 in case of
internal error and just passes value returned by succefull call
to firmware.
FDT implementations return 0 to indicate both errors and absense
of requested node.

OF_peer, child and, and parent are defined to return 0 on failure and a
non-zero number otherwise, so FDT is right. The standard does not allow
an "internal error" value. We should adjust ofw_standard and ofw_real to
return 0 if this occurs (which never happens in practice).

I've fixed these.

Post by Oleksandr Tymoshenko

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
-1 and pass through.
OF_finddevice in FDT returns -1 both on error and if path
can't be found.

These are the same correct behavior: OF_finddevice() is defined to
return -1 on failure of any kind. (On real OF, the firmware returns this
if the path cannot be found.)

Post by Oleksandr Tymoshenko
API consumers of navigation functions are more or less
consistently check for 0. There are two code instances
that check for -1.
API consumers for OF_finddevice are all over the place.
Some check for 0, some for -1, some for both.

I assume only very few check for zero? Those can't work at present.

There are 14 instances of checks for 0 in sys/arm I could find with
grep. I believe they're mostly safeguards against invalid dtb
files so failure path never taken in practice.

Fascinating. Well, those should be easy to fix and is a sure sign of how
important the documentation you are writing is.

Post by Oleksandr Tymoshenko

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
Also phandle_t is uint32_t so consumer code can't be just
converted to if (node > 0) ...

This can't be avoided, sadly. You have to compare to (phandle_t)-1 to be
compliant with the standard.

In this case we need some kind of define, like INVALID_PHANDLE.
Or OF_valid() function. Because phandle_t is opaque type and
seeing -1 as a possible return value it's natural to assume
that it's signed and implement something like "if (node > 0)".

Yes, could. It is irritating that the standard defines a mixture of -1
and 0 returns, so I have a small worry that this would encourage people
to check the results of OF_peer(), OF_parent(), and OF_child() against
INVALID_PHANDLE. I am not sure what the lesser evil is and will defer to
your judgment.

Post by Oleksandr Tymoshenko

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
I didn't find any reserved values for phandle in FDT
specification [1]. The only requirement for it is to be unique
within device tree. So in theory 0 is also valid phandle_t.
In practice both GNU dtc and our own dtc start numbering
nodes from 1.

FDT "phandles" are our "xref phandles" and have no constraints on
numeric value. Our phandles, which cannot be zero or -1, are the FDT
offsets shifted to make 0 an invalid value.

Ah. It makes more sense now. Thanks. This should be documented too :)

Yes, no doubt! It is unfortunate, if understandable in context, that the
FDT people reused this term in a partially-overlapping way.

Post by Oleksandr Tymoshenko

Post by Nathan Whitehorn

Post by Oleksandr Tymoshenko
I think the right way to fix this is to consistently use 0
to indicate error or "no node" situation. I don't have
enough historical insight about OpenFirmware to claim
that this approach is compatible with older standards.
I'd appreciate input on this topic from people who actually
work with non-FDT implementation of OF_ API.
[1] https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2

I think this is not the right approach and will break a lot of code. We
should keep using the 1275 CI values, which we almost entirely already are.

I agree. I didn't have information about IEEE 1275 standard.

Let me know if you need any help on this.
-Nathan