On Mon, Jul 01, 2019 at 10:13:56PM +0100, Al Viro wrote:
> On Mon, Jul 01, 2019 at 10:36:26PM +0200, Heiner Kallweit wrote:
>
> > > The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> > > third argument treats it as a host-endian integer. So... Has anyone
> > > tested that code on b-e host? Should that ntohs() actually be swab16(),
> > > yielding (on any host) the same value we currently get for l-e hosts only?
> > >
> > I haven't seen any b-e host with a Realtek network chip yet.
>
> Ever tried to google for realtek 8169 pcie card? The first hit is this:
> https://www.amazon.com/Realtek-Chipset-Ethernet-Interface-Software/dp/B007MWYCG2
> and certainly does look like it should fit into at least some G5 Macs.
> What's more, googling for realtek 8169 PCI card brings quite a bit (top
> hit happens to be on ebay for ~$8). That certainly shall fit into
> any number of big-endian motherboards...
>
> Sure, there's a plenty of embedded r8169 on motherboards (mostly x86 ones),
> but these beasts do exist on discrete cards. I'm fairly certain that I've
> got one or two somewhere in the detritus pile and they are fairly cheap
> these days.
>
> So it wouldn't cost too much to put together a mixed network, with
> r8169 both on l-e and b-e hosts and play with VLAN setups there...
FWIW, looking at r8169 docs, they say that Rx descriptor has, at offset
4, a 32bit value (l-e) with
bits 17..31 reserved
bit 16: TAVA (Tag Available)
bits 8..15: VIDL (lower 8 bits of VLAN ID)
bits 5..7: PRIO (priority)
bit 4: CFI (Canonical Format Indicator)
bits 0..3: VIDH (upper 4 bits of VLAN ID)
AFAICS, in kernel-side representation we want VIDL in bits 0..7, VIDH - 8..11,
CFI - 12 and PRIO - 13..15, so that VLAN ID is simply value & 0xfff.
IOW, we do want 256 * octet4 + octet5, i.e.
swab16(le32_to_cpu(desc->opts2) & 0xffff).
Regardless of the host endianness. Some cards might be possible to set up to
byteswap the 32bit values on the way to/from host, but even in that case we
would have wanted
swab16(desc->opts2 & 0xffff),
not
be16_to_cpu(le32_to_cpu(desc->opts2 & 0xffff)
and if it *is* set that way, the current code will break anyway.
Again, all of that is from RTFS alone - I have _not_ tested that, but
it does look like your original patch has missed that while these
16 bits are stored in network order _in_ _card_ _memory_, we'd
already done cpu_to_le32 to the containing 32bit word. So the
value is host-endian, and the lower 16 bits are consistently in
the wrong order, be it b-e or l-e host. IOW, swab16 is the right
thing to do and sparse warning has been correct.
