On 7/16/21 10:57 AM, Jason Gunthorpe wrote:
> On Tue, Jul 06, 2021 at 11:00:36PM -0500, Bob Pearson wrote:
>
>> +/* rxe_icrc_generate- compute ICRC for a packet. */
>> +void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
>> +{
>> + __be32 *icrcp;
>> + u32 icrc;
>> +
>> + icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
>> + icrc = rxe_icrc_hdr(pkt, skb);
>> + icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
>> + payload_size(pkt) + bth_pad(pkt));
>> + *icrcp = (__force __be32)~icrc;
>> +}
>
> Same comment here, the u32 icrc should be a __be32 because that is
> what rxe_crc32 returns, no force
>
> Jason
>
I agree. The last patch in the series tries to make sense of the byte order.
Here I was trying to take baby steps and just move the code without changing anything.
It makes the thing easier for Zhu to review because no logic changed just where the code is.
However as you point out it doesn't really make sense on the face of it. There isn't any
really good resolution because both the hardware and software versions of the crc32 calculation
are clearly labeled __le but they are stuffed into the ICRC which is clearly identified as __be.
The problem is that it works i.e. interoperates with ConnectX. I would love a conversation with one
of the IBA architects to resolve this.
Bob
