From owner-svn-src-head@FreeBSD.ORG Fri Dec 14 22:50:05 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 268AD995; Fri, 14 Dec 2012 22:50:05 +0000 (UTC) (envelope-from carl.r.delsey@intel.com) Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by mx1.freebsd.org (Postfix) with ESMTP id BE3058FC14; Fri, 14 Dec 2012 22:50:04 +0000 (UTC) Received: from azsmga002.ch.intel.com ([10.2.17.35]) by azsmga101.ch.intel.com with ESMTP; 14 Dec 2012 14:49:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.84,284,1355126400"; d="scan'208";a="181102956" Received: from crdelsey-fbsd.ch.intel.com (HELO [10.2.105.127]) ([10.2.105.127]) by AZSMGA002.ch.intel.com with ESMTP; 14 Dec 2012 14:49:50 -0800 Message-ID: <50CBAD0E.5050907@intel.com> Date: Fri, 14 Dec 2012 15:49:50 -0700 From: Carl Delsey User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:13.0) Gecko/20120724 Thunderbird/13.0.1 MIME-Version: 1.0 To: Bruce Evans Subject: Re: svn commit: r244193 - head/sys/x86/include References: <201212132140.qBDLeBhd019751@svn.freebsd.org> <20121214154930.D973@besplex.bde.org> In-Reply-To: <20121214154930.D973@besplex.bde.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: svn-src-head@FreeBSD.org, Jim Harris , 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 22:50:05 -0000 On 12/13/12 21:49, Bruce Evans wrote: > 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. I double checked and 0xffffffffffffffff is being returned when compiled by both clang and gcc. > >> 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. Me neither, but it sounds like there is a good reason for it, which I'll admit I don't fully understand. Sounds like it has something to do with cross-compiling. > >> 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. Are you saying it is a style bug to not match the style used above, regardless of whether that style is right or wrong, or are you saying (~0) is always a style bug? > > 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. I think this should be a separate patch though, since it is unrelated to this change. > >> >> +#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). Same with this - separate patch > >> ... >> @@ -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. All the #if 0 lines also start with an end of line comment, and they are all capitalized. By "not indented" are you saying that all end of line comments must be preceded by a tab? > >> + return (BUS_SPACE_INVALID_DATA); >> + return (*(volatile uint64_t *)(handle + offset)); >> +} >> #endif > > Bruce > _______________________________________________ > 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"