Subject and comments are provided below for review. This is for review not a formal one. Please let me know if any more changes are required so that I can include it in a formal patch. -------------------------------------------------------------------------------------------------------- Subject: [PATCH 3/3 v2] netxen: qlogic ethernet : Fix endian bug. Change the datatype of "ip_addr" to __be32 as 'ip' should be in big endian format. Adapter needs "ip address" in big endian format stored at lower 32bit of req.word[1]. netxen_config_ipaddr() now receives 'ip' in big endian format. To satisfy adapter's need, use memcpy() to copy byte by byte of 'ip' into lower 32bit of req.word[1]. Mac address and serial number of adapter need to be in little endian format. Change the data type of the related variables to __le32 / __le64 or cast it explicitly to __le32 / __le64 depending upon the requirement. --------------------------------------------------------------------------------------------------------------- Regards Santosh On Mon, Mar 12, 2012 at 12:22 PM, santosh prasad nayak <santoshprasadnayak@xxxxxxxxx> wrote: > Sure David. > I will resend it with proper comment. > > Regards > Santosh > > On Mon, Mar 12, 2012 at 12:18 PM, David Miller <davem@xxxxxxxxxxxxx> wrote: >> From: santosh nayak <santoshprasadnayak@xxxxxxxxx> >> Date: Mon, 12 Mar 2012 12:08:23 +0530 >> >>> From: Santosh Nayak <santoshprasadnayak@xxxxxxxxx> >>> >>> Fix endian bug. >>> >>> Signed-off-by: Santosh Nayak <santoshprasadnayak@xxxxxxxxx> >> >> This is a very non-trivial patch, therefore the terse commit >> message and the lack of any comments in this code is unacceptable. >> >> This commit message is not only terse, it's wrong. You're not >> fixing an endian bug, you're fixing several of them. >> >> Explain everything in your commit message. >> >>> req.words[0] = cpu_to_le64(cmd); >>> - req.words[1] = cpu_to_le64(ip); >>> + memcpy(&req.words[1], &ip, sizeof(u32)); >> >> Nobody is going to read that and have any idea why it's correct unless >> they happen to be able to discover the long thread you guys had this >> past week working out how to do this change correctly. >> >> Putting a comment here for the rest of us mere mortals is therefore >> absolutely required. -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
