From owner-freebsd-arch@freebsd.org Mon Feb 19 15:55:28 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C9429F0ACAF for ; Mon, 19 Feb 2018 15:55:27 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from c.mail.sonic.net (c.mail.sonic.net [64.142.111.80]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 5F7427BE4C for ; Mon, 19 Feb 2018 15:55:26 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from comporellon.tachypleus.net (cpe-75-82-218-62.socal.res.rr.com [75.82.218.62]) (authenticated bits=0) by c.mail.sonic.net (8.15.1/8.15.1) with ESMTPSA id w1JFtHHA018030 (version=TLSv1.2 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Mon, 19 Feb 2018 07:55:17 -0800 Subject: Re: Inconsistencies in OF_* functions return values To: Oleksandr Tymoshenko Cc: freebsd-arch@freebsd.org References: <20180218062504.GA3226@bluezbox.com> <7c4cabad-421c-63db-bf31-289cf4b57fd1@freebsd.org> <20180218201350.GA15860@bluezbox.com> From: Nathan Whitehorn Message-ID: <8a18fd5f-8734-8323-7638-039b5b2b5876@freebsd.org> Date: Mon, 19 Feb 2018 07:55:17 -0800 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180218201350.GA15860@bluezbox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Sonic-CAuth: UmFuZG9tSVZnS87YbJ2A3GBRvDccZcVtaT/pGqT+eRUklBVBHXcVB6yu0lpYvXHLUlz2oILh7OieSTMX3bofB4vl9exMLYUVhUkb+vSSRWA= X-Sonic-ID: C;StPHR40V6BGPD1DNXaHR5A== M;UJL4R40V6BGPD1DNXaHR5A== X-Spam-Flag: No X-Sonic-Spam-Details: 0.0/5.0 by cerberusd X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Feb 2018 15:55:28 -0000 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