Date: Sun, 18 Feb 2018 07:14:29 -0800 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: freebsd-arch@freebsd.org Subject: Re: Inconsistencies in OF_* functions return values Message-ID: <7c4cabad-421c-63db-bf31-289cf4b57fd1@freebsd.org> In-Reply-To: <20180218062504.GA3226@bluezbox.com> References: <20180218062504.GA3226@bluezbox.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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. > 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. > 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. > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7c4cabad-421c-63db-bf31-289cf4b57fd1>