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