From owner-freebsd-arch@FreeBSD.ORG Mon Nov 25 19:48:41 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 99211D5B for ; Mon, 25 Nov 2013 19:48:41 +0000 (UTC) Received: from mail-qe0-x229.google.com (mail-qe0-x229.google.com [IPv6:2607:f8b0:400d:c02::229]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 5CDB92484 for ; Mon, 25 Nov 2013 19:48:41 +0000 (UTC) Received: by mail-qe0-f41.google.com with SMTP id gh4so1944372qeb.28 for ; Mon, 25 Nov 2013 11:48:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eitanadler.com; s=0xdeadbeef; h=mime-version:from:date:message-id:subject:to:content-type; bh=KC0AqfAkT1t9trxLuT/bwoYOTbiQLwCDeaxkcpOzB68=; b=BIkyuRoFV87PHimXQQhZt+nVTCHWH+j/wNJU4mJt/908kdLEfEzA2Mg8UxOGRCTm1K U6wXG9sZBE8ukJ2tg2bOOoQjs44Qnq6nE+ZGxrrY85u6h3uPnjfTdILYKUMNFWQxO47F 21XN90U737j5B9DqOZTGYbOj/K5tmLNxJe6eE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:from:date:message-id:subject:to :content-type; bh=KC0AqfAkT1t9trxLuT/bwoYOTbiQLwCDeaxkcpOzB68=; b=cybdIa5ZIkGPVgNtKzWzIK92vbaIcfqX3VBPg18cdF1e/ivrBnMFhsc8gUPavwmFN5 0IASsGytIzDD+2GbWwxnMM+6NVzscHezlu1mfUkNZyxTdPVkClonCo8qjkc67uL/7pHx p0rZ7hXhY8w49FJRQBnS5Gk+UNA/uOeppa/DvRboNnkhRAbUiNg61Iud0hyn73jZf/S3 6NzICu/YC79zzxNhOK6iJ3kDUgXV52nLKsnJEQQGgthDUHKMbt+aMI7UIYV8WOGgiZ0X Ue9bQOerj7vXc5LW/3N2VJBH+SRn2J+y5BJLAdfEEiX+dQjwPquxp3Ls8eEWBVcb9GI5 S4Ng== X-Gm-Message-State: ALoCoQkMfIAiRoK7yj7icYs/1vbFz93SZnUuMF/LxLWPAm4dkMOy0hnxpgJuWK24KrBWslr8Ty+t X-Received: by 10.224.8.65 with SMTP id g1mr49757411qag.68.1385408920412; Mon, 25 Nov 2013 11:48:40 -0800 (PST) MIME-Version: 1.0 Received: by 10.96.63.101 with HTTP; Mon, 25 Nov 2013 11:48:10 -0800 (PST) From: Eitan Adler Date: Mon, 25 Nov 2013 14:48:10 -0500 Message-ID: Subject: 1 << 31 and related issues To: "freebsd-arch@freebsd.org" Content-Type: text/plain; charset=UTF-8 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Nov 2013 19:48:41 -0000 There are a few cases in FreeBSD where the expression (1 << 31) is used. However this produces undefined behavior as '1' is of type int. (see 6.4.4p5: The type of an unmarked integer constant is the first of the following list in which its value can be represented: int, long int, long long int). The shift of 31 is illegal (see 6.5.7p4) if the size of an int is 32. The same issue arises with 2 << 30 and 3 << 30. I have been working on fixing this issue in a few different projects. The correct fix here is to use 1U as the literal instead. adrian@ has advocated for the macros BIT32(bit) and BIT64(bit) which I would also support. I have patches which fix the issue by using 1U in these cases. I did not change non-broken cases. I also did not change contributed code. An incomplete listing of the issues available here: http://people.freebsd.org/~eadler/files/1..31.txt -- Eitan Adler From owner-freebsd-arch@FreeBSD.ORG Mon Nov 25 20:29:42 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 7D18C5B0 for ; Mon, 25 Nov 2013 20:29:42 +0000 (UTC) Received: from mail-pb0-x22a.google.com (mail-pb0-x22a.google.com [IPv6:2607:f8b0:400e:c01::22a]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 52001272E for ; Mon, 25 Nov 2013 20:29:42 +0000 (UTC) Received: by mail-pb0-f42.google.com with SMTP id uo5so6515798pbc.15 for ; Mon, 25 Nov 2013 12:29:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wemm.org; s=google; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type:content-transfer-encoding; bh=5zoxdTmvcUvY5yfy6Aach/VTKs74WsceMXrnPO+IiuM=; b=TlxJML9sHilppMEaRDabG5+bFlSV1KcrVT6+FTPcrSDlzh7HkJ0Osj8ACiDnX4sO2u cLC5k4q/YHnBiDjOZoa0ZpRzz+/9XMqXWSq9hQJ9QWUmSlAQ9/gca3orzYFb1/nuEDfJ gW9yc4zm54uRYHblZL8bHC8zpy/PAWyqs9hMc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=5zoxdTmvcUvY5yfy6Aach/VTKs74WsceMXrnPO+IiuM=; b=VSChGZmJ6EBgWqs3vEEFa0b4zwj2h/TomhgZljwxQHjyKw0MyCTWiMrr2Y9gGKEC42 4R0qU+2BnegXECjY0egPUBvl2xgVxj/TQA6C3VAws7EjcYj3xPopajkoSZkmBQ0JLwHx wzP3OyT+q/XvH1b7FknoPvoLavvh7cPp27VzJkNxfUOzjjM4URfZDrnZm7Age4VWurTU jm9/UEwUExom6uUB9ATXRBmclRasvhEd0+lOsc314FGhWwxnO2ZnMokmghACiGRntgpD spwXTDKAepGqxSwslN/43qz044Kqgr/rwKliewu02JLBR5Z4hFKBZdoiZGPsiPh/Df8Y M8jA== X-Gm-Message-State: ALoCoQkWN80ZBXt/swmqn+y1RtO27un4cR3oLOPm/ekWNFES2BwAzor7oniYZ97gm4VoxYeNqe4f X-Received: by 10.66.13.70 with SMTP id f6mr3148796pac.180.1385411381800; Mon, 25 Nov 2013 12:29:41 -0800 (PST) Received: from hater-dm.corp.yahoo.com (nat-dip4.cfw-a-gci.corp.yahoo.com. [209.131.62.113]) by mx.google.com with ESMTPSA id xv2sm75293567pbb.39.2013.11.25.12.29.40 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 25 Nov 2013 12:29:40 -0800 (PST) Message-ID: <5293B333.9070804@wemm.org> Date: Mon, 25 Nov 2013 12:29:39 -0800 From: Peter Wemm User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.1.1 MIME-Version: 1.0 To: freebsd-arch@freebsd.org Subject: Re: 1 << 31 and related issues References: In-Reply-To: X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Nov 2013 20:29:42 -0000 On 11/25/13, 11:48 AM, Eitan Adler wrote: > There are a few cases in FreeBSD where the expression (1 << 31) is used. > > However this produces undefined behavior as '1' is of type int. (see > 6.4.4p5: The type of an unmarked integer constant is the first of the > following list in which its value can be represented: int, long int, > long long int). The shift of 31 is illegal (see 6.5.7p4) if the size > of an int is 32. The same issue arises with 2 << 30 and 3 << 30. > > I have been working on fixing this issue in a few different projects. > The correct fix here is to use 1U as the literal instead. adrian@ has > advocated for the macros BIT32(bit) and BIT64(bit) which I would also > support. > > I have patches which fix the issue by using 1U in these cases. I did > not change non-broken cases. I also did not change contributed code. > > An incomplete listing of the issues available here: > http://people.freebsd.org/~eadler/files/1..31.txt > I find it particularly enjoyable to see things like this in crypto code: crypto/heimdal/lib/hx509/ref/pkcs11.h:#define CKF_EXTENSION ((unsigned long) (1 << 31)) crypto/openssh/pkcs11.h:#define CKO_VENDOR_DEFINED ((unsigned long) (1 << 31)) FWIW, This came up in both ia64 and amd64 early days. Most of this was hunted down in the kernel back then. Obviously some has crept back in, or is in contrib or driver code. The problem there is bigger though. On 64 bit machines, 1u << N tends to get used where N > 32 as well. 1u << 33 is an overflow and doesn't extend up into the 33rd bit. We changed most uses to 1ul << N where this was likely. This did predate the BIT* macros you referenced. FWIW2: we had an acpi bug with 1 << 31 way back in the early days. Back then, the compiler generated different code when compiled with -O2 vs not and 1<<31 had different values depending on compile time switches. -Peter From owner-freebsd-arch@FreeBSD.ORG Mon Nov 25 21:24:33 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 3D319784 for ; Mon, 25 Nov 2013 21:24:33 +0000 (UTC) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 04EE72A83 for ; Mon, 25 Nov 2013 21:24:32 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id AF591D65E28; Tue, 26 Nov 2013 07:54:38 +1100 (EST) Date: Tue, 26 Nov 2013 07:54:37 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eitan Adler Subject: Re: 1 << 31 and related issues In-Reply-To: Message-ID: <20131126070729.N3780@besplex.bde.org> References: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=bpB1Wiqi c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=l52YrXMppdkA:10 a=6I5d2MoRAAAA:8 a=67qok6EK7sYU6pGF0DYA:9 a=CjuIK1q_8ugA:10 Cc: "freebsd-arch@freebsd.org" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Nov 2013 21:24:33 -0000 On Mon, 25 Nov 2013, Eitan Adler wrote: > There are a few cases in FreeBSD where the expression (1 << 31) is used. More than a few. Compilers are broken for not warning about this. clang and gcc both warn for (1 << 30) + (1 << 30). Apparently, they intentionally turn off their overflow checking for bitwise expressions. > However this produces undefined behavior as '1' is of type int. (see > 6.4.4p5: The type of an unmarked integer constant is the first of the > following list in which its value can be represented: int, long int, > long long int). The shift of 31 is illegal (see 6.5.7p4) if the size > of an int is 32. The same issue arises with 2 << 30 and 3 << 30. > > I have been working on fixing this issue in a few different projects. > The correct fix here is to use 1U as the literal instead. adrian@ has Not really correct. This assumes >= 32-bit unsigned ints. 1UL doesn't assume anything, but might break the type of the result, e.g., for printf()ing. Even using 1U may break the type -- perhaps a signed type with the usual undefined behaviour of a value of INT32_MIN is what is wanted. > advocated for the macros BIT32(bit) and BIT64(bit) which I would also > support. I don't like these much. 1ULL would work for fixing (wrong_case)1 << 64, but uses the long long abomination. There must already be casts or type suffixes for 1 in 1 << 64 because this just won't work without them. (The long long abomination is now used 1017 times in /sys in just the form 1ULL. This is up from zero outside of alpha in FreeBSD-3 and 61 outside of alpha in FreeBSD-5.2. :-(). I enforced this for parts of the kernel that I used up to about 1999 by building with K&R and C90 compilers. This mainly involved changing long long literals to int64_t ones. quad_t was used excessively, and it wasn't long long so compilers accepted it. Even "K&R" and "C90" ones, by declaring int64_t as an extension (this was done up to about FreeBSD-4). Now, int64_t is used even more excessively, and it is declared as long long for 32-bit arches. C90 compilers are just not supported.) The macro would hide the details of the cast, and this seems negatively useful since you still need to know the result type for some uses. > I have patches which fix the issue by using 1U in these cases. I did > not change non-broken cases. I also did not change contributed code. It is also reasonable not to touch code in arch-dependent headers. > An incomplete listing of the issues available here: > http://people.freebsd.org/~eadler/files/1..31.txt The first few in the list (in binutils) are probably correct, since the 1 is cast (not when the cast is to bfd_signed_vma though). These are in contrib'ed code and casting is not very common. Bruce From owner-freebsd-arch@FreeBSD.ORG Mon Nov 25 21:41:09 2013 Return-Path: Delivered-To: freebsd-arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id C9460F73 for ; Mon, 25 Nov 2013 21:41:09 +0000 (UTC) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 8AFDB2BC3 for ; Mon, 25 Nov 2013 21:41:09 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 2DF69104AD86; Tue, 26 Nov 2013 08:17:23 +1100 (EST) Date: Tue, 26 Nov 2013 08:17:22 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Peter Wemm Subject: Re: 1 << 31 and related issues In-Reply-To: <5293B333.9070804@wemm.org> Message-ID: <20131126075626.A4024@besplex.bde.org> References: <5293B333.9070804@wemm.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=YYGEuWhf c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=l52YrXMppdkA:10 a=6I5d2MoRAAAA:8 a=xOKQsN7nvL1WrRQpgtsA:9 a=CjuIK1q_8ugA:10 Cc: freebsd-arch@FreeBSD.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Nov 2013 21:41:09 -0000 On Mon, 25 Nov 2013, Peter Wemm wrote: > On 11/25/13, 11:48 AM, Eitan Adler wrote: >> There are a few cases in FreeBSD where the expression (1 << 31) is used. >> ... >> An incomplete listing of the issues available here: >> http://people.freebsd.org/~eadler/files/1..31.txt > > I find it particularly enjoyable to see things like this in crypto code: > > crypto/heimdal/lib/hx509/ref/pkcs11.h:#define CKF_EXTENSION ((unsigned > long) (1 << 31)) > crypto/openssh/pkcs11.h:#define CKO_VENDOR_DEFINED ((unsigned long) (1 > << 31)) I almost said that in my earlier reply too. > FWIW, This came up in both ia64 and amd64 early days. Most of this was > hunted down in the kernel back then. Obviously some has crept back in, > or is in contrib or driver code. > > The problem there is bigger though. On 64 bit machines, 1u << N tends > to get used where N > 32 as well. 1u << 33 is an overflow and doesn't > extend up into the 33rd bit. We changed most uses to 1ul << N where > this was likely. This did predate the BIT* macros you referenced. I don't think anyone expected 1u << 33 to work. This reminds me to complain about use of the unsigned type again :-). Compilers should warn for 1 << 31, and do warn for (1 << 30) + (1 << 30), but an unsigned type almost anywhere in the expression must prevent the warning. Compilers do warn for (1u << 32), however -- cases where the shift count is too large or negative are udnefined even for unsigned types. Cases like (1u << 31) + (1 << 30) + (1 << 30) are defined (this one has value 0 with 32-bit unsigned's). Bruce From owner-freebsd-arch@FreeBSD.ORG Tue Nov 26 00:27:55 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 2676957D for ; Tue, 26 Nov 2013 00:27:55 +0000 (UTC) Received: from mail.rlwinm.de (mail.rlwinm.de [IPv6:2a01:4f8:140:72e1::ac16:e45e]) by mx1.freebsd.org (Postfix) with ESMTP id E214324C4 for ; Tue, 26 Nov 2013 00:27:54 +0000 (UTC) Received: from hexe.rlwinm.de (p57A7C0C7.dip0.t-ipconnect.de [87.167.192.199]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.rlwinm.de (Postfix) with ESMTPSA id 465595AD5 for ; Tue, 26 Nov 2013 00:27:47 +0000 (UTC) Message-ID: <5293EB02.8060509@rlwinm.de> Date: Tue, 26 Nov 2013 01:27:46 +0100 From: Jan Bramkamp User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0 MIME-Version: 1.0 To: freebsd-arch@freebsd.org Subject: Re: 1 << 31 and related issues References: <5293B333.9070804@wemm.org> <20131126075626.A4024@besplex.bde.org> In-Reply-To: <20131126075626.A4024@besplex.bde.org> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Nov 2013 00:27:55 -0000 On 25.11.2013 22:17, Bruce Evans wrote: > On Mon, 25 Nov 2013, Peter Wemm wrote: > >> On 11/25/13, 11:48 AM, Eitan Adler wrote: >>> There are a few cases in FreeBSD where the expression (1 << 31) is used. >>> ... >>> An incomplete listing of the issues available here: >>> http://people.freebsd.org/~eadler/files/1..31.txt >> >> I find it particularly enjoyable to see things like this in crypto code: >> >> crypto/heimdal/lib/hx509/ref/pkcs11.h:#define CKF_EXTENSION >> ((unsigned >> long) (1 << 31)) >> crypto/openssh/pkcs11.h:#define CKO_VENDOR_DEFINED ((unsigned long) (1 >> << 31)) > > I almost said that in my earlier reply too. > >> FWIW, This came up in both ia64 and amd64 early days. Most of this was >> hunted down in the kernel back then. Obviously some has crept back in, >> or is in contrib or driver code. >> >> The problem there is bigger though. On 64 bit machines, 1u << N tends >> to get used where N > 32 as well. 1u << 33 is an overflow and doesn't >> extend up into the 33rd bit. We changed most uses to 1ul << N where >> this was likely. This did predate the BIT* macros you referenced. > > I don't think anyone expected 1u << 33 to work. It would work on an ILP64 platform like Cray Unicos or HP Tru64 but most codebases will explode in flames on those platforms among other reasons because they are ILP64. > This reminds me to complain about use of the unsigned type again :-). > Compilers should warn for 1 << 31, and do warn for (1 << 30) + (1 << 30), > but an unsigned type almost anywhere in the expression must prevent > the warning. Compilers do warn for (1u << 32), however -- cases where > the shift count is too large or negative are udnefined even for unsigned > types. Cases like (1u << 31) + (1 << 30) + (1 << 30) are defined (this > one has value 0 with 32-bit unsigned's). > > Bruce > _______________________________________________ > freebsd-arch@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org" From owner-freebsd-arch@FreeBSD.ORG Tue Nov 26 01:20:34 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 802052E0 for ; Tue, 26 Nov 2013 01:20:34 +0000 (UTC) Received: from mail-vc0-x233.google.com (mail-vc0-x233.google.com [IPv6:2607:f8b0:400c:c03::233]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 3D5772759 for ; Tue, 26 Nov 2013 01:20:34 +0000 (UTC) Received: by mail-vc0-f179.google.com with SMTP id ie18so3286353vcb.38 for ; Mon, 25 Nov 2013 17:20:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wemm.org; s=google; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=N3dB+SN39jTjEIE5b8rcSx/mS1sVZkM0WHfzE7Mz8Tk=; b=A4X16bCGDde8ghVjUZDHFodEw7BN8LuIeGkiK1sQthWakoTR+uqvyF6mXcYwW5IFYQ yCpXPlkP89CBW/8JaPhK3Jk6whw7SDVB47dqdZaYUvu722YS0kqtyA9phI+QnPNeSDMr w9Ncwpr51vVqzOIdXgv6a34G1uKMFxYn6WvAQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=N3dB+SN39jTjEIE5b8rcSx/mS1sVZkM0WHfzE7Mz8Tk=; b=EMRr0GHPWy31SIUkDyVui1+Z6/TPNMBgcOg0pucyVicfDZpQeR2EBMv155waVd3iMy n1pfTy4OAjBdMF2qKXnvOc3iJqD7lrv2Am3T6RuLNB13F7uW5BItd12UvIZRtzS9KslF I0kQAMqB6YEQrdxeECI3PlLR0ykJYZ2hTHIWdkGkedrOEqG09jwR3TjZQ7Y3cMrcO+Mr xULX2pa793j81N6KNkPMsCUip47x2EdiTyutluacOWS/FX4b6ysSTjSHFl2BEG4UHHO4 QpLh8aRHrDzlJrtWbEacF4F6NSIEwBnvFC+MZSnKj4hSGoEtNKmGuNLkQVWvlLCKiEJ2 korQ== X-Gm-Message-State: ALoCoQkjDV6oTpeA0zMjo/FtYrNoi/SKkqJbATQC1pgFo0YMjjBaXE8fR8LQn+UQjAEYyT/TMaAi MIME-Version: 1.0 X-Received: by 10.58.118.84 with SMTP id kk20mr3321482veb.26.1385428832405; Mon, 25 Nov 2013 17:20:32 -0800 (PST) Received: by 10.220.167.74 with HTTP; Mon, 25 Nov 2013 17:20:32 -0800 (PST) In-Reply-To: <20131126075626.A4024@besplex.bde.org> References: <5293B333.9070804@wemm.org> <20131126075626.A4024@besplex.bde.org> Date: Mon, 25 Nov 2013 17:20:32 -0800 Message-ID: Subject: Re: 1 << 31 and related issues From: Peter Wemm To: Bruce Evans Content-Type: text/plain; charset=ISO-8859-1 Cc: FreeBSD Arch X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Nov 2013 01:20:34 -0000 On Mon, Nov 25, 2013 at 1:17 PM, Bruce Evans wrote: > On Mon, 25 Nov 2013, Peter Wemm wrote: > >> On 11/25/13, 11:48 AM, Eitan Adler wrote: >>> >>> There are a few cases in FreeBSD where the expression (1 << 31) is used. >>> ... >>> >>> An incomplete listing of the issues available here: >>> http://people.freebsd.org/~eadler/files/1..31.txt >> >> >> I find it particularly enjoyable to see things like this in crypto code: >> >> crypto/heimdal/lib/hx509/ref/pkcs11.h:#define CKF_EXTENSION >> ((unsigned >> long) (1 << 31)) >> crypto/openssh/pkcs11.h:#define CKO_VENDOR_DEFINED ((unsigned long) >> (1 >> << 31)) > > > I almost said that in my earlier reply too. Yep. Invalid or undefined behavior in crypto or security critical code is hardly a good thing, even if it usually works out ok. >> FWIW, This came up in both ia64 and amd64 early days. Most of this was >> hunted down in the kernel back then. Obviously some has crept back in, >> or is in contrib or driver code. >> >> The problem there is bigger though. On 64 bit machines, 1u << N tends >> to get used where N > 32 as well. 1u << 33 is an overflow and doesn't >> extend up into the 33rd bit. We changed most uses to 1ul << N where >> this was likely. This did predate the BIT* macros you referenced. > > > I don't think anyone expected 1u << 33 to work. Well, when we had a cpumask_t as a 64 bit integer type and did (1 << cpu) in both MI and MD code, it was a pretty big deal before cpuset_t came along. -- Peter Wemm - peter@wemm.org; peter@FreeBSD.org; peter@yahoo-inc.com; KI6FJV UTF-8: for when a ' just won\342\200\231t do. From owner-freebsd-arch@FreeBSD.ORG Tue Nov 26 18:44:55 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A4881BC0; Tue, 26 Nov 2013 18:44:55 +0000 (UTC) Received: from mail-qa0-x22f.google.com (mail-qa0-x22f.google.com [IPv6:2607:f8b0:400d:c00::22f]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 344672403; Tue, 26 Nov 2013 18:44:55 +0000 (UTC) Received: by mail-qa0-f47.google.com with SMTP id w5so4753349qac.20 for ; Tue, 26 Nov 2013 10:44:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=content-type:mime-version:subject:from:in-reply-to:date:cc :message-id:references:to; bh=tna7gGd8vew1HBVocdXnxozLc+QgD9eRT/jIqcm5ul4=; b=vxhCKbhJyNuAMLAXxNAKrZn+e0rAk2UoNlH3qyyKdAd5kykeoFREgYtEkUW7BINvwp /pIuSimFG0e3zbYgCZ9Nxeb0vymWkT6tvT/2sCzBYepbNWI9X77TBbThHhg5dMQAG0LZ UTZqOka4f8mVkyrgPHuuowkuE8an4mGlxqWOUUAZp3JyJ1Ke5Jg4sdJqrCytmEbHEvAu ZOamNlOeohft92bOCS48jzCQ1qRs2bHnYO18osXv9h6YpuZW9fIZ/Bhz2aH3RougkBML Lbdp9kkWOtusXYS09u3wMWRxfQNQv6r8qcaFULrbhPdJnaBrMffRd+Znrs3uZ24NnPLh W7xQ== X-Received: by 10.224.94.8 with SMTP id x8mr59293405qam.1.1385491494393; Tue, 26 Nov 2013 10:44:54 -0800 (PST) Received: from [192.168.1.7] ([187.120.137.162]) by mx.google.com with ESMTPSA id lc1sm7876434qeb.5.2013.11.26.10.44.52 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 26 Nov 2013 10:44:53 -0800 (PST) Content-Type: multipart/mixed; boundary="Apple-Mail=_160E938F-A6C1-49B1-990B-54725E9C61A5" Mime-Version: 1.0 (Mac OS X Mail 7.0 \(1822\)) Subject: Re: FDT Support for GPIO (gpiobus and friends) From: Luiz Otavio O Souza In-Reply-To: Date: Tue, 26 Nov 2013 16:44:44 -0200 Message-Id: <7DBC8BB4-8CEF-48E0-BB6B-40C00F71284A@gmail.com> References: <20121205.060056.592894859995638978.hrs@allbsd.org> To: freebsd-arch@FreeBSD.org X-Mailer: Apple Mail (2.1822) Cc: Rui Paulo X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Nov 2013 18:44:55 -0000 --Apple-Mail=_160E938F-A6C1-49B1-990B-54725E9C61A5 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Right, so after playing a little bit with IIC and SPI on FDT i have now = a patch, which i believe, is more clean and complete to support an OFW = GPIO bus in the same way as it has been done with IIC and SPI. The first patch (001-ofw-gpiobus.diff) implements the OFW GPIO bus and = all the changes to make possible to the available gpio devices (gpioled = and gpioiic) attach to it. The second patch (002-iicbb-ofw-iicbus.diff) implements the changes to = make the OFW IIC bus attach to a gpioiic bit-banging (iicbb(4)) kind of = driver. The third patch (006-ofw-gpio-man.diff) has the manual changes for = gpioiic and gpioled. I would appreciate if you people could take a look and give me some = feedback. Luiz --Apple-Mail=_160E938F-A6C1-49B1-990B-54725E9C61A5 Content-Disposition: attachment; filename=001-ofw-gpiobus.diff Content-Type: application/octet-stream; name="001-ofw-gpiobus.diff" Content-Transfer-Encoding: 7bit Index: sys/conf/files =================================================================== --- sys/conf/files (revision 258138) +++ sys/conf/files (working copy) @@ -1432,6 +1432,7 @@ dev/gpio/gpioled.c optional gpioled dev/gpio/gpio_if.m optional gpio dev/gpio/gpiobus_if.m optional gpio +dev/gpio/ofw_gpiobus.c optional fdt gpio dev/hatm/if_hatm.c optional hatm pci dev/hatm/if_hatm_intr.c optional hatm pci dev/hatm/if_hatm_ioctl.c optional hatm pci Index: sys/dev/gpio/gpiobus.c =================================================================== --- sys/dev/gpio/gpiobus.c (revision 258138) +++ sys/dev/gpio/gpiobus.c (working copy) @@ -46,7 +46,6 @@ #include "gpio_if.h" #include "gpiobus_if.h" -static void gpiobus_print_pins(struct gpiobus_ivar *); static int gpiobus_parse_pins(struct gpiobus_softc *, device_t, int); static int gpiobus_probe(device_t); static int gpiobus_attach(device_t); @@ -73,17 +72,7 @@ static int gpiobus_pin_get(device_t, device_t, uint32_t, unsigned int*); static int gpiobus_pin_toggle(device_t, device_t, uint32_t); -#define GPIOBUS_LOCK(_sc) mtx_lock(&(_sc)->sc_mtx) -#define GPIOBUS_UNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx) -#define GPIOBUS_LOCK_INIT(_sc) \ - mtx_init(&_sc->sc_mtx, device_get_nameunit(_sc->sc_dev), \ - "gpiobus", MTX_DEF) -#define GPIOBUS_LOCK_DESTROY(_sc) mtx_destroy(&_sc->sc_mtx); -#define GPIOBUS_ASSERT_LOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_OWNED); -#define GPIOBUS_ASSERT_UNLOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_NOTOWNED); - - -static void +void gpiobus_print_pins(struct gpiobus_ivar *devi) { int range_start, range_stop, need_coma; Index: sys/dev/gpio/gpiobusvar.h =================================================================== --- sys/dev/gpio/gpiobusvar.h (revision 258138) +++ sys/dev/gpio/gpiobusvar.h (working copy) @@ -30,13 +30,26 @@ #ifndef __GPIOBUS_H__ #define __GPIOBUS_H__ +#include "opt_platform.h" + #include #include #include -#define GPIOBUS_IVAR(d) (struct gpiobus_ivar *) device_get_ivars(d) -#define GPIOBUS_SOFTC(d) (struct gpiobus_softc *) device_get_softc(d) +#ifdef FDT +#include +#endif +#define GPIOBUS_IVAR(d) (struct gpiobus_ivar *) device_get_ivars(d) +#define GPIOBUS_SOFTC(d) (struct gpiobus_softc *) device_get_softc(d) +#define GPIOBUS_LOCK(_sc) mtx_lock(&(_sc)->sc_mtx) +#define GPIOBUS_UNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx) +#define GPIOBUS_LOCK_INIT(_sc) mtx_init(&_sc->sc_mtx, \ + device_get_nameunit(_sc->sc_dev), "gpiobus", MTX_DEF) +#define GPIOBUS_LOCK_DESTROY(_sc) mtx_destroy(&_sc->sc_mtx) +#define GPIOBUS_ASSERT_LOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_OWNED) +#define GPIOBUS_ASSERT_UNLOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_NOTOWNED) + struct gpiobus_softc { struct mtx sc_mtx; /* bus mutex */ @@ -54,4 +67,11 @@ uint32_t *pins; /* pins map */ }; +void gpiobus_print_pins(struct gpiobus_ivar *); +#ifdef FDT +device_t ofw_gpiobus_add_fdt_child(device_t, phandle_t); +#endif + +extern driver_t gpiobus_driver; + #endif /* __GPIOBUS_H__ */ Index: sys/dev/gpio/gpioiic.c =================================================================== --- sys/dev/gpio/gpioiic.c (revision 258138) +++ sys/dev/gpio/gpioiic.c (working copy) @@ -28,6 +28,8 @@ #include __FBSDID("$FreeBSD$"); +#include "opt_platform.h" + #include #include #include @@ -43,6 +45,12 @@ #include #include "gpiobus_if.h" +#ifdef FDT +#include +#include +#include +#endif + #include #include @@ -77,7 +85,12 @@ gpioiic_probe(device_t dev) { +#ifdef FDT + if (!ofw_bus_is_compatible(dev, "gpioiic")) + return (ENXIO); +#endif device_set_desc(dev, "GPIO I2C bit-banging driver"); + return (0); } @@ -86,6 +99,10 @@ { struct gpioiic_softc *sc = device_get_softc(dev); device_t bitbang; +#ifdef FDT + phandle_t node; + pcell_t pin; +#endif sc->sc_dev = dev; sc->sc_busdev = device_get_parent(dev); @@ -96,6 +113,15 @@ device_get_unit(dev), "sda", &sc->sda_pin)) sc->sda_pin = SDA_PIN_DEFAULT; +#ifdef FDT + if ((node = ofw_bus_get_node(dev)) == -1) + return (ENXIO); + if (OF_getencprop(node, "scl", &pin, sizeof(pin)) > 0) + sc->scl_pin = (int)pin; + if (OF_getencprop(node, "sda", &pin, sizeof(pin)) > 0) + sc->sda_pin = (int)pin; +#endif + /* add generic bit-banging code */ bitbang = device_add_child(dev, "iicbb", -1); device_probe_and_attach(bitbang); @@ -214,6 +240,16 @@ return (IIC_ENOADDR); } +#ifdef FDT +static phandle_t +gpioiic_get_node(device_t bus, device_t dev) +{ + + /* We only have one child, the iicbb, which needs our own node. */ + return (ofw_bus_get_node(bus)); +} +#endif + static devclass_t gpioiic_devclass; static device_method_t gpioiic_methods[] = { @@ -230,6 +266,11 @@ DEVMETHOD(iicbb_getscl, gpioiic_getscl), DEVMETHOD(iicbb_reset, gpioiic_reset), +#ifdef FDT + /* OFW bus interface */ + DEVMETHOD(ofw_bus_get_node, gpioiic_get_node), +#endif + { 0, 0 } }; Index: sys/dev/gpio/gpioled.c =================================================================== --- sys/dev/gpio/gpioled.c (revision 258138) +++ sys/dev/gpio/gpioled.c (working copy) @@ -27,6 +27,8 @@ #include __FBSDID("$FreeBSD$"); +#include "opt_platform.h" + #include #include #include @@ -39,6 +41,12 @@ #include #include +#ifdef FDT +#include +#include +#include +#endif + #include #include #include "gpiobus_if.h" @@ -82,10 +90,65 @@ GPIOLED_UNLOCK(sc); } +#ifdef FDT +static void +gpioled_identify(driver_t *driver, device_t bus) +{ + phandle_t child, leds, root; + + root = OF_finddevice("/"); + if (root == 0) + return; + leds = fdt_find_compatible(root, "gpio-leds", 1); + if (leds == 0) + return; + + /* Traverse the 'gpio-leds' node and add its children. */ + for (child = OF_child(leds); child != 0; child = OF_peer(child)) + if (ofw_gpiobus_add_fdt_child(bus, child) == NULL) + continue; +} +#endif + static int gpioled_probe(device_t dev) { +#ifdef FDT + int match; + phandle_t node; + char *compat; + + /* + * We can match against our own node compatible string and also against + * our parent node compatible string. The first is normally used to + * describe leds on a gpiobus and the later when there is a common node + * compatible with 'gpio-leds' which is used to concentrate all the + * leds nodes on the dts. + */ + match = 0; + if (ofw_bus_is_compatible(dev, "gpioled")) + match = 1; + + if (match == 0) { + if ((node = ofw_bus_get_node(dev)) == -1) + return (ENXIO); + if ((node = OF_parent(node)) == -1) + return (ENXIO); + if (OF_getprop_alloc(node, "compatible", 1, + (void **)&compat) == -1) + return (ENXIO); + + if (strcasecmp(compat, "gpio-leds") == 0) + match = 1; + + free(compat, M_OFWPROP); + } + + if (match == 0) + return (ENXIO); +#endif device_set_desc(dev, "GPIO led"); + return (0); } @@ -93,15 +156,28 @@ gpioled_attach(device_t dev) { struct gpioled_softc *sc; +#ifdef FDT + phandle_t node; + char *name; +#else const char *name; +#endif sc = device_get_softc(dev); sc->sc_dev = dev; sc->sc_busdev = device_get_parent(dev); GPIOLED_LOCK_INIT(sc); +#ifdef FDT + name = NULL; + if ((node = ofw_bus_get_node(dev)) == -1) + return (ENXIO); + if (OF_getprop_alloc(node, "label", 1, (void **)&name) == -1) + OF_getprop_alloc(node, "name", 1, (void **)&name); +#else if (resource_string_value(device_get_name(dev), device_get_unit(dev), "name", &name)) name = NULL; +#endif GPIOBUS_PIN_SETFLAGS(sc->sc_busdev, sc->sc_dev, GPIOLED_PIN, GPIO_PIN_OUTPUT); @@ -108,6 +184,10 @@ sc->sc_leddev = led_create(gpioled_control, sc, name ? name : device_get_nameunit(dev)); +#ifdef FDT + if (name != NULL) + free(name, M_OFWPROP); +#endif return (0); } @@ -130,6 +210,9 @@ static device_method_t gpioled_methods[] = { /* Device interface */ +#ifdef FDT + DEVMETHOD(device_identify, gpioled_identify), +#endif DEVMETHOD(device_probe, gpioled_probe), DEVMETHOD(device_attach, gpioled_attach), DEVMETHOD(device_detach, gpioled_detach), --- /dev/null 2013-11-26 11:30:06.000000000 -0200 +++ sys/dev/gpio/ofw_gpiobus.c 2013-11-25 10:15:00.322070491 -0200 @@ -0,0 +1,342 @@ +/*- + * Copyright (c) 2009, Nathan Whitehorn + * Copyright (c) 2013, Luiz Otavio O Souza + * Copyright (c) 2013 The FreeBSD Foundation + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice unmodified, this list of conditions, and the following + * disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include +__FBSDID("$FreeBSD$"); + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include + +#include "gpio_if.h" +#include "gpiobus_if.h" + +struct ofw_gpiobus_devinfo { + struct gpiobus_ivar opd_dinfo; + struct ofw_bus_devinfo opd_obdinfo; +}; + +static int ofw_gpiobus_parse_gpios(struct gpiobus_softc *, + struct gpiobus_ivar *, phandle_t); +static struct ofw_gpiobus_devinfo *ofw_gpiobus_setup_devinfo(device_t, + phandle_t); +static void ofw_gpiobus_destroy_devinfo(struct ofw_gpiobus_devinfo *); + +device_t +ofw_gpiobus_add_fdt_child(device_t bus, phandle_t child) +{ + struct ofw_gpiobus_devinfo *dinfo; + device_t childdev; + + /* + * Set up the GPIO child and OFW bus layer devinfo and add it to bus. + */ + dinfo = ofw_gpiobus_setup_devinfo(bus, child); + if (dinfo == NULL) + return (NULL); + childdev = device_add_child(bus, NULL, -1); + if (childdev == NULL) { + device_printf(bus, "could not add child: %s\n", + dinfo->opd_obdinfo.obd_name); + ofw_gpiobus_destroy_devinfo(dinfo); + return (NULL); + } + device_set_ivars(childdev, dinfo); + + return (childdev); +} + +static int +ofw_gpiobus_parse_gpios(struct gpiobus_softc *sc, struct gpiobus_ivar *dinfo, + phandle_t child) +{ + int i, len; + pcell_t *gpios; + phandle_t gpio; + + /* Check and process 'status' property. */ + if (!(fdt_is_enabled(child))) + return (ENXIO); + + /* Retrieve the gpios property. */ + if ((len = OF_getproplen(child, "gpios")) < 0) + return (EINVAL); + gpios = malloc(len, M_DEVBUF, M_NOWAIT | M_ZERO); + if (gpios == NULL) + return (ENOMEM); + if (OF_getencprop(child, "gpios", gpios, len) < 0) { + free(gpios, M_DEVBUF); + return (EINVAL); + } + + /* + * Each 'gpios' entry must contain 4 pcells. + * The first one is the GPIO controller phandler. + * Then the last three are the GPIO pin, the GPIO pin direction and + * the GPIO pin flags. + */ + if ((len / sizeof(pcell_t)) % 4) { + free(gpios, M_DEVBUF); + return (EINVAL); + } + dinfo->npins = len / (sizeof(pcell_t) * 4); + dinfo->pins = malloc(sizeof(uint32_t) * dinfo->npins, M_DEVBUF, + M_NOWAIT | M_ZERO); + if (dinfo->pins == NULL) { + free(gpios, M_DEVBUF); + return (ENOMEM); + } + + for (i = 0; i < dinfo->npins; i++) { + + /* Verify if we're attaching to the correct gpio controller. */ + gpio = OF_xref_phandle(gpios[i * 4 + 0]); + if (!OF_hasprop(gpio, "gpio-controller") || + gpio != ofw_bus_get_node(sc->sc_dev)) { + free(dinfo->pins, M_DEVBUF); + free(gpios, M_DEVBUF); + return (EINVAL); + } + + /* Get the GPIO pin number. */ + dinfo->pins[i] = gpios[i * 4 + 1]; + /* gpios[i * 4 + 2] - GPIO pin direction */ + /* gpios[i * 4 + 3] - GPIO pin flags */ + + if (dinfo->pins[i] > sc->sc_npins) { + device_printf(sc->sc_busdev, + "invalid pin %d, max: %d\n", + dinfo->pins[i], sc->sc_npins - 1); + free(dinfo->pins, M_DEVBUF); + free(gpios, M_DEVBUF); + return (EINVAL); + } + + /* + * Mark pin as mapped and give warning if it's already mapped. + */ + if (sc->sc_pins_mapped[dinfo->pins[i]]) { + device_printf(sc->sc_busdev, + "warning: pin %d is already mapped\n", + dinfo->pins[i]); + free(dinfo->pins, M_DEVBUF); + free(gpios, M_DEVBUF); + return (EINVAL); + } + sc->sc_pins_mapped[dinfo->pins[i]] = 1; + } + + free(gpios, M_DEVBUF); + + return (0); +} + +static struct ofw_gpiobus_devinfo * +ofw_gpiobus_setup_devinfo(device_t dev, phandle_t node) +{ + struct gpiobus_softc *sc; + struct ofw_gpiobus_devinfo *dinfo; + + sc = device_get_softc(dev); + dinfo = malloc(sizeof(*dinfo), M_DEVBUF, M_NOWAIT | M_ZERO); + if (dinfo == NULL) + return (NULL); + if (ofw_bus_gen_setup_devinfo(&dinfo->opd_obdinfo, node) != 0) { + free(dinfo, M_DEVBUF); + return (NULL); + } + + /* Parse the gpios property for the child. */ + if (ofw_gpiobus_parse_gpios(sc, &dinfo->opd_dinfo, node) != 0) { + ofw_bus_gen_destroy_devinfo(&dinfo->opd_obdinfo); + free(dinfo, M_DEVBUF); + return (NULL); + } + + return (dinfo); +} + +static void +ofw_gpiobus_destroy_devinfo(struct ofw_gpiobus_devinfo *dinfo) +{ + + ofw_bus_gen_destroy_devinfo(&dinfo->opd_obdinfo); + free(dinfo, M_DEVBUF); +} + +static int +ofw_gpiobus_probe(device_t dev) +{ + + if (ofw_bus_get_node(dev) == -1) + return (ENXIO); + device_set_desc(dev, "OFW GPIO bus"); + + return (0); +} + +static int +ofw_gpiobus_attach(device_t dev) +{ + struct gpiobus_softc *sc; + phandle_t child; + + sc = GPIOBUS_SOFTC(dev); + sc->sc_busdev = dev; + sc->sc_dev = device_get_parent(dev); + + /* Read the pin max. value */ + if (GPIO_PIN_MAX(sc->sc_dev, &sc->sc_npins) != 0) + return (ENXIO); + + KASSERT(sc->sc_npins != 0, ("GPIO device with no pins")); + + /* + * Increase to get number of pins. + */ + sc->sc_npins++; + + sc->sc_pins_mapped = malloc(sizeof(int) * sc->sc_npins, M_DEVBUF, + M_NOWAIT | M_ZERO); + + if (!sc->sc_pins_mapped) + return (ENOMEM); + + /* Init the bus lock. */ + GPIOBUS_LOCK_INIT(sc); + + bus_generic_probe(dev); + bus_enumerate_hinted_children(dev); + + /* + * Attach the children represented in the device tree. + */ + for (child = OF_child(ofw_bus_get_node(dev)); child != 0; + child = OF_peer(child)) + if (ofw_gpiobus_add_fdt_child(dev, child) == NULL) + continue; + + return (bus_generic_attach(dev)); +} + +static device_t +ofw_gpiobus_add_child(device_t dev, u_int order, const char *name, int unit) +{ + device_t child; + struct ofw_gpiobus_devinfo *devi; + + child = device_add_child_ordered(dev, order, name, unit); + if (child == NULL) + return (child); + devi = malloc(sizeof(struct ofw_gpiobus_devinfo), M_DEVBUF, + M_NOWAIT | M_ZERO); + if (devi == NULL) { + device_delete_child(dev, child); + return (0); + } + + /* + * NULL all the OFW-related parts of the ivars for non-OFW + * children. + */ + devi->opd_obdinfo.obd_node = -1; + devi->opd_obdinfo.obd_name = NULL; + devi->opd_obdinfo.obd_compat = NULL; + devi->opd_obdinfo.obd_type = NULL; + devi->opd_obdinfo.obd_model = NULL; + + device_set_ivars(child, devi); + + return (child); +} + +static int +ofw_gpiobus_print_child(device_t dev, device_t child) +{ + struct ofw_gpiobus_devinfo *devi; + int retval = 0; + + devi = device_get_ivars(child); + retval += bus_print_child_header(dev, child); + retval += printf(" at pin(s) "); + gpiobus_print_pins(&devi->opd_dinfo); + retval += bus_print_child_footer(dev, child); + + return (retval); +} + +static const struct ofw_bus_devinfo * +ofw_gpiobus_get_devinfo(device_t bus, device_t dev) +{ + struct ofw_gpiobus_devinfo *dinfo; + + dinfo = device_get_ivars(dev); + + return (&dinfo->opd_obdinfo); +} + +static device_method_t ofw_gpiobus_methods[] = { + /* Device interface */ + DEVMETHOD(device_probe, ofw_gpiobus_probe), + DEVMETHOD(device_attach, ofw_gpiobus_attach), + + /* Bus interface */ + DEVMETHOD(bus_child_pnpinfo_str, ofw_bus_gen_child_pnpinfo_str), + DEVMETHOD(bus_print_child, ofw_gpiobus_print_child), + DEVMETHOD(bus_add_child, ofw_gpiobus_add_child), + + /* ofw_bus interface */ + DEVMETHOD(ofw_bus_get_devinfo, ofw_gpiobus_get_devinfo), + DEVMETHOD(ofw_bus_get_compat, ofw_bus_gen_get_compat), + DEVMETHOD(ofw_bus_get_model, ofw_bus_gen_get_model), + DEVMETHOD(ofw_bus_get_name, ofw_bus_gen_get_name), + DEVMETHOD(ofw_bus_get_node, ofw_bus_gen_get_node), + DEVMETHOD(ofw_bus_get_type, ofw_bus_gen_get_type), + + DEVMETHOD_END +}; + +static devclass_t ofwgpiobus_devclass; + +DEFINE_CLASS_1(gpiobus, ofw_gpiobus_driver, ofw_gpiobus_methods, + sizeof(struct gpiobus_softc), gpiobus_driver); +DRIVER_MODULE(ofw_gpiobus, gpio, ofw_gpiobus_driver, ofwgpiobus_devclass, 0, 0); +MODULE_VERSION(ofw_gpiobus, 1); +MODULE_DEPEND(ofw_gpiobus, gpiobus, 1, 1, 1); --Apple-Mail=_160E938F-A6C1-49B1-990B-54725E9C61A5 Content-Disposition: attachment; filename=002-iicbb-ofw-iicbus.diff Content-Type: application/octet-stream; name="002-iicbb-ofw-iicbus.diff" Content-Transfer-Encoding: 7bit Index: sys/dev/iicbus/iicbb.c =================================================================== --- sys/dev/iicbus/iicbb.c (revision 258138) +++ sys/dev/iicbus/iicbb.c (working copy) @@ -43,6 +43,8 @@ * */ +#include "opt_platform.h" + #include #include #include @@ -50,6 +52,11 @@ #include #include +#ifdef FDT +#include +#include +#include +#endif #include #include @@ -77,6 +84,9 @@ static int iicbb_read(device_t, char *, int, int *, int, int); static int iicbb_reset(device_t, u_char, u_char, u_char *); static int iicbb_transfer(device_t dev, struct iic_msg *msgs, uint32_t nmsgs); +#ifdef FDT +static phandle_t iicbb_get_node(device_t, device_t); +#endif static device_method_t iicbb_methods[] = { /* device interface */ @@ -98,6 +108,11 @@ DEVMETHOD(iicbus_reset, iicbb_reset), DEVMETHOD(iicbus_transfer, iicbb_transfer), +#ifdef FDT + /* ofw_bus interface */ + DEVMETHOD(ofw_bus_get_node, iicbb_get_node), +#endif + { 0, 0 } }; @@ -154,6 +169,16 @@ return (0); } +#ifdef FDT +static phandle_t +iicbb_get_node(device_t bus, device_t dev) +{ + + /* We only have one child, the I2C bus, which needs our own node. */ + return (ofw_bus_get_node(bus)); +} +#endif + static void iicbb_child_detached( device_t dev, device_t child ) { Index: sys/dev/ofw/ofw_iicbus.c =================================================================== --- sys/dev/ofw/ofw_iicbus.c (revision 258138) +++ sys/dev/ofw/ofw_iicbus.c (working copy) @@ -80,6 +80,7 @@ DEFINE_CLASS_1(iicbus, ofw_iicbus_driver, ofw_iicbus_methods, sizeof(struct iicbus_softc), iicbus_driver); +DRIVER_MODULE(ofw_iicbus, iicbb, ofw_iicbus_driver, ofwiicbus_devclass, 0, 0); DRIVER_MODULE(ofw_iicbus, iichb, ofw_iicbus_driver, ofwiicbus_devclass, 0, 0); MODULE_VERSION(ofw_iicbus, 1); MODULE_DEPEND(ofw_iicbus, iicbus, 1, 1, 1); --Apple-Mail=_160E938F-A6C1-49B1-990B-54725E9C61A5 Content-Disposition: attachment; filename=006-ofw-gpio-man.diff Content-Type: application/octet-stream; name="006-ofw-gpio-man.diff" Content-Transfer-Encoding: 7bit Index: share/man/man4/gpioiic.4 =================================================================== --- share/man/man4/gpioiic.4 (revision 258550) +++ share/man/man4/gpioiic.4 (working copy) @@ -24,7 +24,7 @@ .\" .\" $FreeBSD$ .\" -.Dd November 5, 2013 +.Dd November 26, 2013 .Dt GPIOIIC 4 .Os .Sh NAME @@ -73,12 +73,75 @@ .Va hint.gpioiic.%d.pins should be used as the SCLOCK source. +Optional, defaults to 0. .It Va hint.gpioiic.%d.sda Indicates which bit in the .Va hint.gpioiic.%d.pins should be used as the SDATA source. +Optional, defaults to 1. .El +.Pp +On a +.Xr FDT 4 +based system, like +.Li ARM , the dts part for a +.Nm gpioiic +device usually looks like: +.Bd -literal +gpio: gpio { + + gpio-controller; + ... + + gpioiic0 { + compatible = "gpioiic"; + /* + * Attach to GPIO pins 21 and 22. Set them + * initially as inputs. + */ + gpios = <&gpio 21 1 0 + &gpio 22 1 0>; + scl = <0>; /* GPIO pin 21 */ + sda = <1>; /* GPIO pin 22 */ + + /* This is an example of a gpioiic child. */ + gpioiic-child0 { + compatible = "lm75"; + i2c-address = <0x9e>; + }; + }; +}; +.Ed +.Pp +Where: +.Bl -tag -width ".Va compatible" +.It Va compatible +Should always be set to "gpioiic". +.It Va gpios +The +.Va gpios +property indicates which GPIO pins should be used for SCLOCK and SDATA +on the GPIO IIC bit-banging bus. +For more details about the +.Va gpios +property, please consult +.Pa /usr/src/sys/boot/fdt/dts/bindings-gpio.txt . +.It Va scl +The +.Va scl +option indicates which bit in the +.Va gpios +should be used as the SCLOCK source. +Optional, defaults to 0. +.It Va sda +The +.Va sda +option indicates which bit in the +.Va gpios +should be used as the SDATA source. +Optional, defaults to 1. +.El .Sh SEE ALSO .Xr gpio 4 , .Xr gpioled 4 , Index: share/man/man4/gpioled.4 =================================================================== --- share/man/man4/gpioled.4 (revision 258550) +++ share/man/man4/gpioled.4 (working copy) @@ -24,7 +24,7 @@ .\" .\" $FreeBSD$ .\" -.Dd November 5, 2013 +.Dd November 26, 2013 .Dt GPIOLED 4 .Os .Sh NAME @@ -68,6 +68,73 @@ Please note that this mask should only ever have one bit set (any others bits - i.e., pins - will be ignored). .El +.Pp +On a +.Xr FDT 4 +based system, like +.Li ARM , the dts part for a +.Nm gpioled +device usually looks like: +.Bd -literal +gpio: gpio { + + gpio-controller; + ... + + led0 { + compatible = "gpioled"; + gpios = <&gpio 16 2 0>; /* GPIO pin 16. */ + name = "ok"; + }; + + led1 { + compatible = "gpioled"; + gpios = <&gpio 17 2 0>; /* GPIO pin 17. */ + name = "user-led1"; + }; +}; +.Ed +.Pp +And optionally, you can choose combine all the leds under a single +.Dq gpio-leds +compatible node: +.Bd -literal +simplebus0 { + + ... + + leds { + compatible = "gpio-leds"; + + led0 { + gpios = <&gpio 16 2 0>; + name = "ok" + }; + + led1 { + gpios = <&gpio 17 2 0>; + name = "user-led1" + }; + }; +}; +.Ed +.Pp +Both methods are equally supported and it is possible to have the leds +defined with any sort of mix between the methods. +The only restriction is that a GPIO pin cannot be mapped by two different +(gpio)leds. +.Pp +For more details about the +.Va gpios +property, please consult +.Pa /usr/src/sys/boot/fdt/dts/bindings-gpio.txt . +.Pp +The property +.Va name +is the arbitrary name of device in +.Pa /dev/led/ +to create for +.Xr led 4 . .Sh SEE ALSO .Xr gpio 4 , .Xr led 4 , --Apple-Mail=_160E938F-A6C1-49B1-990B-54725E9C61A5-- From owner-freebsd-arch@FreeBSD.ORG Wed Nov 27 05:00:16 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 526FEC4B; Wed, 27 Nov 2013 05:00:16 +0000 (UTC) Received: from mail-ob0-x232.google.com (mail-ob0-x232.google.com [IPv6:2607:f8b0:4003:c01::232]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id A76D326E9; Wed, 27 Nov 2013 05:00:15 +0000 (UTC) Received: by mail-ob0-f178.google.com with SMTP id uz6so6850904obc.37 for ; Tue, 26 Nov 2013 21:00:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=7wp8Ot4d3RPOEuzGIKE8Jz7/RDGhPDbWHY7y/6mBhm0=; b=rYdUJseyUwPHWqcQklRUoisRlU/voDrjkEVu56SE2PRXK78VJrYZJnNkvZiZBe5msk t/dLUItELVpt+dIhsjvwWuAiv1RrnF7x57H2g0TVkwJHjhGAIkHovwcqVxRzn6Zlmzzx nTVppQdatBEq0cIU1cpNvgv9YXGe9uIQiXz/LxwdV6MtBYe7lERQseDdfldx+2aRPzmz saZfxjWp5fyunamrXBxnfd/jkk6Bwv+Z9HiHdtMt5KxYvP9iKl3PWnBfPjXIV+aU9VWn KPRPzD2pjNEARv8GBunsYArF+m3CI8U59rvzEPK7m4AIu3jKYPobj07ne3ac3kXxVxiq 4eqQ== MIME-Version: 1.0 X-Received: by 10.60.145.241 with SMTP id sx17mr8276633oeb.57.1385528414314; Tue, 26 Nov 2013 21:00:14 -0800 (PST) Received: by 10.182.153.65 with HTTP; Tue, 26 Nov 2013 21:00:14 -0800 (PST) In-Reply-To: References: <201311182258.rAIMwEFd048783@svn.freebsd.org> <528D6173.4080406@freebsd.org> Date: Tue, 26 Nov 2013 21:00:14 -0800 Message-ID: Subject: Re: svn commit: r258328 - head/sys/net From: Vijay Singh To: Robert Watson Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.16 Cc: Adrian Chadd , "src-committers@freebsd.org" , FreeBSD Net , "svn-src-all@freebsd.org" , "George V. Neville-Neil" , "freebsd-arch@freebsd.org" , "svn-src-head@freebsd.org" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Nov 2013 05:00:16 -0000 Sorry to join this late, I've been busy preparing other patches to roll out. I am OK either way. FWIW we're running with this @netapp for more than 2 years, but we do use only very few drivers. I would like to avoid the proliferation of APIs but Robert's point also does make sense. On Sat, Nov 23, 2013 at 2:57 AM, Robert Watson wrote: > On Wed, 20 Nov 2013, Julian Elischer wrote: > > After that it'd be nice to write a set of mbuf list macros for abstract >>> the whole queue, dequeue, concat, iterate, etc (like sys/queue.h, but for >>> mbufs.) >>> >>> What do people think? >>> >>> (I've been doing it for m->next chained things, but not m->m_nextpkt >>> things..) >>> >> >> I was thinking: new interfaces.. (your -multi names sound good). macros >> for handling said lists so that people don't screw them up Old drivers run >> with no change. >> > > To me, the name "multi" is ambiguous: could be multicast. If we call the > new datastructure "mbqueue" or "mqueue", then we should name the method > ether_input_mbqueue or ether_input_mqueue. > > Robert > > > > > > >> >>> >>> >>> -adrian >>> >>> >>> On 18 November 2013 14:58, George V. Neville-Neil >>> wrote: >>> >>>> Author: gnn >>>> Date: Mon Nov 18 22:58:14 2013 >>>> New Revision: 258328 >>>> URL: http://svnweb.freebsd.org/changeset/base/258328 >>>> >>>> Log: >>>> Allow ethernet drivers to pass in packets connected via the nextpkt >>>> pointer. >>>> Handling packets in this way allows drivers to amortize work during >>>> packet reception. >>>> >>>> Submitted by: Vijay Singh >>>> Sponsored by: NetApp >>>> >>>> Modified: >>>> head/sys/net/if_ethersubr.c >>>> >>>> Modified: head/sys/net/if_ethersubr.c >>>> ============================================================ >>>> ================== >>>> --- head/sys/net/if_ethersubr.c Mon Nov 18 22:55:50 2013 >>>> (r258327) >>>> +++ head/sys/net/if_ethersubr.c Mon Nov 18 22:58:14 2013 >>>> (r258328) >>>> @@ -708,13 +708,25 @@ static void >>>> ether_input(struct ifnet *ifp, struct mbuf *m) >>>> { >>>> >>>> + struct mbuf *mn; >>>> + >>>> /* >>>> - * We will rely on rcvif being set properly in the deferred >>>> context, >>>> - * so assert it is correct here. >>>> + * The drivers are allowed to pass in a chain of packets linked >>>> with >>>> + * m_nextpkt. We split them up into separate packets here and >>>> pass >>>> + * them up. This allows the drivers to amortize the receive >>>> lock. >>>> */ >>>> - KASSERT(m->m_pkthdr.rcvif == ifp, ("%s: ifnet mismatch", >>>> __func__)); >>>> + while (m) { >>>> + mn = m->m_nextpkt; >>>> + m->m_nextpkt = NULL; >>>> >>>> - netisr_dispatch(NETISR_ETHER, m); >>>> + /* >>>> + * We will rely on rcvif being set properly in the >>>> deferred context, >>>> + * so assert it is correct here. >>>> + */ >>>> + KASSERT(m->m_pkthdr.rcvif == ifp, ("%s: ifnet >>>> mismatch", __func__)); >>>> + netisr_dispatch(NETISR_ETHER, m); >>>> + m = mn; >>>> + } >>>> } >>>> >>>> /* >>>> >>> >> >> _______________________________________________ > svn-src-head@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/svn-src-head > To unsubscribe, send any mail to "svn-src-head-unsubscribe@freebsd.org" > From owner-freebsd-arch@FreeBSD.ORG Wed Nov 27 19:36:45 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 6F54F99C; Wed, 27 Nov 2013 19:36:45 +0000 (UTC) Received: from mail-qa0-x235.google.com (mail-qa0-x235.google.com [IPv6:2607:f8b0:400d:c00::235]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 027252389; Wed, 27 Nov 2013 19:36:44 +0000 (UTC) Received: by mail-qa0-f53.google.com with SMTP id j5so9805744qaq.5 for ; Wed, 27 Nov 2013 11:36:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:date:message-id:subject:from:to:content-type; bh=4mEUWUSVnJ6zXwOvo4JC2LM0sxNvIoGqmgMEzV5PGUM=; b=je7uj2nxnRnw1xu5DXw+0WMz30Du7AUGiTp3kCdxN/WbEutGQ2jyXP/fvvD8a2WnAW btd67S7Ra3rPD9hFRqFKELt0ThaugnMOR0L4MnHI5zwFxJSIHuOKqL5godAgyu9PfAtv VO9nJABabSzH4RFOwiFvIzqj8vbyOnRA9+eprI49edE9Yrxqoe/OZQOWPplyYhVxJwWn HCipZQRvMlLKriK2OcV2KETsoeyUWFtmUrY2BACt9sSqYglLu6OF185uJPLq3LbLFL+O aTQBIfTTvy+eJ58qWXNBRZsxgcJxhoeEKEYSFaO5b0kpRtekYLr9iboLV1Gwhsls9UbZ D2RQ== MIME-Version: 1.0 X-Received: by 10.229.56.200 with SMTP id z8mr69229405qcg.1.1385581004120; Wed, 27 Nov 2013 11:36:44 -0800 (PST) Sender: adrian.chadd@gmail.com Received: by 10.224.53.200 with HTTP; Wed, 27 Nov 2013 11:36:44 -0800 (PST) Date: Wed, 27 Nov 2013 11:36:44 -0800 X-Google-Sender-Auth: EHAFstJ03Ts9iXCGhgrkajCtdL4 Message-ID: Subject: [review] sendfile / sendfile_sync refactoring From: Adrian Chadd To: "freebsd-arch@freebsd.org" , freebsd-current , Konstantin Belousov , Gleb Smirnoff Content-Type: text/plain; charset=ISO-8859-1 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Nov 2013 19:36:45 -0000 Hi, Here's part #2 in my sendfile refactoring. This is a little more intrusive than the first patch. http://people.freebsd.org/~adrian/netflix/20131126-sendfile-sync-refactor-2.diff My aim here is to move the sendfile_sync stuff out of uipc_syscalls.c and out of sendfile-only code and into something that can be reused elsewhere or replaced later on. I've created methods for all the sendfile_sync stuff, I've moved it out of the do_sendfile() loop so do_sendfile() doesn't specifically implement the completion behaviour. Initially, I'm going to implement a sendfile knote representing the completion of this particular mbuf transaction. I also have a vague, handwav-y idea to use this kind of mbuf transaction representation for later work with aio_write() (and maybe an aio_writev()) when writing to a socket - wire in the userland memory, create a chain of mbufs to represent those, wrap them up in a sendfile_sync (or whatever it mutates into) and then use that for the kqueue completion notification. It needs the same kind of mbuf transaction and kqueue notification mechanism so I'd like to eventually make the sendfile_sync stuff generic, or use the memory buffer representation from jhb, to implement this in a variety of places. I'm not entirely happy where the sendfile_sync stuff is living but this is all meant to be transition-y and enable further hacking on sendfile / variations thereof without having to duplicate too much code. Thanks, -adrian From owner-freebsd-arch@FreeBSD.ORG Thu Nov 28 08:20:06 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 7C41C486; Thu, 28 Nov 2013 08:20:06 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 1C3E71CCF; Thu, 28 Nov 2013 08:20:05 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.7/8.14.7) with ESMTP id rAS8K0Lw025018; Thu, 28 Nov 2013 10:20:00 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.8.3 kib.kiev.ua rAS8K0Lw025018 Received: (from kostik@localhost) by tom.home (8.14.7/8.14.7/Submit) id rAS8K0kJ025017; Thu, 28 Nov 2013 10:20:00 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 28 Nov 2013 10:20:00 +0200 From: Konstantin Belousov To: Adrian Chadd Subject: Re: [review] sendfile / sendfile_sync refactoring Message-ID: <20131128082000.GK59496@kib.kiev.ua> References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="H55dy74Id38Dzf32" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.22 (2013-10-16) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on tom.home Cc: freebsd-current , Gleb Smirnoff , "freebsd-arch@freebsd.org" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Nov 2013 08:20:06 -0000 --H55dy74Id38Dzf32 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 27, 2013 at 11:36:44AM -0800, Adrian Chadd wrote: > Hi, >=20 > Here's part #2 in my sendfile refactoring. This is a little more > intrusive than the first patch. >=20 > http://people.freebsd.org/~adrian/netflix/20131126-sendfile-sync-refactor= -2.diff >=20 sf_buf.h is for sf buffers, and not for sendfile stuff. Please do not pollute this header. The changes you have to make to if_ti.c illustrate my point. The sys/event.h change of adding new kevent type is useless now and=20 has no relation to the rest of the patch. Patch has too many XXX notes for its triviality, some of which are baseless, e.g. the comment for struct sendfile_sync forward declaration in sys/file.h. You extracted all sfs accesses from the sendfile implementation, except the one in sf_buf_mext(). This is weird, please move the code from sf_buf_mext() into static function and put it near sf_sync_ref(). While moving the code into sf_sync_syscall_wait(), please use the opportunity and replace the if (sfs->count !=3D 0) with the while () cv_wait(); loop, to avoid spurious wakeups. I do not see any use for sfs->flags. Also, I do not see any use for passing the flags masked with SF_SYNC, which is always set, to sf_sync_alloc(). If the flags are going to be useful later, it should be added when needed later. The sf_sync_* stuff in the compat32 code just duplicates the main syscall. It should be done in the common code. > My aim here is to move the sendfile_sync stuff out of uipc_syscalls.c > and out of sendfile-only code and into something that can be reused > elsewhere or replaced later on. I've created methods for all the > sendfile_sync stuff, I've moved it out of the do_sendfile() loop so > do_sendfile() doesn't specifically implement the completion behaviour. As is, the patch is sincere nop. Modulo the comments above, I do not object against it. --H55dy74Id38Dzf32 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (FreeBSD) iQIcBAEBAgAGBQJSlvywAAoJEJDCuSvBvK1Bs9gP/iv7zcSWRf51kPbynKkWlM42 XZx4Tg6OnMkykU/G/M5091AyBgtM6NdJcalmtn5suFST5xa42gP0XXhWx7PhRdTQ m+tCqEqs19QNhElDCQ9a/BtemTuEN1ZIg4vYyXpG1qH4mWK+hJr+zJrQNcEMUXDo P3n4t7H5p4z9+20ETIQUGMqyuP0xHlAafvljGTuDzgDfk+y/3LN8Y8bM6VTGDm9B NzCbkP4cvMzuoeGiHOzpv13xd7jiLPJxaDbzUJQNQblWR1j9cWZe/z4AnAeZpm3z n5Pon2SeDxIi3i8p357J3H4UouDEwN52yt7DaXCUTSaRjI1T1ElDmMOnZAkl3YxX f8ZWP+YtJCaS+ZOW3OzV6pIqDVNkLpXuuNrsj2AISNNyz7SxwL7WEB3aRGaksj1f IVEBGs0CDboG0DYO49AFcEJCih1c6FvSvKyzkI6dvyX6L+1di3BnrpIIe7SKxUfb bxl6vxuGuFHSQhGKcb6eT9D6RaDIrQBhHzyvMWaWGFqdITOSH/7uHqk5twYfFBN8 8hd4NVEG8Q3zv6UHjWX+wYf3Vs/NcZ9ayEWjimdSsUqykuerDBUfTWm5t6uuupNp QxiHGI85nEGx2xxiQW22vLn1ESeaP0oY7qWXd3i9GcUP3h0p5uVnpB+ZYPbP+1s9 v991+RluwIIic34q+5I8 =MPo5 -----END PGP SIGNATURE----- --H55dy74Id38Dzf32-- From owner-freebsd-arch@FreeBSD.ORG Thu Nov 28 08:33:09 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id CB40E886; Thu, 28 Nov 2013 08:33:09 +0000 (UTC) Received: from mail-qe0-x229.google.com (mail-qe0-x229.google.com [IPv6:2607:f8b0:400d:c02::229]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 661CF1D8A; Thu, 28 Nov 2013 08:33:09 +0000 (UTC) Received: by mail-qe0-f41.google.com with SMTP id gh4so6251511qeb.0 for ; Thu, 28 Nov 2013 00:33:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=CZB3Yt/dOFXcZK5gTD2hVMe4YLuhFVyzTBRbQlOZMhU=; b=Jx5aAIdGBlW0WlDahoK60Jps8ESnOqZcvqmkxxwFF/ZgSFipgSz58iOpv4JHUgG/3F eZtIdDPqw7W0cH63EOyhqaEFg+OfILDcbZT9JkT2QhOabftHbJVoOi4WOFGSrLYEpUhB X2v04n1NKX9MA+aXzoGQxloTyyNTBWi9XIjk6h9zEr+HVn5sOWd3gC17vl0g67QubKpX dn8w75pUdWDgnA1M0FKc6mUKpshuEOvusMnZ0VAYF4spGC8Rh05snWqGKmNy9DN/1CRE HfDbGSeBtyinWKFNiaOIaOsTQ0H+cfCv9CGOlCc8VI4p50w3MZb2AdOZkEX8/2iMMhVv SVLQ== MIME-Version: 1.0 X-Received: by 10.224.111.197 with SMTP id t5mr74848653qap.49.1385627587884; Thu, 28 Nov 2013 00:33:07 -0800 (PST) Sender: adrian.chadd@gmail.com Received: by 10.224.53.200 with HTTP; Thu, 28 Nov 2013 00:33:07 -0800 (PST) In-Reply-To: <20131128082000.GK59496@kib.kiev.ua> References: <20131128082000.GK59496@kib.kiev.ua> Date: Thu, 28 Nov 2013 00:33:07 -0800 X-Google-Sender-Auth: R-KhTJ2Tg2a8hK0JPoWnR4Q9HCA Message-ID: Subject: Re: [review] sendfile / sendfile_sync refactoring From: Adrian Chadd To: Konstantin Belousov Content-Type: text/plain; charset=ISO-8859-1 Cc: freebsd-current , Gleb Smirnoff , "freebsd-arch@freebsd.org" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Nov 2013 08:33:10 -0000 Hi, It's definitely a work in progress, as you've observed. On 28 November 2013 00:20, Konstantin Belousov wrote: > On Wed, Nov 27, 2013 at 11:36:44AM -0800, Adrian Chadd wrote: >> Hi, >> >> Here's part #2 in my sendfile refactoring. This is a little more >> intrusive than the first patch. >> >> http://people.freebsd.org/~adrian/netflix/20131126-sendfile-sync-refactor-2.diff >> > sf_buf.h is for sf buffers, and not for sendfile stuff. Please do not > pollute this header. The changes you have to make to if_ti.c illustrate > my point. Yup. I don't like having it in here either. I may create a new header file specifically for this. > The sys/event.h change of adding new kevent type is useless now and > has no relation to the rest of the patch. Yeah, ignore that bit. I'm working on adding code for that. > > Patch has too many XXX notes for its triviality, some of which are > baseless, e.g. the comment for struct sendfile_sync forward declaration > in sys/file.h. > > You extracted all sfs accesses from the sendfile implementation, except > the one in sf_buf_mext(). This is weird, please move the code from > sf_buf_mext() into static function and put it near sf_sync_ref(). I plan on doing that soon. > While moving the code into sf_sync_syscall_wait(), please use the > opportunity and replace the if (sfs->count != 0) with the while () > cv_wait(); loop, to avoid spurious wakeups. > > I do not see any use for sfs->flags. Also, I do not see any use > for passing the flags masked with SF_SYNC, which is always set, to > sf_sync_alloc(). If the flags are going to be useful later, it should > be added when needed later. It's just precursor work to add support for other SF_* flags - specifically, a new kqueue notification flag for completion. > The sf_sync_* stuff in the compat32 code just duplicates the main > syscall. It should be done in the common code. The main motivation for moving the sendfile_sync setup and teardown out of do_sendfile() is so do_sendfile() can keep a very little/light idea of the behaviour of sendfile_sync. I don't mind keeping the code in there. Thanks for your feedback. I'll post an updated diff when I've fleshed out some more of this. -a From owner-freebsd-arch@FreeBSD.ORG Sat Nov 30 10:58:54 2013 Return-Path: Delivered-To: freebsd-arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id CC9E3B00 for ; Sat, 30 Nov 2013 10:58:54 +0000 (UTC) Received: from mail120.us2.mcsv.net (mail120.us2.mcsv.net [173.231.139.120]) by mx1.freebsd.org (Postfix) with ESMTP id 7429A19DB for ; Sat, 30 Nov 2013 10:58:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; s=k1; d=mail120.us2.mcsv.net; h=Subject:From:Reply-To:To:Date:Message-ID:List-Unsubscribe:Sender:Content-Type:MIME-Version; i=deepak.gope=3Dtechmagnate.com@mail120.us2.mcsv.net; bh=5fl0HA0R1MZ+wqBwW+H5BZ3UBcQ=; b=yqD2Pv8Z92WmxY6ZbUWqRCjo3l9mg6Xh4RKDRGeNS/vhjdRcMvZIBxWm2J0SdkH5XRdY6k6jVfUU hxpzBz459FvibK57UOvYgIoaE7c55YsTT7CNTiSP57yGJ1TFONadRqWKHaLPd4T7wyNf0a4hQ1Xv cjVzNaFnnjsn6F0WFeg= DomainKey-Signature: a=rsa-sha1; c=nofws; q=dns; s=k1; d=mail120.us2.mcsv.net; b=xIkrr5QgsXjzDI6oNCwA0Jo/RHKbQrp4mTXZP5RAT7Q2c0egPLxygBboSH7u25R8hrUUA2awWySM 7Ours6uMc0qZsbv1WHGDIQ3+MD/SK2X+AunxtiCGiy1/oSdjdSEi/uuDn9FufbAFPx5vTokihkNt jm1A2b3KANgpHdWT9kM=; Received: from (127.0.0.1) by mail120.us2.mcsv.net id hj72es174gsr for ; Sat, 30 Nov 2013 10:57:53 +0000 (envelope-from ) Subject: =?utf-8?Q?Urgent=20Request=20=2D=20Removal=20of=20Backlink=2Fs=20to=20Comply=20with=20Penguin=20Update=20=2D=20ACNC.com?= From: =?utf-8?Q?Deepak=20Gope?= To: Date: Sat, 30 Nov 2013 10:57:53 +0000 Message-ID: <2a9dcd6c9efe14b991773507b83ef1b86ff.20131130105743@mail120.us2.mcsv.net> X-Mailer: MailChimp Mailer - **CIDa9adef2c3183ef1b86ff** X-Campaign: mailchimp2a9dcd6c9efe14b991773507b.a9adef2c31 X-campaignid: mailchimp2a9dcd6c9efe14b991773507b.a9adef2c31 X-Report-Abuse: Please report abuse for this campaign here: http://www.mailchimp.com/abuse/abuse.phtml?u=2a9dcd6c9efe14b991773507b&id=a9adef2c31&e=83ef1b86ff X-MC-User: 2a9dcd6c9efe14b991773507b x-accounttype: ff Sender: "Deepak Gope" x-mcda: FALSE MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format="fixed" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.16 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list Reply-To: =?utf-8?Q?Deepak=20Gope?= List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 30 Nov 2013 10:58:54 -0000 Dear Webmaster=2C Greetings from Deepak Gope=2C SEO Project Manager for http://cubedynamic.u= s3.list-manage1.com/track/click?u=3D2a9dcd6c9efe14b991773507b&id=3Da8cb3bf= e02&e=3D83ef1b86ff As you might be aware=2C Google's Penguin update to its ranking algorithm= was rolled out on 4th Oct=2C 2013. In order to safeguard http://cubedynam= ic.us3.list-manage2.com/track/click?u=3D2a9dcd6c9efe14b991773507b&id=3De26= c199d86&e=3D83ef1b86ff from any future updates we are cleaning up ou= r back links profile to comply with Google's Guidelines. In pursuit of thi= s objective=2C we believe that backlinks existing on your domain = anoncvs.heanet.ie should be removed. Please remove all links from your domain to our= s by 3rd Dec=2C 2013. If you need more details on the links to be removed from your domain pleas= e revert to us immediately. If above links are not removed by 3rd Dec=2C we will be constrained to dis= avow them via Google Webmaster Tools. Notice that your proactive action as= per this email is in your best interest. Google disavowal requests are NO= T good for reputation of your domain. Thanks and Best regards Deepak Gope =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Unsubscribe freebsd-arch@FreeBSD.org from this list: http://cubedynamic.us3.list-manage.com/unsubscribe?u=3D2a9dcd6c9efe14b9917= 73507b&id=3D2795d43ef9&e=3D83ef1b86ff&c=3Da9adef2c31 From owner-freebsd-arch@FreeBSD.ORG Sat Nov 30 22:36:25 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 1FE52138; Sat, 30 Nov 2013 22:36:25 +0000 (UTC) Received: from smtpauth2.wiscmail.wisc.edu (wmauth2.doit.wisc.edu [144.92.197.222]) (using TLSv1 with cipher RC4-MD5 (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id D47DB1E67; Sat, 30 Nov 2013 22:36:24 +0000 (UTC) MIME-version: 1.0 Content-type: multipart/mixed; boundary="Boundary_(ID_ftISHxvL5H+AYDZeUZXaaA)" Received: from avs-daemon.smtpauth2.wiscmail.wisc.edu by smtpauth2.wiscmail.wisc.edu (Oracle Communications Messaging Server 7u4-27.01(7.0.4.27.0) 64bit (built Aug 30 2012)) id <0MX300K00LFUN700@smtpauth2.wiscmail.wisc.edu>; Sat, 30 Nov 2013 16:36:22 -0600 (CST) X-Spam-PmxInfo: Server=avs-2, Version=6.0.3.2322014, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2013.11.30.222715, SenderIP=0.0.0.0 X-Spam-Report: AuthenticatedSender=yes, SenderIP=0.0.0.0 Received: from wanderer.tachypleus.net (adsl-76-208-69-44.dsl.mdsnwi.sbcglobal.net [76.208.69.44]) by smtpauth2.wiscmail.wisc.edu (Oracle Communications Messaging Server 7u4-27.01(7.0.4.27.0) 64bit (built Aug 30 2012)) with ESMTPSA id <0MX300AGBLGIEM00@smtpauth2.wiscmail.wisc.edu>; Sat, 30 Nov 2013 16:36:22 -0600 (CST) Date: Sat, 30 Nov 2013 16:36:18 -0600 From: Nathan Whitehorn Subject: Re: [CFT] bsdinstall and zfsboot enhancements In-reply-to: To: Devin Teske Message-id: <529A6862.7060308@freebsd.org> X-Enigmail-Version: 1.5.2 References: <5275C597.6070702@freebsd.org> <97944047-D575-4E2E-B687-9871DFE058E3@fisglobal.com> <52769CFE.5080707@freebsd.org> <5281340E.8080009@callfortesting.org> <52813E53.20403@freebsd.org> <5281441E.7060806@freebsd.org> User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.0 Cc: "freebsd-arch@freebsd.org" , Peter Grehan , "Teske, Devin" , Current Current , Michael Dexter X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 30 Nov 2013 22:36:25 -0000 This is a multi-part message in MIME format. --Boundary_(ID_ftISHxvL5H+AYDZeUZXaaA) Content-type: text/plain; CHARSET=US-ASCII Content-transfer-encoding: 7BIT On 11/11/13 14:57, Teske, Devin wrote: > On Nov 11, 2013, at 12:54 PM, Nathan Whitehorn wrote: > >> On 11/11/13 14:30, Nathan Whitehorn wrote: >>> On 11/11/13 14:18, Teske, Devin wrote: >>>> -----BEGIN PGP SIGNED MESSAGE----- >>>> Hash: SHA512 >>>> >>>> >>>> On Nov 11, 2013, at 11:46 AM, Michael Dexter wrote: >>>> >>>> >>>> Hello all, >>>> >>>> I have been experimenting with various BSD and GNU/Linux boot media >>>> under bhyve and noticed that we may want to accommodate the "LiveCD" >>>> mode of the installer, which in turn requires the correct console. >>>> >>>> Currently, one is prompted for VT100 for installation but this does not >>>> appear to work/stick for LiveCD mode. >>>> >>>> Can anyone verify this? >>>> >>>> >>>> While I developed this patch... >>>> https://urldefense.proofpoint.com/v1/url?u=http://druidbsd.cvs.sf.net/viewvc/druidbsd/bsdinstall_zfs/usr.sbin%253A%253Absdinstall%253A%253Ascripts%253A%253Aconfig.patch?revision%3D1.10%26view%3Dmarkup&k=%2FbkpAUdJWZuiTILCq%2FFnQg%3D%3D%0A&r=Mrjs6vR4%2Faj2Ns9%2FssHJjg%3D%3D%0A&m=aZg%2BxwqLTX6Zcpf3Rc44iPtAQhFrpqoS4FC5B8ykxJQ%3D%0A&s=6d54d337ea5472f5713ddbe7788d3248c45feefd4b291eab0a976c39e9d40428 >>>> >>>> Reasons exist to search for a better solution, see here: >>>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freebsd.org/pipermail/freebsd-current/2013-November/046148.html&k=%2FbkpAUdJWZuiTILCq%2FFnQg%3D%3D%0A&r=Mrjs6vR4%2Faj2Ns9%2FssHJjg%3D%3D%0A&m=aZg%2BxwqLTX6Zcpf3Rc44iPtAQhFrpqoS4FC5B8ykxJQ%3D%0A&s=5f8128901747346f937ffc4478eadb4bc39059504258dfb9b36f0fb9e7a61b79 >>>> (and messages that follow it) >>>> >>>> Is modifying init(8) still the way to go? What modification do we want to make? >>>> I'll do the work if we can come to consensus. >>>> >>>> Or should we touch up the patch in some way to address the original concerns? >>>> >>> I think modifying init is the way to go -- it keeps the install system from interfering with the installed one, as well as fixing this kind of issue with moved hard drives or PXE booting or what have you. If we can provide a guarantee that any system that displays text has a working console (unless explicitly configured not to), useability is improved. >>> >>> I would propose one of the following (and volunteer to write the code): >>> >>> Option A >>> ------------ >>> >>> 1. init checks if there is an entry in /etc/ttys for the terminal[s] corresponding to the value[s] in kern.console >>> 2. If an entry for each console terminal exists in /etc/ttys, enable it >>> 3. If not, invent one with a terminal type of "ansi" >>> >>> The one issue here is that someone may want to force a particular entry to off and still have it be the kernel console. This is tricky. We could invent a new "status" field that is not "on" or "off" ("auto", maybe, or "ifconsole"?). Which brings us to: >> One easy way to accomplish this is just to only implement (1) and (3), then comment out the ttyu0 entry in /etc/ttys on x86 instead of marking it "off". Then the behavior is just that a tty marked "off" stays off, one marked "on" stays on, and one not present spawns login with a terminal type corresponding to "console" (by default "unknown") if it happens to be the console. I will implement this over the next few days and then send patches unless anyone has an objection. > I love it. (smiles) > > Excellent idea and that's the winner I think. > > +1 This took much longer than I'd anticipated, but the patch to init is attached. I chose not to make the changes to init rather than getttyent() and friends in libc, which I am open to revisiting. The behavior changes are as follows: If the "console" device in /etc/ttys in marked "on", instead of opening /dev/console, init will loop through the active kernel console devices, and for each will: 1. If the kernel console device is in /etc/ttys and marked "on", it already has a terminal and will be ignored. 2. If marked "off", that is an explicit statement that a console is not wanted and so it will be ignored. 3. If not present in /etc/ttys, init will run getty with whatever parameters "console" has. (3) is the main behavioral change. No changes in behavior will occur if /etc/ttys is not modified. If we turn on "console" by default, it will usually have no effect instead of trying to run multiple gettys, which is new. If we then also comment out the ttyu0 line, instead of marking it "off", the result will be the conditional presence of a login prompt on the first serial port depending on whether it is an active console device for the kernel. I believe this is the behavior we are going for. Comments and test results would be appreciated. -Nathan --Boundary_(ID_ftISHxvL5H+AYDZeUZXaaA) Content-type: text/plain; CHARSET=US-ASCII; name=init-ttys.diff Content-transfer-encoding: 7BIT Content-disposition: attachment; filename=init-ttys.diff Index: init.c =================================================================== --- init.c (revision 258756) +++ init.c (working copy) @@ -1102,22 +1102,84 @@ } /* + * Do something for all defined TTYs + */ +static void +do_allttys(void (*callback)(struct ttyent *, void *), void *cookie) +{ + struct ttyent *typ; + char *buf, *cons, *nextcons; + size_t len; + + /* Loop through /etc/ttys */ + while ((typ = getttyent()) != NULL) { + if (strcmp(typ->ty_name, "console") == 0) + continue; + + callback(typ, cookie); + } + + /* Mix in any active console devices not in /etc/ttys */ + buf = NULL; + if (sysctlbyname("kern.console", NULL, &len, NULL, 0) == -1) + goto done; + buf = malloc(len); + if (sysctlbyname("kern.console", buf, &len, NULL, 0) == -1) + goto done; + + if ((cons = strchr(buf, '/')) == NULL) + goto done; + *cons = '\0'; + nextcons = buf; + while ((cons = strsep(&nextcons, ",")) != NULL && strlen(cons) != 0) { + typ = getttynam(cons); + if (typ != NULL) /* Skip terminals with known configs */ + continue; + typ = getttynam("console"); + typ->ty_name = cons; + + callback(typ, cookie); + } + +done: + if (buf != NULL) + free(buf); + endttyent(); +} + +/* * Walk the list of ttys and create sessions for each active line. */ +struct read_ttys_args { + int session_index; + session_t *sp; +}; + +static void +read_ttys_callback(struct ttyent *typ, void *cookie) +{ + struct read_ttys_args *args = cookie; + session_t *snext; + + if ((snext = new_session(args->sp, ++args->session_index, typ)) != NULL) + args->sp = snext; +} + static state_func_t read_ttys(void) { - int session_index = 0; - session_t *sp, *snext; - struct ttyent *typ; + struct read_ttys_args args; + session_t *snext; + args.session_index = 0; + /* * Destroy any previous session state. * There shouldn't be any, but just in case... */ - for (sp = sessions; sp; sp = snext) { - snext = sp->se_next; - free_session(sp); + for (args.sp = sessions; args.sp; args.sp = snext) { + snext = args.sp->se_next; + free_session(args.sp); } sessions = 0; if (start_session_db()) @@ -1127,12 +1189,8 @@ * Allocate a session entry for each active port. * Note that sp starts at 0. */ - while ((typ = getttyent()) != NULL) - if ((snext = new_session(sp, ++session_index, typ)) != NULL) - sp = snext; + do_allttys(read_ttys_callback, &args); - endttyent(); - return (state_func_t) multi_user; } @@ -1375,85 +1433,93 @@ /* * This is an (n*2)+(n^2) algorithm. We hope it isn't run often... */ -static state_func_t -clean_ttys(void) +static void +clean_ttys_callback(struct ttyent *typ, void *cookie) { + int *session_index = cookie; session_t *sp, *sprev; - struct ttyent *typ; - int session_index = 0; int devlen; char *old_getty, *old_window, *old_type; - /* - * mark all sessions for death, (!SE_PRESENT) - * as we find or create new ones they'll be marked as keepers, - * we'll later nuke all the ones not found in /etc/ttys - */ - for (sp = sessions; sp != NULL; sp = sp->se_next) - sp->se_flags &= ~SE_PRESENT; - devlen = sizeof(_PATH_DEV) - 1; - while ((typ = getttyent()) != NULL) { - ++session_index; + ++(*session_index); - for (sprev = 0, sp = sessions; sp; sprev = sp, sp = sp->se_next) - if (strcmp(typ->ty_name, sp->se_device + devlen) == 0) - break; + for (sprev = 0, sp = sessions; sp; sprev = sp, sp = sp->se_next) + if (strcmp(typ->ty_name, sp->se_device + devlen) == 0) + break; - if (sp) { - /* we want this one to live */ - sp->se_flags |= SE_PRESENT; - if (sp->se_index != session_index) { - warning("port %s changed utmp index from %d to %d", - sp->se_device, sp->se_index, - session_index); - sp->se_index = session_index; - } - if ((typ->ty_status & TTY_ON) == 0 || - typ->ty_getty == 0) { - sp->se_flags |= SE_SHUTDOWN; - kill(sp->se_process, SIGHUP); - continue; - } - sp->se_flags &= ~SE_SHUTDOWN; - old_getty = sp->se_getty ? strdup(sp->se_getty) : 0; - old_window = sp->se_window ? strdup(sp->se_window) : 0; - old_type = sp->se_type ? strdup(sp->se_type) : 0; - if (setupargv(sp, typ) == 0) { - warning("can't parse getty for port %s", - sp->se_device); - sp->se_flags |= SE_SHUTDOWN; - kill(sp->se_process, SIGHUP); - } - else if ( !old_getty - || (!old_type && sp->se_type) - || (old_type && !sp->se_type) - || (!old_window && sp->se_window) - || (old_window && !sp->se_window) - || (strcmp(old_getty, sp->se_getty) != 0) - || (old_window && strcmp(old_window, sp->se_window) != 0) - || (old_type && strcmp(old_type, sp->se_type) != 0) - ) { + if (sp) { + /* we want this one to live */ + sp->se_flags |= SE_PRESENT; + if (sp->se_index != *session_index) { + warning("port %s changed utmp index from %d to %d", + sp->se_device, sp->se_index, + *session_index); + sp->se_index = *session_index; + } + if ((typ->ty_status & TTY_ON) == 0 || + typ->ty_getty == 0) { + sp->se_flags |= SE_SHUTDOWN; + kill(sp->se_process, SIGHUP); + return; + } + sp->se_flags &= ~SE_SHUTDOWN; + old_getty = sp->se_getty ? strdup(sp->se_getty) : 0; + old_window = sp->se_window ? strdup(sp->se_window) : 0; + old_type = sp->se_type ? strdup(sp->se_type) : 0; + if (setupargv(sp, typ) == 0) { + warning("can't parse getty for port %s", + sp->se_device); + sp->se_flags |= SE_SHUTDOWN; + kill(sp->se_process, SIGHUP); + } + else if ( !old_getty + || (!old_type && sp->se_type) + || (old_type && !sp->se_type) + || (!old_window && sp->se_window) + || (old_window && !sp->se_window) + || (strcmp(old_getty, sp->se_getty) != 0) + || (old_window && strcmp(old_window, sp->se_window) != 0) + || (old_type && strcmp(old_type, sp->se_type) != 0) + ) { /* Don't set SE_SHUTDOWN here */ sp->se_nspace = 0; sp->se_started = 0; kill(sp->se_process, SIGHUP); - } - if (old_getty) - free(old_getty); - if (old_window) - free(old_window); - if (old_type) - free(old_type); - continue; } - - new_session(sprev, session_index, typ); + if (old_getty) + free(old_getty); + if (old_window) + free(old_window); + if (old_type) + free(old_type); + return; } - endttyent(); + new_session(sprev, *session_index, typ); +} +static state_func_t +clean_ttys(void) +{ + session_t *sp; + int session_index = 0; + /* + * mark all sessions for death, (!SE_PRESENT) + * as we find or create new ones they'll be marked as keepers, + * we'll later nuke all the ones not found in /etc/ttys + */ + for (sp = sessions; sp != NULL; sp = sp->se_next) + sp->se_flags &= ~SE_PRESENT; + + /* + * Loop over all TTYs and sync state + */ + do_allttys(clean_ttys_callback, &session_index); + + + /* * sweep through and kill all deleted sessions * ones who's /etc/ttys line was deleted (SE_PRESENT unset) */ --Boundary_(ID_ftISHxvL5H+AYDZeUZXaaA)--