Fixing a bug in another system, using an ixgbe driver derived from yours, I've happened upon the fact that IXGBE_LE32_TO_CPUS appears to be a no-op. On FreeBSD, it's defined to be le32dec(), which is a pure function, however the uses in ixgbe_common.c are as if it were side-effecting its argument: 3964 /* Pull in the rest of the buffer (bi is where we left off)*/ 3965 for (; bi <= dword_len; bi++) { 3966 buffer[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi); 3967 IXGBE_LE32_TO_CPUS(&buffer[bi]); 3968 } Fix: Guessing suggests that: buffer[bi] = IXGBE_LE32_TO_CPUS(buffer[bi]); (there are two instances). Is what was intended. How-To-Repeat: Read through ixgbe_common.c and ixgbe_osdep.h. I'm unsure if this causes practical problems (I don't have a big-endian system with this board).
Responsible Changed From-To: freebsd-bugs->freebsd-net
Hrm ... currently, its not even defined to le32dec() in head. ixgbe_osdep.h: /* XXX these need to be revisited */ #define IXGBE_CPU_TO_LE32 htole32 #define IXGBE_LE32_TO_CPUS(x) #define IXGBE_CPU_TO_BE16 htobe16 #define IXGBE_CPU_TO_BE32 htobe32
Hrm ... it looks like it was intentionally removed. Typo? https://svnweb.freebsd.org/base/head/sys/dev/ixgbe/ixgbe_osdep.h?r1=251964&r2=282289
I can re-add it in. I removed it because it doesn't really have an effect on i386/amd64, but I realize someone may want to (try to) use the driver on a big-endian architecture.
(In reply to Eric Joyner from comment #4) Yeah, I think the ppc folks will want this to work. Any input from the comment that originally opened the ticket? It appears that the code wants to convert the array element, but the return value of the macro isn't being assigned to anything. In addition the invocation is converting the address of the element in the array, not the value of the array ... so, I'm unclear what is the right thing to do in the context here.
(In reply to Sean Bruno from comment #5) It's supposed to take the value of buffer[bi] and convert it from little endian to the CPU architecture in-place -- that's why it's using the address of buffer[bi] instead of the value itself.
(In reply to Eric Joyner from comment #6) The prototype for le32dec in byteorder(9) seems to indicate that it does *not* modify the value passed in but instead returns a modified value. sys/endian.h seems to agree with this. I think the man page isn't super clear on this fact in the descriptions. static __inline uint32_t le32dec(const void *pp) { uint8_t const *p = (uint8_t const *)pp; return (((unsigned)p[3] << 24) | (p[2] << 16) | (p[1] << 8) | p[0]); }
It looks like the intent for the macro is for it be an alias to a function like this (on Linux): http://lxr.free-electrons.com/source/include/uapi/linux/swab.h#L235. I think I ended up removing that function because I couldn't find a version in FreeBSD that did the same thing.
(In reply to Eric Joyner from comment #8) quick grep -r of sys shows that most folks #define the swabXX functions to bswapXX functions. dev/cxgbe/osdep.h:#define swab16(x) bswap16(x) dev/cxgbe/osdep.h:#define swab32(x) bswap32(x) dev/cxgbe/osdep.h:#define swab64(x) bswap64(x) ofed/include/asm/byteorder.h:#define swab16 bswap16 ofed/include/asm/byteorder.h:#define swab32 bswap32 ofed/include/asm/byteorder.h:#define swab64 bswap64
(In reply to Sean Bruno from comment #9) The bswap implementation doesn't do the conversion in-place, unlike what the IXGBE macro expects it to. However, in byteorder.h, it looks like they had to make their own implementation to make le32_to_cpus work.
batch change: For bugs that match the following - Status Is In progress AND - Untouched since 2018-01-01. AND - Affects Base System OR Documentation DO: Reset to open status. Note: I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.