From owner-svn-src-head@FreeBSD.ORG Fri Dec 14 04:49:47 2012 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 23C56C9; Fri, 14 Dec 2012 04:49:47 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx08.syd.optusnet.com.au (fallbackmx08.syd.optusnet.com.au [211.29.132.10]) by mx1.freebsd.org (Postfix) with ESMTP id A69AA8FC08; Fri, 14 Dec 2012 04:49:46 +0000 (UTC) Received: from mail17.syd.optusnet.com.au (mail17.syd.optusnet.com.au [211.29.132.198]) by fallbackmx08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qBE4njAY015689; Fri, 14 Dec 2012 15:49:45 +1100 Received: from c122-106-175-26.carlnfd1.nsw.optusnet.com.au (c122-106-175-26.carlnfd1.nsw.optusnet.com.au [122.106.175.26]) by mail17.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qBE4nZEB020208 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 14 Dec 2012 15:49:37 +1100 Date: Fri, 14 Dec 2012 15:49:35 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Jim Harris Subject: Re: svn commit: r244193 - head/sys/x86/include In-Reply-To: <201212132140.qBDLeBhd019751@svn.freebsd.org> Message-ID: <20121214154930.D973@besplex.bde.org> References: <201212132140.qBDLeBhd019751@svn.freebsd.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.0 cv=CJhyxmXD c=1 sm=1 a=L6I1Zbe1xPYA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=RQfEP2Kgk-UA:10 a=DA7H5E3C-HH1s4Do58cA:9 a=CjuIK1q_8ugA:10 a=bxQHXO5Py4tHmhUgaywp5w==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Dec 2012 04:49:47 -0000 On Thu, 13 Dec 2012, Jim Harris wrote: > Log: > Add bus_space_read_8 and bus_space_write_8 for amd64. > > Rather than trying to KASSERT for callers that invoke this on > IO tags, either do nothing (for write_8) or return ~0 (for read_8). read_8 returns a uint64_t, so it cannot return the signed integer ~0. It actually returns this signed integer converted to uint64_t. On amd64, this is the 32 bit value 0xffffffff. The 64-bit value 0xffffffffffffffff should be returned. > Using KASSERT here just makes bus.h too messy from both > polluting bus.h with systm.h (for any number of drivers that include > bus.h without first including systm.h) or ports that use bus.h > directly (i.e. libpciaccess) as reported by zeising@. > > Also don't try to implement all of the other bus_space functions for > 8 byte access since realistically only these two are needed for some > devices that expose 64-bit memory-mapped registers. Good. > Put the amd64-specific functions here rather than sys/amd64/include/bus.h > so that we can keep this header unified for x86, as requested by mdf@ > and tijl@. Not so good. I don't really like any of the unified headers. > Modified: head/sys/x86/include/bus.h > ============================================================================== > --- head/sys/x86/include/bus.h Thu Dec 13 21:39:59 2012 (r244192) > +++ head/sys/x86/include/bus.h Thu Dec 13 21:40:11 2012 (r244193) > @@ -130,6 +130,7 @@ > #define BUS_SPACE_MAXADDR 0xFFFFFFFF > #endif This file spells the F in hex constants in upper case. In the above definition and in previous ones, it is careful to spell out the constants and not depend on sign extension. So it is also a style bug to use ~0. Style bug visible in the above: space instead of tab after #define. This style bugs is in all #define's near here, including the new one. Type error in #define's just before the above: the above BUS_SPACE_MAXADDR is for 32 bits. For amd64 and i386-PAE, BUS_SPACE_MAXADDR is of course 64 bits, but the ifdef tangle for it is not tangled enough to be correct: BUS_SPACE_MAXADDR is 0xFFFFFFFFFFFFFFFFULL, on both, but bus_addr_t is only unsigned long long on i386-PAE. > > +#define BUS_SPACE_INVALID_DATA (~0) > #define BUS_SPACE_UNRESTRICTED (~0) > > /* > @@ -221,6 +222,12 @@ static __inline u_int32_t bus_space_read > bus_space_handle_t handle, > bus_size_t offset); > > +#ifdef __amd64__ > +static __inline uint64_t bus_space_read_8(bus_space_tag_t tag, > + bus_space_handle_t handle, > + bus_size_t offset); > +#endif > + This is style-bug for bug compatible with the existing forward declarations. Forward declarations of inline functions are nonsense. They are from NetBSD, for K&R support. But FreeBSD never supported K&R in bus-space headers, and the forward declarations never even compiled with K&R, since they never used __P(()). Almost 1/3 of the x86 bus.h consists of these negatively useful forward declarations. Some of them are almost as large as the full functions, since they are misformatted worse, with parameters starting at about column 40 instead of about column 20, so so that many lines are needed just for the parameters (to line them up in perfectly non-KNF gnu style). > ... > @@ -251,8 +258,16 @@ bus_space_read_4(bus_space_tag_t tag, bu > return (*(volatile u_int32_t *)(handle + offset)); > } > > -#if 0 /* Cause a link error for bus_space_read_8 */ > -#define bus_space_read_8(t, h, o) !!! bus_space_read_8 unimplemented !!! > +#ifdef __amd64__ > +static __inline uint64_t > +bus_space_read_8(bus_space_tag_t tag, bus_space_handle_t handle, > + bus_size_t offset) > +{ > + > + if (tag == X86_BUS_SPACE_IO) /* No 8 byte IO space access on x86 */ The comment is not indented, and should probably not be placed to the right of the code. This file mostly doesn't place comments there, and when it does it doesn't capitalize them. One that does also spells IO as i/o. > + return (BUS_SPACE_INVALID_DATA); > + return (*(volatile uint64_t *)(handle + offset)); > +} > #endif Bruce