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