>-----Original Message----- >From: Keller, Jacob E <[email protected]> >Sent: Thursday, January 29, 2026 7:11 PM >To: Andrew Lunn <[email protected]>; Kwapulinski, Piotr ><[email protected]> >Cc: [email protected]; [email protected]; >[email protected]; [email protected]; [email protected] >Subject: Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: e610: remove redundant >assignment > > > >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..... I'll work out the solution. Meanwhile, possibly it's good idea to take this patch since this code is unnecessary regardless of the final outcome. Thanks, Piotr
[...]
