From owner-freebsd-arch@freebsd.org Sun Feb 18 20:14:00 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 7B343F26C16 for ; Sun, 18 Feb 2018 20:14:00 +0000 (UTC) (envelope-from gonzo@bluezbox.com) Received: from id.bluezbox.com (id.bluezbox.com [45.55.20.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 1B69868F96; Sun, 18 Feb 2018 20:13:59 +0000 (UTC) (envelope-from gonzo@bluezbox.com) Received: from localhost ([127.0.0.1] helo=id.bluezbox.com) by id.bluezbox.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89 (FreeBSD)) (envelope-from ) id 1enVLL-0004D8-J9; Sun, 18 Feb 2018 12:13:52 -0800 Received: (from gonzo@localhost) by id.bluezbox.com (8.15.2/8.15.2/Submit) id w1IKDoee016189; Sun, 18 Feb 2018 12:13:50 -0800 (PST) (envelope-from gonzo@bluezbox.com) X-Authentication-Warning: id.bluezbox.com: gonzo set sender to gonzo@bluezbox.com using -f Date: Sun, 18 Feb 2018 12:13:50 -0800 From: Oleksandr Tymoshenko To: Nathan Whitehorn Cc: freebsd-arch@freebsd.org Subject: Re: Inconsistencies in OF_* functions return values Message-ID: <20180218201350.GA15860@bluezbox.com> References: <20180218062504.GA3226@bluezbox.com> <7c4cabad-421c-63db-bf31-289cf4b57fd1@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7c4cabad-421c-63db-bf31-289cf4b57fd1@freebsd.org> X-Operating-System: FreeBSD/11.1-RELEASE-p4 (amd64) User-Agent: Mutt/1.9.1 (2017-09-22) X-Spam-Level: -- X-Spam-Report: Spam detection software, running on the system "id.bluezbox.com", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see The administrator of that system for details. Content preview: 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 [...] Content analysis details: (-2.9 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] 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: Sun, 18 Feb 2018 20:14:00 -0000 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