| Message ID | 1366280830-26034-2-git-send-email-dmitry@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Dmitry Kravkov <dmitry@broadcom.com> : > introduce a procedure to read in u32 granularity. > > CC: Francois Romieu <romieu@fr.zoreil.com> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com> > Signed-off-by: Eilon Greenstein <eilong@broadcom.com> > --- > .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 54 +++++++++++++--------- > 1 file changed, 31 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c > index 129d6b2..e7e0ac1 100644 > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c > @@ -1364,11 +1364,25 @@ static int bnx2x_nvram_read(struct bnx2x *bp, u32 offset, u8 *ret_buf, > return rc; > } > > +static int bnx2x_nvram_read32(struct bnx2x *bp, u32 offset, u32 *buf, > + int buf_size) > +{ > + int i, rc = bnx2x_nvram_read(bp, offset, (u8 *)buf, buf_size); > + __be32 *be = (__be32 *)buf; > + > + if (rc) > + return rc; Nit: one of those may be a tad more idiomatic. Not sure. __be32 *be = (__be32 *)buf; int i, rc; rc = bnx2x_nvram_read(bp, offset, (u8 *)buf, buf_size); if (rc) ... or: int rc; rc = bnx2x_nvram_read(bp, offset, (u8 *)buf, buf_size); if (!rc) { __be32 *be = (__be32 *)buf; while ((buf_size -= 4) >= 0) *buf++ = be32_to_cpu(*be++); } [...] > @@ -1383,9 +1397,7 @@ static int bnx2x_get_eeprom(struct net_device *dev, > > /* parameters already validated in ethtool_get_eeprom */ > > - rc = bnx2x_nvram_read(bp, eeprom->offset, eebuf, eeprom->len); > - > - return rc; > + return bnx2x_nvram_read(bp, eeprom->offset, eebuf, eeprom->len); > } Nit--: it's a bit off-topic. [...] > @@ -1573,16 +1584,16 @@ static int bnx2x_nvram_write1(struct bnx2x *bp, u32 offset, u8 *data_buf, > > cmd_flags = (MCPR_NVM_COMMAND_FIRST | MCPR_NVM_COMMAND_LAST); > align_offset = (offset & ~0x03); > - rc = bnx2x_nvram_read_dword(bp, align_offset, &val, cmd_flags); > + rc = bnx2x_nvram_read_dword(bp, align_offset, &val_be, 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_be); (1) > > - /* nvram data is returned as an array of bytes > - * convert it back to cpu order > - */ > - val = be32_to_cpu(val); > + if (rc == 0) { > + val &= ~le32_to_cpu(0xff << BYTE_OFFSET(offset)); > + val |= le32_to_cpu(*data_buf << BYTE_OFFSET(offset)); (2) Either be32_to_cpu or le32_to_cpu above isn't a nop but the commit message only talks of refactoring. It imho lacks a statement about a fix.
> -----Original Message----- > From: Francois Romieu [mailto:romieu@fr.zoreil.com] > Sent: Friday, April 19, 2013 2:15 AM > To: Dmitry Kravkov > Cc: davem@davemloft.net; netdev@vger.kernel.org; Eilon Greenstein > Subject: Re: [PATCH v2 net-next 1/4] bnx2x: refactor nvram read procedure > > Dmitry Kravkov <dmitry@broadcom.com> : > > introduce a procedure to read in u32 granularity. > > > > CC: Francois Romieu <romieu@fr.zoreil.com> > > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com> > > Signed-off-by: Eilon Greenstein <eilong@broadcom.com> > > --- > > .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 54 +++++++++++++--------- > > 1 file changed, 31 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c > > index 129d6b2..e7e0ac1 100644 > > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c > > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c > > @@ -1364,11 +1364,25 @@ static int bnx2x_nvram_read(struct bnx2x *bp, u32 offset, u8 *ret_buf, > > return rc; > > } > > > > +static int bnx2x_nvram_read32(struct bnx2x *bp, u32 offset, u32 *buf, > > + int buf_size) > > +{ > > + int i, rc = bnx2x_nvram_read(bp, offset, (u8 *)buf, buf_size); > > + __be32 *be = (__be32 *)buf; > > + > > + if (rc) > > + return rc; > > Nit: one of those may be a tad more idiomatic. Not sure. > > __be32 *be = (__be32 *)buf; > int i, rc; > > rc = bnx2x_nvram_read(bp, offset, (u8 *)buf, buf_size); > if (rc) > ... > or: > int rc; > > rc = bnx2x_nvram_read(bp, offset, (u8 *)buf, buf_size); > if (!rc) { > __be32 *be = (__be32 *)buf; > > while ((buf_size -= 4) >= 0) > *buf++ = be32_to_cpu(*be++); > } I will adopt this one :) > [...] > > @@ -1383,9 +1397,7 @@ static int bnx2x_get_eeprom(struct net_device *dev, > > > > /* parameters already validated in ethtool_get_eeprom */ > > > > - rc = bnx2x_nvram_read(bp, eeprom->offset, eebuf, eeprom->len); > > - > > - return rc; > > + return bnx2x_nvram_read(bp, eeprom->offset, eebuf, eeprom->len); > > } > > Nit--: it's a bit off-topic. Will separate it > [...] > > @@ -1573,16 +1584,16 @@ static int bnx2x_nvram_write1(struct bnx2x *bp, u32 offset, u8 *data_buf, > > > > cmd_flags = (MCPR_NVM_COMMAND_FIRST | MCPR_NVM_COMMAND_LAST); > > align_offset = (offset & ~0x03); > > - rc = bnx2x_nvram_read_dword(bp, align_offset, &val, cmd_flags); > > + rc = bnx2x_nvram_read_dword(bp, align_offset, &val_be, 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_be); > > (1) > > > > > - /* nvram data is returned as an array of bytes > > - * convert it back to cpu order > > - */ > > - val = be32_to_cpu(val); > > + if (rc == 0) { > > + val &= ~le32_to_cpu(0xff << BYTE_OFFSET(offset)); > > + val |= le32_to_cpu(*data_buf << BYTE_OFFSET(offset)); > > (2) > > Either be32_to_cpu or le32_to_cpu above isn't a nop but the commit > message only talks of refactoring. It imho lacks a statement about > a fix. Good catch - this one should go to -net, I will leave it in net-next series as separate patch with proper message to avoid painful merging, if Dave does not objection for this. > -- > Ueimor -- 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/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c index 129d6b2..e7e0ac1 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c @@ -1364,11 +1364,25 @@ static int bnx2x_nvram_read(struct bnx2x *bp, u32 offset, u8 *ret_buf, return rc; } +static int bnx2x_nvram_read32(struct bnx2x *bp, u32 offset, u32 *buf, + int buf_size) +{ + int i, rc = bnx2x_nvram_read(bp, offset, (u8 *)buf, buf_size); + __be32 *be = (__be32 *)buf; + + if (rc) + return rc; + + for (i = 0; i < buf_size; i += 4) + *buf++ = be32_to_cpu(*be++); + + return 0; +} + static int bnx2x_get_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom, u8 *eebuf) { struct bnx2x *bp = netdev_priv(dev); - int rc; if (!netif_running(dev)) { DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM, @@ -1383,9 +1397,7 @@ static int bnx2x_get_eeprom(struct net_device *dev, /* parameters already validated in ethtool_get_eeprom */ - rc = bnx2x_nvram_read(bp, eeprom->offset, eebuf, eeprom->len); - - return rc; + return bnx2x_nvram_read(bp, eeprom->offset, eebuf, eeprom->len); } static int bnx2x_get_module_eeprom(struct net_device *dev, @@ -1552,9 +1564,8 @@ static int bnx2x_nvram_write1(struct bnx2x *bp, u32 offset, u8 *data_buf, int buf_size) { int rc; - u32 cmd_flags; - u32 align_offset; - __be32 val; + u32 cmd_flags, align_offset, val; + __be32 val_be; if (offset + buf_size > bp->common.flash_size) { DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM, @@ -1573,16 +1584,16 @@ static int bnx2x_nvram_write1(struct bnx2x *bp, u32 offset, u8 *data_buf, cmd_flags = (MCPR_NVM_COMMAND_FIRST | MCPR_NVM_COMMAND_LAST); align_offset = (offset & ~0x03); - rc = bnx2x_nvram_read_dword(bp, align_offset, &val, cmd_flags); + rc = bnx2x_nvram_read_dword(bp, align_offset, &val_be, 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_be); - /* nvram data is returned as an array of bytes - * convert it back to cpu order - */ - val = be32_to_cpu(val); + if (rc == 0) { + val &= ~le32_to_cpu(0xff << BYTE_OFFSET(offset)); + val |= le32_to_cpu(*data_buf << BYTE_OFFSET(offset)); rc = bnx2x_nvram_write_dword(bp, align_offset, val, cmd_flags); @@ -2598,8 +2609,7 @@ static int bnx2x_test_nvram(struct bnx2x *bp) { 0x708, 0x70 }, /* manuf_key_info */ { 0, 0 } }; - __be32 *buf; - u8 *data; + u8 *buf; int i, rc; u32 magic, crc; @@ -2612,26 +2622,24 @@ static int bnx2x_test_nvram(struct bnx2x *bp) rc = -ENOMEM; goto test_nvram_exit; } - data = (u8 *)buf; - rc = bnx2x_nvram_read(bp, 0, data, 4); + rc = bnx2x_nvram_read32(bp, 0, &magic, sizeof(magic)); if (rc) { DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM, "magic value read (rc %d)\n", rc); goto test_nvram_exit; } - magic = be32_to_cpu(buf[0]); if (magic != 0x669955aa) { DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM, "wrong magic value (0x%08x)\n", magic); - rc = -ENODEV; + rc = -EINVAL; goto test_nvram_exit; } for (i = 0; nvram_tbl[i].size; i++) { - rc = bnx2x_nvram_read(bp, nvram_tbl[i].offset, data, + rc = bnx2x_nvram_read(bp, nvram_tbl[i].offset, buf, nvram_tbl[i].size); if (rc) { DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM, @@ -2639,7 +2647,7 @@ static int bnx2x_test_nvram(struct bnx2x *bp) goto test_nvram_exit; } - crc = ether_crc_le(nvram_tbl[i].size, data); + crc = ether_crc_le(nvram_tbl[i].size, buf); if (crc != CRC32_RESIDUAL) { DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM, "nvram_tbl[%d] wrong crc value (0x%08x)\n", i, crc);