On 1/29/2026 9:44 AM, Andrew Lunn wrote:
/* Read sync Admin Command response */
- if ((hicr & IXGBE_PF_HICR_SV)) {
- for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
+ if ((hicr & IXGBE_PF_HICR_SV))
+ for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA(i));
- raw_desc[i] = raw_desc[i];
Did you look through the history? When i see code like this it makes
me want to have an understanding why it exists, since it looks so odd.
Is it a merge conflict resolution gone bad? Is it a typo and there is
a cooked_desc[i] which could be set?
Nope. Looking at git blame for the kernel, this appears to have been
here since its submission. I then went and checked what was done in the
out-of-tree releases out of curiosity, and it looks like there was
originally a CPU_TO_LE32 macro which was doing byte swaps, and an
equivalent when writing the data in to the registers.
raw_desc[1] = read_reg(...);
raw_desc[1] = cpu_to_le32(raw_desc[i]);
I suspect the byte swapping got removed from the original upstream
submission but no one noticed the extra assignment.
Of course the register accesses always read the values in host order,
and I can't imagine an OS not doing that...
Hmm.. But now I have some questions...
The raw_desc value comes from the desc parameter of the function. Thats
libie_aq_desc now, and its defined using __le* values..
We're chunking up the descriptor buffer and writing it to a bunch of
register blocks, and we don't convert from LE32 to CPU now... so on a
BigEndian system this is going to not swap the values properly...
Which makes me think the original change would be required on BE32
systems.. But.. even worse..
I don't think we actually can just blindly copy the values as chunks of
4 bytes and byte swap them.. That would re-arrange the fields, since the
structure layout uses __le16:
struct libie_aq_desc {
__le16 flags;
__le16 opcode;
These would get swapped out of order.
__le16 datalen;
__le16 retval;
__le32 cookie_high;
__le32 cookie_low;
union {
u8 raw[16];
struct libie_aqc_generic generic;
struct libie_aqc_get_ver get_ver;
struct libie_aqc_driver_ver driver_ver;
struct libie_aqc_req_res res_owner;
struct libie_aqc_list_caps get_cap;
struct libie_aqc_fw_log fw_log;
} params;
};
LIBIE_CHECK_STRUCT_LEN(32, libie_aq_desc);
I actually don't know which is "correct", as I don't really understand
what the register interface expects, and how it will get interpreted by
the firmware...
Maybe the byteswap of each 4-byte block is right? but I'm really
uncertain now...
Presumably it expects flags first and then opcode? we're writing it that
way now on a LE system.. But on a BE system thats going to be
byteswapped before going into the register.. so our 4byte chunk would
end up potentially reversing the flags and opcode w.r.t what the
firmware sees??
Hmm.....
/* Read async Admin Command response */
- if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C)) {
- for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
+ if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C))
+ for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));
- raw_desc[i] = raw_desc[i];
and it exists twice. Which makes it even odder....
Andrew
