On Fri, Jan 27, 2006 at 03:35:02AM +0000, Christos Zoulas wrote:
>  | --- if_sip.c.~1.101.~	Sun Feb 27 01:27:33 2005
>  | +++ if_sip.c	Wed Jan 25 00:51:11 2006
>  | @@ -1354,10 +1354,20 @@
>  |  		 * This apparently has to be on the last descriptor of
>  |  		 * the packet.
>  |  		 */
>  | +
>  | +		/*
>  | +		 * Byte swapping is tricky. We need to provide the tag
>  | +		 * in a network byte order. On a big-endian machine,
>  | +		 * the byteorder is correct, but we need to swap it
>  | +		 * anyway, because this will be undone by the outside
>  | +		 * htole32(). That's why there must be an
>  | +		 * unconditional swap instead of htons() inside.
>  | +		 */
>  |  		if ((mtag = VLAN_OUTPUT_TAG(&sc->sc_ethercom, m0)) != NULL) {
>  |  			sc->sc_txdescs[lasttx].sipd_extsts |=
>  | -			    htole32(EXTSTS_VPKT |
>  | -				    (VLAN_TAG_VALUE(mtag) & EXTSTS_VTCI));
>  | +			    htole32(EXTSTS_VPKT | 
>  | +					(bswap16(VLAN_TAG_VALUE(mtag)) &
>  | +					 EXTSTS_VTCI));
>  |  		}
>  |  
>  |  		/*
>  | @@ -1969,8 +1979,19 @@
>  |  		 * If VLANs are enabled, VLAN packets have been unwrapped
>  |  		 * for us.  Associate the tag with the packet.
>  |  		 */
>  | +
>  | +		/*
>  | +		 * Again, byte swapping is tricky. Hardware provided
>  | +		 * the tag in the network byte order, but extsts was
>  | +		 * passed through le32toh() in the meantime. On a
>  | +		 * big-endian machine, we need to swap it again. On a
>  | +		 * little-endian machine, we need to convert from the
>  | +		 * network to host byte order. This means that we must
>  | +		 * swap it in any case, so unconditional swap instead
>  | +		 * of htons() is used.
>  | +		 */
>  |  		if ((extsts & EXTSTS_VPKT) != 0) {
>  | -			VLAN_INPUT_TAG(ifp, m, ntohs(extsts & EXTSTS_VTCI),
>  | +			VLAN_INPUT_TAG(ifp, m, bswap16(extsts & EXTSTS_VTCI),
>  |  			    continue);
>  |  		}
>  |  
>  -- End of excerpt from Pavel Cahyna
>  
>  
>  This does not seem right because in the first part of the patch you do:
>  
>  +			    htole32(EXTSTS_VPKT | 
>  +					(bswap16(VLAN_TAG_VALUE(mtag)) &
>  +					 EXTSTS_VTCI));
>  
>  and in the second:
>  
>  | +			VLAN_INPUT_TAG(ifp, m, bswap16(extsts & EXTSTS_VTCI),
>  
>  because in the first patch you are not bswapping16 EXTSTS_VTCI and in the
>  second you do?

I think it can be justified algebraically:

The firts patched part does:

mtag(host byte order) -> bswap16 -> &= EXTSTS_VTCI -> |= EXTSTS_VPKT ->
-> htole32 -> tx descriptor

while the second does:

rx descriptor -> le32toh -> &= EXTSTS_VTCI -> bswap16 -> mtag(host byte
order)

So, the second sequence is inverse to the first.

Pavel