cvs commit: src/sys/dev/fxp if_fxp.c

[Bruce Evans]

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 <sys/cdefs.h>
> -__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 <sys/param.h>

$FreeBSD$ should not be used in the kernel.

> @@ -58,4 +58,5 @@
>  #include <net/if_arp.h>
>
> +#include <machine/atomic.h>

<machine/atomic.h> is part of <sys/systm.h>.  It should not be included
directly if <sys/systm.h> is included.

>  #include <machine/clock.h>	/* for DELAY */

Old style bug: <machine/clock.h> 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