Date: Mon, 19 Feb 2018 07:55:17 -0800 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Oleksandr Tymoshenko <gonzo@bluezbox.com> Cc: freebsd-arch@freebsd.org Subject: Re: Inconsistencies in OF_* functions return values Message-ID: <8a18fd5f-8734-8323-7638-039b5b2b5876@freebsd.org> In-Reply-To: <20180218201350.GA15860@bluezbox.com> References: <20180218062504.GA3226@bluezbox.com> <7c4cabad-421c-63db-bf31-289cf4b57fd1@freebsd.org> <20180218201350.GA15860@bluezbox.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 02/18/18 12:13, Oleksandr Tymoshenko wrote: > 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). I've fixed these. >>> 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. Fascinating. Well, those should be easy to fix and is a sure sign of how important the documentation you are writing is. > >>> 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. > >>> 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. > >>> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8a18fd5f-8734-8323-7638-039b5b2b5876>