Date: Wed, 28 Dec 2011 11:08:26 +0300 From: Sergey Kandaurov <pluknet@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, Sean Bruno <seanbru@yahoo-inc.com> Subject: Re: i386 compile sys/dev/ie Message-ID: <CAE-mSOKcdaWtZAR0XT2tikyXFbjqBJWwrEo95vhyWM6MkLPuXw@mail.gmail.com> In-Reply-To: <20111228164938.F936@besplex.bde.org> References: <1325015120.17645.7.camel@hitfishpass-lx.corp.yahoo.com> <CAE-mSOLDrPLkVbM8i-1q=wzMdF_Kz1FJNJqtW-4tnC0_VWvrKA@mail.gmail.com> <20111228162621.G936@besplex.bde.org> <20111228164938.F936@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 28 December 2011 10:58, Bruce Evans <brde@optusnet.com.au> wrote:
> On Wed, 28 Dec 2011, Bruce Evans wrote:
>
>> On Wed, 28 Dec 2011, Sergey Kandaurov wrote:
>
>
>>> These were used in probe routine and are left from the newbus rewrite.
>>> I hacked ie a bit to build cleanly. [Not sure if I did this correctly.]
>>
>>
>> Use of the __DEVOLATILE() abomination is never correct. =A0It exploits t=
he
>> bug that -Wcast-qual is broken for casts through [u]intptr_t.
Agreed.
> PS: I used to maintain some bad fixes in this area for ie, but now
> have only the following:
>
> % .Index: if_ie.c
> % .=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> % .RCS file: /home/ncvs/src/sys/dev/ie/if_ie.c,v
> % .retrieving revision 1.99
> % .diff -u -2 -r1.99 if_ie.c
> % .--- if_ie.c =A017 Mar 2004 17:50:35 -0000 =A0 =A0 =A01.99
> % .+++ if_ie.c =A031 May 2004 06:57:05 -0000
> % .@@ -159,4 +159,7 @@
> % . #define IE_BUF_LEN =A0ETHER_MAX_LEN =A0 /* length of transmit buffer =
*/
> % . % .+/* XXX this driver uses `volatile' and `caddr_t' to a fault. */
> % .+typedef =A0 =A0 volatile char *v_caddr_t; =A0 =A0 =A0 /* core address=
, pointer to
> volatile */
> % .+
> % . /* Forward declaration */
> % . struct ie_softc;
Perhaps, it should be finally committed. :)
> I never allowed the __DEFOO() abominations in my <sys/cdefs.h>, and at
> one time had all uses to them in /usr/src removed and ready to commit
> (there were about 2 of them. =A0There are hundreds if not thousands now :=
-().
> c_caddr_t and v_caddr_t are as abominable as __DEFOO(), so of course I
> never allowed them in my <sys/types.h>. =A0But v_caddr_t is still easy to
> remove, since it is only used in the ie driver. =A0It is used the break
> warnings from -Wcast-qual that more natural casts would give (but warning=
s
> still result from the implicit conversion for bcopy()):
>
> % if_ie.c:static v_caddr_t setup_rfa =A0 =A0 =A0 =A0 =A0 =A0(struct ie_so=
ftc *,
> v_caddr_t);
>
> The very idea of a v_caddr_t is nonsense. =A0caddr_t is a bad old type fo=
r
> a "core address". =A0In most cases, it means the same as "void *", but wa=
s
> used because "void *" didn't exist. =A0In FreeBSD, it must be precisely
> "char *" to work, since pointer arithmetic is perpetrated on it (the
> pointer arithmetic defeats its opaqueness). =A0In a few cases, it is
> used for physical or device memory accesses. =A0In must places, it should
> be spelled "void *", and in other places it should be replaced by
> a vm virtual or physical address type, or a bus space type.
>
> To this mess, the ie driver, but mercifully no other code in /usr/src,
> adds v_caddr_t. =A0caddr_t is supposed to be opaque, but if we just cast
> to that we will get a cast-qual error if we start with one of ie's
> excessively qualified pointers. =A0Also, "const caddr_t" doesn't work
> since it puts the const in the wrong place. =A0This is "solved" by lookin=
g
> inside caddr_t to add the const there.
>
> ie uses v_caddr_t as follows:
>
> First, in the above, some of the excessive qualifiers are in setup_rfa()'=
s
> return type and second parameter...
>
> % if_ie.c: =A0 =A0 =A0setup_rfa(sc, (v_caddr_t) sc->rframes[0]);
> % if_ie.c: =A0 =A0 =A0setup_rfa(sc, (v_caddr_t) sc->rframes[0]); =A0 =A0 =
=A0/* ignore
> cast-qual */
>
> ... this makes even calling setup_rfa() difficult. =A0We start with a
> volatile struct ie_mumble **rframes. =A0We can't just cast this to void *
> or caddr_t since this would cause a -Wcast-qual warning. =A0Casting it
> to "volatile void *" would be even worse, since it removes a volatile
> qualifier that is in the (technically) right place and adds it back in
> a wrong place. =A0So we use v_caddr_t to keep it in the same place.
I should say for above that initially sc->rframes was:
volatile struct ie_recv_frame_desc *rframes[MXFRAMES];
as well as its friends rbuffs, cbuffs, etc.
Then it was changed to the current volatile ** form, and MXFRAMES
was replaced to some dynamic value calculated from rman, so that
gives even less chances to fix the ie code properly.
/*
* based on the amount of memory we have, allocate our tx and rx
* resources.
*/
factor =3D rman_get_size(sc->mem_res) / 8192;
sc->nframes =3D factor * NFRAMES;
sc->nrxbufs =3D factor * NRXBUFS;
sc->ntxbufs =3D factor * NTXBUFS;
/*
* Since all of these guys are arrays of pointers, allocate as one
* big chunk and dole out accordingly.
*/
allocsize =3D sizeof(void *) * (sc->nframes
+ (sc->nrxbufs * 2)
+ (sc->ntxbufs * 3));
sc->rframes =3D (volatile struct ie_recv_frame_desc **) malloc(allo=
csize,
M_DEVB=
UF,
M_NOWAIT=
);
then all these numerous buffs volatile friends point somewhere to this
memory by setting its pointer to rframes like.
sc->rbuffs =3D
(volatile struct ie_recv_buf_desc **)&sc->rframes[sc->nframes];
This prompts me to that the volatile qualifier is probably abused for many
sc fields there.
> % if_ie.c: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcopy((v_caddr_t) (=
sc->cbuffs[head] +
> offset),
> % if_ie.c: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcopy((v_caddr_t) (=
sc->cbuffs[head] +
> offset),
> % if_ie.c: =A0 =A0 =A0 =A0 =A0 =A0 =A0bcopy((v_caddr_t) (sc->cbuffs[head]=
+ offset),
>
> Similarly. =A0cbuffs is a volatile u_char **. =A0We want to aplply bcopy(=
)
> to (cbuffs[head] + offset), which is a volatile u_char *. =A0We assume th=
at
> bcopy() is still the 1980's version which takes a caddr_t, although it
> took a (void *) even in 1992. =A0So we originally tried to bogusly cast t=
he
> arg to caddr_t. =A0This had no effect, as would the more correct cast to
> (void *) have done, since the prototype converts to void * anyway.
> Eventually, -Wcast-qual was used and this bug was detected. =A0(There may=
be
> a more serious bug involving char * being incompatble with void *. =A0A
> grandfather kludge keeps char * equivalent to void * in many context.
> But it is not clear what happens with complicated qualifiers.) =A0Some
> time later, the -Wcast-qual was broken using a volatile cast. =A0But
> the warning for converting away the qualifier by the prototype
> remained. =A0Omitting the cast would have had the same effect.
>
> % if_ie.c: =A0 =A0 =A0bcopy((v_caddr_t) (sc->rframes[num]), &rfd,
>
> Similarly.
>
> % if_ie.c:static v_caddr_t
> % if_ie.c:setup_rfa(struct ie_softc *sc, v_caddr_t ptr)
>
> The definition matching the above prototype.
>
> % if_ie.c: =A0 =A0 =A0bcopy((v_caddr_t) sc->mcast_addrs, (v_caddr_t)
> cmd->ie_mcast_addrs,
>
> Similarly.
>
> % if_ie.c: =A0 =A0 =A0 =A0 =A0 =A0 =A0bzero((v_caddr_t) sc->xmit_cmds[i],=
sizeof
> *sc->xmit_cmds[i]);
> % if_ie.c: =A0 =A0 =A0 =A0 =A0 =A0 =A0bzero((v_caddr_t) sc->xmit_buffs[i]=
, sizeof
> *sc->xmit_buffs[i]);
>
> Same mistakes for bzero().
Honestly, I can hardly imagine where v_caddr_t is used there properly,
and if not then why it still ever exists...
[Of course, this can be explained by my low C skills.]
> My change isolates the v_caddr_t mistake in ie.
>
> The c_caddr_t mistake is used 17 times in the kernel and 75 times in
> /usr/src.
>
> Bruce
Thanks for your valuable comments.
--=20
wbr,
pluknet
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAE-mSOKcdaWtZAR0XT2tikyXFbjqBJWwrEo95vhyWM6MkLPuXw>