Message ID | 1232663768-4028-2-git-send-email-vladislav.yasevich@hp.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Vlad Yasevich <vladislav.yasevich@hp.com> wrote: > > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h > index b799fb2..2fec3c3 100644 > --- a/include/net/sctp/checksum.h > +++ b/include/net/sctp/checksum.h > @@ -79,5 +79,5 @@ static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32) > > static inline __be32 sctp_end_cksum(__be32 crc32) > { > - return ~crc32; > + return (__force __be32)~cpu_to_le32((__force u32)crc32); > } Ouch, surely there is a better way to do this? Cheers,
On Fri, Jan 23, 2009 at 04:19:56PM +1100, Herbert Xu wrote: > Vlad Yasevich <vladislav.yasevich@hp.com> wrote: > > > > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h > > index b799fb2..2fec3c3 100644 > > --- a/include/net/sctp/checksum.h > > +++ b/include/net/sctp/checksum.h > > @@ -79,5 +79,5 @@ static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32) > > > > static inline __be32 sctp_end_cksum(__be32 crc32) > > { > > - return ~crc32; > > + return (__force __be32)~cpu_to_le32((__force u32)crc32); > > } > > Ouch, surely there is a better way to do this? In fact this looks wrong. Has this code actually been tested on big-endian? Cheers,
On Fri, Jan 23, 2009 at 04:28:49PM +1100, Herbert Xu wrote: > > > > static inline __be32 sctp_end_cksum(__be32 crc32) > > > { > > > - return ~crc32; > > > + return (__force __be32)~cpu_to_le32((__force u32)crc32); > > > } > > > > Ouch, surely there is a better way to do this? > > In fact this looks wrong. Has this code actually been tested > on big-endian? Reading this again it does seem to do the right thing as it's using the raw crc32c interface as opposed to crypto crc32c. However, I suggest that we change it as follows: 1) Make sh->csum __le32 since we're using crc32c_le. 2) Change all intermediate values in sctp/checksum.h to u32. 3) Make sctp_end_cksum return __le32 and have it do return cpu_to_le32(~crc); Cheers,
Herbert Xu wrote: > On Fri, Jan 23, 2009 at 04:28:49PM +1100, Herbert Xu wrote: >>>> static inline __be32 sctp_end_cksum(__be32 crc32) >>>> { >>>> - return ~crc32; >>>> + return (__force __be32)~cpu_to_le32((__force u32)crc32); >>>> } >>> Ouch, surely there is a better way to do this? >> In fact this looks wrong. Has this code actually been tested >> on big-endian? Yes, the code was tested by me and the person who reported it as listed in the commit log. :) > > Reading this again it does seem to do the right thing as it's > using the raw crc32c interface as opposed to crypto crc32c. > > However, I suggest that we change it as follows: > > 1) Make sh->csum __le32 since we're using crc32c_le. > 2) Change all intermediate values in sctp/checksum.h to u32. > 3) Make sctp_end_cksum return __le32 and have it do > > return cpu_to_le32(~crc); > I'll give it some thought. It would clean up all the __force casts, but it will be a little misleading to define a packet checksum as little endian. :) -vlad > Cheers, -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 23, 2009 at 09:52:44AM -0500, Vlad Yasevich wrote: > > I'll give it some thought. It would clean up all the __force > casts, but it will be a little misleading to define a packet > checksum as little endian. :) Not at all! Just because something is on the network doesn't mean that it's big-endian. In this case, we're using the little- endian CRC32C implementation for the checksum so it makes perfect sense for it to be marked as __le32. Network-order just has to be fixed in endian, i.e., either big or small, you just need to stick with the one you choose regardless of the endianness of the host. Cheers,
diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h index b799fb2..2fec3c3 100644 --- a/include/net/sctp/checksum.h +++ b/include/net/sctp/checksum.h @@ -79,5 +79,5 @@ static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32) static inline __be32 sctp_end_cksum(__be32 crc32) { - return ~crc32; + return (__force __be32)~cpu_to_le32((__force u32)crc32); }