Inconsistencies in OF_* functions return values

[Oleksandr Tymoshenko]

Nathan Whitehorn (nwhitehorn at freebsd.org) wrote:
> 
> 
> On 02/17/18 22:25, Oleksandr Tymoshenko wrote:
> > Hello,
> >
> > I've been writing documentation for Open Firmware API
> > (OF_* functions) and found some inconsistencies.
> 
> Thank you!
> 
> > 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.
> 
> > 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).
> 
> > 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.
> 
> 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.)
> 
> >
> > 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. 
 
> > 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)".

> > 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 :)

> > 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. 

-- 
gonzo