Date: Fri, 14 Dec 2012 15:49:50 -0700 From: Carl Delsey <carl.r.delsey@intel.com> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, Jim Harris <jimharris@FreeBSD.org>, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r244193 - head/sys/x86/include Message-ID: <50CBAD0E.5050907@intel.com> In-Reply-To: <20121214154930.D973@besplex.bde.org> References: <201212132140.qBDLeBhd019751@svn.freebsd.org> <20121214154930.D973@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?50CBAD0E.5050907>