Bug 170267

Summary: [ixgbe] IXGBE_LE32_TO_CPUS is probably an unintentional no-op
Product: Base System Reporter: Richard Lowe <richlowe>
Component: kernAssignee: Eric Joyner <erj>
Status: Open ---    
Severity: Affects Only Me CC: hiren, kbowling, sbruno
Priority: Normal Keywords: IntelNetworking
Version: unspecified   
Hardware: Any   
OS: Any   

Description Richard Lowe 2012-07-30 05:10:10 UTC
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).
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2012-07-30 05:50:51 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-net
Comment 2 Sean Bruno freebsd_committer 2015-07-01 15:13:14 UTC
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
Comment 3 Sean Bruno freebsd_committer 2015-07-01 15:15:47 UTC
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
Comment 4 Eric Joyner freebsd_committer 2015-07-01 16:48:57 UTC
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.
Comment 5 Sean Bruno freebsd_committer 2015-07-01 17:12:38 UTC
(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.
Comment 6 Eric Joyner freebsd_committer 2015-07-01 18:19:46 UTC
(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.
Comment 7 Sean Bruno freebsd_committer 2015-07-01 18:26:57 UTC
(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]);
}
Comment 8 Eric Joyner freebsd_committer 2015-07-01 18:36:18 UTC
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.
Comment 9 Sean Bruno freebsd_committer 2015-07-01 18:44:38 UTC
(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
Comment 10 Eric Joyner freebsd_committer 2015-07-01 18:59:19 UTC
(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.
Comment 11 Eitan Adler freebsd_committer freebsd_triage 2018-05-28 19:42:56 UTC
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.