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.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=bef7d49101cdf28830a648f63ff00998fbe54715 commit bef7d49101cdf28830a648f63ff00998fbe54715 Author: Kevin Bowling <kbowling@FreeBSD.org> AuthorDate: 2023-08-14 01:47:09 +0000 Commit: Kevin Bowling <kbowling@FreeBSD.org> CommitDate: 2023-08-14 01:55:39 +0000 ixgbe: define IXGBE_LE32_TO_CPUS Richard Lowe notes in PR 170267 IXGBE_LE32_TO_CPUS was previously directly defined as le32dec() which is a pure function but the shared code is expecting an in place conversion. In SVN r282289 its assignment was removed altogether. There was some deliberation in the PR on what to define this as, we just need to do the update in place which is easy enough. The uintptr_t casts in the shared code were from a DPDK sync and are unwanted with our new ixgbe_osdep.h implementation. PR: 170267 Reported by: Richard Lowe <richlowe@richlowe.net> MFC after: 1 week sys/dev/ixgbe/ixgbe_common.c | 4 ++-- sys/dev/ixgbe/ixgbe_osdep.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=42d73e9ec1464a4ebc26d32201def571550e9ba9 commit 42d73e9ec1464a4ebc26d32201def571550e9ba9 Author: Kevin Bowling <kbowling@FreeBSD.org> AuthorDate: 2023-08-14 01:47:09 +0000 Commit: Kevin Bowling <kbowling@FreeBSD.org> CommitDate: 2023-08-25 07:17:23 +0000 ixgbe: define IXGBE_LE32_TO_CPUS Richard Lowe notes in PR 170267 IXGBE_LE32_TO_CPUS was previously directly defined as le32dec() which is a pure function but the shared code is expecting an in place conversion. In SVN r282289 its assignment was removed altogether. There was some deliberation in the PR on what to define this as, we just need to do the update in place which is easy enough. The uintptr_t casts in the shared code were from a DPDK sync and are unwanted with our new ixgbe_osdep.h implementation. PR: 170267 Reported by: Richard Lowe <richlowe@richlowe.net> (cherry picked from commit bef7d49101cdf28830a648f63ff00998fbe54715) sys/dev/ixgbe/ixgbe_common.c | 4 ++-- sys/dev/ixgbe/ixgbe_osdep.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)