Date: Sat, 15 Dec 2012 21:56:24 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Carl Delsey <carl.r.delsey@intel.com> Cc: svn-src-head@freebsd.org, Jim Harris <jimharris@freebsd.org>, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r244193 - head/sys/x86/include Message-ID: <20121215212259.F2058@besplex.bde.org> In-Reply-To: <50CBAD0E.5050907@intel.com> References: <201212132140.qBDLeBhd019751@svn.freebsd.org> <20121214154930.D973@besplex.bde.org> <50CBAD0E.5050907@intel.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 14 Dec 2012, Carl Delsey wrote:
> 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.
Oops. ~0 is (int)-1 (assuming 2's complement. This gets converted to
(uint64_t)-1 = 0xfff...fffUL.
I still prefer the explicit hex constant. Higher-level bus.h APIs and
clients mostly use ~0ul (with that spelling) for this. This is the
documented spelling in bus_alloc_resource(9). It is shorter and
more portable than 0xfff...fffUL where the number of f's is MD. The
ul in it avoids the sign extension problems that I was worried about
here. ~0 is unportable (depends on 2's complement). ~0u is portable
but even harder to use than ~0 in practice. It is what gives the
32-bit value unless it is actually ~0ul.
I wonder how these ~0ul's work for PAE -- on i386, they only give
32-bit values, but bus addresses are 64 bits for PAE. I guess they
just don't work for PAE. Nothing at all in the MI sys/bus.h supports
PAE's 64-bit bus addresses, since sys/bus.h uses u_long for almost
everything (int never uses [u]int64_t, [u]_quad_t or [unsigned] long
long).
>>> 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?
You might as well match the above style.
~0 is at worst a style bug, not a wrong value like I said. It is harder to
use since its correctness depends on the arch being 2's complement, and
understanding its correctness depends on knowing C's promotion rules and
not forgetting them like I did.
I also don't mind using (~0ul) for most of the values, to be consistent
with the MI spelling. This might reduce ifdefs. But there is only 1 ifdef
for a 64-bit value now, and 1 would still be needed for the PAE case if
64 bits actually work for PAE.
>> 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.
Fine.
>>> +#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
Fine.
>>> -#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?
#if 0 lines tend to be sloppier. I would put the comments for them on
separate lines.
End of line comments should be preceded by enough tabs to reach the normal
indentation level for such comments. This is 32 or 40 columns (indent -ci
33 or -ci 41). Verbose code may need more. Then it is impossible to use
the normal indentation level. I use 2 spaces then, but prefer to avoid
such code and comments to the right of any code (within functions).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121215212259.F2058>