Re: [PATCH 3/3 v2] netxen: qlogic ethernet : Fix endian bug.

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
[off-list ref] wrote:

Sure David.
I will resend it with proper comment.

Regards
Santosh

On Mon, Mar 12, 2012 at 12:18 PM, David Miller [off-list ref] wrote:

quoted

From: santosh nayak <redacted>
Date: Mon, 12 Mar 2012 12:08:23 +0530

quoted

From: Santosh Nayak <redacted>

Fix endian bug.

Signed-off-by: Santosh Nayak <redacted>

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.

quoted

      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.