| Message ID | 1232517051.9701.27.camel@brick |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- 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 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 -- 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 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 -- 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
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); }
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(-)