From owner-cvs-src@FreeBSD.ORG Sun Apr 6 23:26:35 2003 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 96BB537B401; Sun, 6 Apr 2003 23:26:35 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id CA32343F93; Sun, 6 Apr 2003 23:26:33 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id QAA15009; Mon, 7 Apr 2003 16:26:30 +1000 Date: Mon, 7 Apr 2003 16:26:28 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Maxime Henrion In-Reply-To: <200304062309.h36N9vka088364@repoman.freebsd.org> Message-ID: <20030407153635.J3268@gamplex.bde.org> References: <200304062309.h36N9vka088364@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Apr 2003 06:26:36 -0000 On Sun, 6 Apr 2003, Maxime Henrion wrote: > mux 2003/04/06 16:09:57 PDT > > FreeBSD src repository > > Modified files: > sys/dev/fxp if_fxp.c > Log: > Because alpha can't access memory in 16-bit granularity, > we're using an atomic operation to clear the suspend flag > in fxp_start(). Since other architectures may need the > same thing, we want to do it all the time and not only > in the __alpha__ case. However, we don't want to use > atomic operations on 16-bit integers, because those may > not be available on any architecture. We're thus faking > a 32-bit atomic operation here. This patch also deals > with endianness here. Not to pick on this commit but... Atomic operations on integers other than int and u_int shouldn't be available on any arches, since they are little need in MD code and cannot be used in MI code since using them break would break some arches. I removed them in my i386 version. This mainly exposes brokenness in netgraph and drm, since these sources use atomic operations on longs and there are no atomic operations on longs on some arches (e.g., i386's with correctly-sized longs). The patch has some style bugs: > Index: if_fxp.c > =================================================================== > RCS file: /home/ncvs/src/sys/dev/fxp/if_fxp.c,v > retrieving revision 1.158 > retrieving revision 1.159 > diff -u -2 -r1.158 -r1.159 > --- if_fxp.c 6 Apr 2003 01:27:12 -0000 1.158 > +++ if_fxp.c 6 Apr 2003 23:09:57 -0000 1.159 > @@ -33,5 +33,5 @@ > > #include > -__FBSDID("$FreeBSD: src/sys/dev/fxp/if_fxp.c,v 1.158 2003/04/06 01:27:12 mux Exp $"); > +__FBSDID("$FreeBSD: src/sys/dev/fxp/if_fxp.c,v 1.159 2003/04/06 23:09:57 mux Exp $"); > > #include $FreeBSD$ should not be used in the kernel. > @@ -58,4 +58,5 @@ > #include > > +#include is part of . It should not be included directly if is included. > #include /* for DELAY */ Old style bug: should rarely be included. It especially shouldn't be included "for DELAY" since it doesn't declare DELAY. It is only needed for a couple of misplaced and/or problematic interfaces like sysbeep(). (sysbeep() only needs to have an MI implementation, but it currently has a PC-specific interface (its `pitch' arg is actually its wavelength in units of i8254 timer cycles for the PC implementation of i8254's, except on alphas and some other arches cloned from alphas where its `pitch' arg is actually its pitch so that it is not even bug for bug compatible with its callers). Some means of beeping is needed generally although the machine might not have any hardware for it.) This include was removed from most drivers, but was restored in some for compatibility with RELENG_2 or RELENG_3 and then cut and pasted into zillions. In this file, it was removed in rev.1.97 and then rotted back in in rev.1.107. > @@ -1208,5 +1209,5 @@ > { > struct fxp_softc *sc = ifp->if_softc; > - struct fxp_tx *txp; > + struct fxp_tx *txp, *last; > struct mbuf *mb_head; > int error; Disordered declarations (new: 't' > 'l' && 's' > 'l'; old: 'm' > 's'). > @@ -1384,10 +1384,14 @@ > * up the status while we update the command field. > * This could cause us to overwrite the completion status. > + * > + * This is a bit tricky, because we want to avoid using > + * atomic operations on 16bits values, since they may not > + * be available on any architecture or may be very > + * inefficient. > */ > - atomic_clear_short(&sc->fxp_desc.tx_last->tx_cb->cb_command, > - FXP_CB_COMMAND_S); > -#else > - sc->fxp_desc.tx_last->tx_cb->cb_command &= ~FXP_CB_COMMAND_S; > -#endif /*__alpha__*/ > + last = sc->fxp_desc.tx_last; > + atomic_clear_32((u_int32_t *)&last->tx_cb->cb_status, > + htobe32(bswap16(FXP_CB_COMMAND_S))); Some arches might not have 32-bit atomic operations either. This problem is not limited to alphas, so the ifdef shouldn't be '#ifdef __alpha__'. I thought that this problem also affects sparc64s since I first learned about the full unportability of atomic operations from jake. Short condtionals should not have a comment on the #endif. > + > sc->fxp_desc.tx_last = txp; Extra blank line. KNF uses no optional blank lines (indent -sob), and this blank line is not an option -- it disassociates the final statement of the initialization of tx_last from the rest of the initialization. There is also a wrong blank line before the initialization of tx_last. It disassociates the initialization from the comment that describes the initialization. The big comment for the alpha case makes the scope of comment on the initialization especially unclear. Bruce