Skip site navigation (1)Skip section navigation (2)
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>