[PATCH 9/9] bnx2x: annotate nvram dword reading/writing

[PATCH 9/9] bnx2x: annotate nvram dword reading/writing

[Harvey Harrison]

I believe this exposed an endian bug as the shifting of
bytes from the data buffer was done in cpu-order, then
masked into a be32 and the combined value was then converted
to cpu-order, this does all the masking in be-byteorder and
passes a cpu-ordered value to the write routine.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 drivers/net/bnx2x_main.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index 5e5e008..d0b0f6e 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -8011,7 +8011,7 @@ static void bnx2x_disable_nvram_access(struct bnx2x *bp)
 			MCPR_NVM_ACCESS_ENABLE_WR_EN)));
 }
 
-static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, u32 *ret_val,
+static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, __be32 *ret_val,
 				  u32 cmd_flags)
 {
 	int count, i, rc;
@@ -8047,8 +8047,7 @@ static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, u32 *ret_val,
 			/* we read nvram data in cpu order
 			 * but ethtool sees it as an array of bytes
 			 * converting to big-endian will do the work */
-			val = cpu_to_be32(val);
-			*ret_val = val;
+			*ret_val = cpu_to_be32(val);
 			rc = 0;
 			break;
 		}
@@ -8062,7 +8061,7 @@ static int bnx2x_nvram_read(struct bnx2x *bp, u32 offset, u8 *ret_buf,
 {
 	int rc;
 	u32 cmd_flags;
-	u32 val;
+	__be32 val;
 
 	if ((offset & 0x03) || (buf_size & 0x03) || (buf_size == 0)) {
 		DP(BNX2X_MSG_NVM,
@@ -8181,7 +8180,7 @@ static int bnx2x_nvram_write1(struct bnx2x *bp, u32 offset, u8 *data_buf,
 	int rc;
 	u32 cmd_flags;
 	u32 align_offset;
-	u32 val;
+	__be32 val;
 
 	if (offset + buf_size > bp->common.flash_size) {
 		DP(BNX2X_MSG_NVM, "Invalid parameter: offset (0x%x) +"
@@ -8203,14 +8202,12 @@ static int bnx2x_nvram_write1(struct bnx2x *bp, u32 offset, u8 *data_buf,
 	rc = bnx2x_nvram_read_dword(bp, align_offset, &val, cmd_flags);
 
 	if (rc == 0) {
-		val &= ~(0xff << BYTE_OFFSET(offset));
-		val |= (*data_buf << BYTE_OFFSET(offset));
-
 		/* nvram data is returned as an array of bytes
 		 * convert it back to cpu order */
-		val = be32_to_cpu(val);
+		val &= ~cpu_to_be32(0xff << (8 * (offset & 0x03)));
+		val |= cpu_to_be32(*data_buf << (8 * (offset & 0x03)));
 
-		rc = bnx2x_nvram_write_dword(bp, align_offset, val,
+		rc = bnx2x_nvram_write_dword(bp, align_offset, be32_to_cpu(val),
 					     cmd_flags);
 	}
 
-- 
1.6.1.249.g455e5

Re: [PATCH 9/9] bnx2x: annotate nvram dword reading/writing

[Eilon Greenstein]

On Tue, 2009-01-20 at 21:50 -0800, Harvey Harrison wrote:
> I believe this exposed an endian bug as the shifting of
> bytes from the data buffer was done in cpu-order, then
> masked into a be32 and the combined value was then converted
> to cpu-order, this does all the masking in be-byteorder and
> passes a cpu-ordered value to the write routine.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

This change is breaking the FW upgrade utility which uses this interface
since it is changing the content. I will need to work with the engineer
that owns this utility, but this change will probably stay out for a
while

Re: [PATCH 9/9] bnx2x: annotate nvram dword reading/writing

[Harvey Harrison]

On Thu, 2009-01-22 at 19:50 +0200, Eilon Greenstein wrote:
> On Tue, 2009-01-20 at 21:50 -0800, Harvey Harrison wrote:
> > I believe this exposed an endian bug as the shifting of
> > bytes from the data buffer was done in cpu-order, then
> > masked into a be32 and the combined value was then converted
> > to cpu-order, this does all the masking in be-byteorder and
> > passes a cpu-ordered value to the write routine.
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> 
> This change is breaking the FW upgrade utility which uses this interface
> since it is changing the content. I will need to work with the engineer
> that owns this utility, but this change will probably stay out for a
> while
> 

No worries, can you explain briefly what the code is trying to accomplish?
Maybe it was written assuming a le-machine, because this is going to
work differently on a be-machine.

Harvey

Re: [PATCH 9/9] bnx2x: annotate nvram dword reading/writing

[Eilon Greenstein]

On Thu, 2009-01-22 at 10:18 -0800, Harvey Harrison wrote:
> On Thu, 2009-01-22 at 19:50 +0200, Eilon Greenstein wrote:
> > On Tue, 2009-01-20 at 21:50 -0800, Harvey Harrison wrote:
> > > I believe this exposed an endian bug as the shifting of
> > > bytes from the data buffer was done in cpu-order, then
> > > masked into a be32 and the combined value was then converted
> > > to cpu-order, this does all the masking in be-byteorder and
> > > passes a cpu-ordered value to the write routine.
> > > 
> > > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > 
> > This change is breaking the FW upgrade utility which uses this interface
> > since it is changing the content. I will need to work with the engineer
> > that owns this utility, but this change will probably stay out for a
> > while
> > 
> 
> No worries, can you explain briefly what the code is trying to accomplish?
> Maybe it was written assuming a le-machine, because this is going to
> work differently on a be-machine.
> 
> Harvey

The utility simply works with the current implementation (on both be and
le) and this is why changing the logic breaks it. I will look into it to
see if we can change the utility logic as well (maybe according to the
driver version) - it is not that easy since this utility supports other
drivers as well.

Harvey - thanks again for this patch, it is definitely a step in the
right direction.

Eilon