[PATCH net-next 0/4] bnx2x: refactor nvram related code and update version

[PATCH net-next 0/4] bnx2x: refactor nvram related code and update version

[Dmitry Kravkov]

Hi Dave,

Please cosider appliyng this short series to net-next

Thanks
Dmitry

[PATCH net-next 1/4] bnx2x: refactor nvram read procedure

[Dmitry Kravkov]

introduce a parameter to allow nvram read to return
data in BE or cpu order.

Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c    |   53 +++++++++-----------
 1 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 129d6b2..900c0d7 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -1261,11 +1261,12 @@ 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, __be32 *ret_val,
-				  u32 cmd_flags)
+static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, u32 *ret_val,
+				  u32 cmd_flags, bool to_be)
 {
 	int count, i, rc;
 	u32 val;
+	__be32 *be_val = (__be32 *)ret_val;
 
 	/* build the command word */
 	cmd_flags |= MCPR_NVM_COMMAND_DOIT;
@@ -1295,10 +1296,14 @@ static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, __be32 *ret_val,
 		if (val & MCPR_NVM_COMMAND_DONE) {
 			val = REG_RD(bp, MCP_REG_MCPR_NVM_READ);
 			/* we read nvram data in cpu order
-			 * but ethtool sees it as an array of bytes
+			 * but ethtool uses it as an array of bytes
 			 * converting to big-endian will do the work
+			 * if requested.
 			 */
-			*ret_val = cpu_to_be32(val);
+			if (to_be)
+				*be_val = cpu_to_be32(val);
+			else
+				*ret_val = val;
 			rc = 0;
 			break;
 		}
@@ -1310,11 +1315,10 @@ static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, __be32 *ret_val,
 }
 
 static int bnx2x_nvram_read(struct bnx2x *bp, u32 offset, u8 *ret_buf,
-			    int buf_size)
+			    int buf_size, bool to_be)
 {
 	int rc;
-	u32 cmd_flags;
-	__be32 val;
+	u32 cmd_flags, val;
 
 	if ((offset & 0x03) || (buf_size & 0x03) || (buf_size == 0)) {
 		DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
@@ -1341,7 +1345,7 @@ static int bnx2x_nvram_read(struct bnx2x *bp, u32 offset, u8 *ret_buf,
 	/* read the first word(s) */
 	cmd_flags = MCPR_NVM_COMMAND_FIRST;
 	while ((buf_size > sizeof(u32)) && (rc == 0)) {
-		rc = bnx2x_nvram_read_dword(bp, offset, &val, cmd_flags);
+		rc = bnx2x_nvram_read_dword(bp, offset, &val, cmd_flags, to_be);
 		memcpy(ret_buf, &val, 4);
 
 		/* advance to the next dword */
@@ -1353,7 +1357,7 @@ static int bnx2x_nvram_read(struct bnx2x *bp, u32 offset, u8 *ret_buf,
 
 	if (rc == 0) {
 		cmd_flags |= MCPR_NVM_COMMAND_LAST;
-		rc = bnx2x_nvram_read_dword(bp, offset, &val, cmd_flags);
+		rc = bnx2x_nvram_read_dword(bp, offset, &val, cmd_flags, to_be);
 		memcpy(ret_buf, &val, 4);
 	}
 
@@ -1383,7 +1387,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);
+	rc = bnx2x_nvram_read(bp, eeprom->offset, eebuf, eeprom->len, true);
 
 	return rc;
 }
@@ -1552,9 +1556,7 @@ 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;
 
 	if (offset + buf_size > bp->common.flash_size) {
 		DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
@@ -1573,16 +1575,11 @@ 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, cmd_flags, false);
 
 	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 &= ~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 +2595,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,16 +2608,15 @@ 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_read(bp, 0, buf, 4, false);
 	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]);
+	magic = *(u32 *)(buf);
 	if (magic != 0x669955aa) {
 		DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
 		   "wrong magic value (0x%08x)\n", magic);
@@ -2631,15 +2626,15 @@ static int bnx2x_test_nvram(struct bnx2x *bp)
 
 	for (i = 0; nvram_tbl[i].size; i++) {
 
-		rc = bnx2x_nvram_read(bp, nvram_tbl[i].offset, data,
-				      nvram_tbl[i].size);
+		rc = bnx2x_nvram_read(bp, nvram_tbl[i].offset, buf,
+				      nvram_tbl[i].size, true);
 		if (rc) {
 			DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
 			   "nvram_tbl[%d] read data (rc %d)\n", i, rc);
 			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);
-- 
1.7.7.2

[PATCH net-next 2/4] bnx2x: add additional regions for CRC memory test

[Dmitry Kravkov]

a. Common tree of `dir` structures.
b. Multi-port devices structures.

Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h        |    7 +-
 .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c    |  168 +++++++++++++++++---
 2 files changed, 151 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index c630342..87629fd 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -850,6 +850,9 @@ struct bnx2x_common {
 #define CHIP_IS_57840_VF(bp)		(CHIP_NUM(bp) == CHIP_NUM_57840_VF)
 #define CHIP_IS_E1H(bp)			(CHIP_IS_57711(bp) || \
 					 CHIP_IS_57711E(bp))
+#define CHIP_IS_57811xx(bp)		(CHIP_IS_57811(bp) || \
+					 CHIP_IS_57811_MF(bp) || \
+					 CHIP_IS_57811_VF(bp))
 #define CHIP_IS_E2(bp)			(CHIP_IS_57712(bp) || \
 					 CHIP_IS_57712_MF(bp) || \
 					 CHIP_IS_57712_VF(bp))
@@ -859,9 +862,7 @@ struct bnx2x_common {
 					 CHIP_IS_57810(bp) || \
 					 CHIP_IS_57810_MF(bp) || \
 					 CHIP_IS_57810_VF(bp) || \
-					 CHIP_IS_57811(bp) || \
-					 CHIP_IS_57811_MF(bp) || \
-					 CHIP_IS_57811_VF(bp) || \
+					 CHIP_IS_57811xx(bp) || \
 					 CHIP_IS_57840(bp) || \
 					 CHIP_IS_57840_MF(bp) || \
 					 CHIP_IS_57840_VF(bp))
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 900c0d7..b32a7f0 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -2579,14 +2579,133 @@ static int bnx2x_test_ext_loopback(struct bnx2x *bp)
 	return rc;
 }
 
+struct code_entry {
+	u32 sram_start_addr;
+	u32 code_attribute;
+#define CODE_IMAGE_TYPE_MASK			0xf0800003
+#define CODE_IMAGE_AFEX_PROFILES_DATA		0xd0000003
+#define CODE_IMAGE_LENGTH_MASK			0x007ffffc
+#define CODE_IMAGE_TYPE_EXTENDED_DIR		0xe0000000
+	u32 nvm_start_addr;
+};
+
+#define CODE_ENTRY_MAX			16
+#define CODE_ENTRY_EXTENDED_DIR_IDX	15
+#define MAX_IMAGES_IN_EXTENDED_DIR	64
+
 #define CRC32_RESIDUAL			0xdebb20e3
+#define CRC_BUFF_SIZE			256
+
+static int bnx2x_nvram_crc(struct bnx2x *bp,
+			   int offset,
+			   int size,
+			   u8 *buff)
+{
+	u32 crc = ~0;
+	int done = 0;
+
+	while (done < size) {
+		int count = min_t(int, size - done, CRC_BUFF_SIZE);
+
+		if (bnx2x_nvram_read(bp, offset + done, buff, count, true))
+			return -EIO;
+		crc = crc32_le(crc, buff, count);
+		done += count;
+	}
+
+	if (crc != CRC32_RESIDUAL)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int bnx2x_test_nvram_dir(struct bnx2x *bp,
+				struct code_entry *entry,
+				u8 *buff)
+{
+	size_t size = entry->code_attribute & CODE_IMAGE_LENGTH_MASK;
+	u32 type = entry->code_attribute & CODE_IMAGE_TYPE_MASK;
+	int rc = 0;
+
+	/* Zero-length images and AFEX profiles does not have CRC */
+	if (size == 0 || type == CODE_IMAGE_AFEX_PROFILES_DATA)
+		return 0;
+
+	rc = bnx2x_nvram_crc(bp, entry->nvm_start_addr, size, buff);
+	if (rc == -EIO)
+		DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
+		   "Unable to read image %x\n", type);
+	else if (rc)
+		DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
+		   "image %x has wrong crc value\n", type);
+
+	return rc;
+}
+
+static int bnx2x_test_nvram_dirs(struct bnx2x *bp, u8 *buff)
+{
+	u32 res = 0, ext_cnt, dir_offset = 0x14;
+	struct code_entry entry;
+	int i;
+
+	for (i = 0; i < CODE_ENTRY_EXTENDED_DIR_IDX; i++) {
+		bnx2x_nvram_read(bp, dir_offset + sizeof(entry) * i,
+				 (char *)&entry, sizeof(entry), false);
+		res |= bnx2x_test_nvram_dir(bp, &entry, buff);
+	}
+
+	bnx2x_nvram_read(bp, dir_offset +
+			     sizeof(entry) * CODE_ENTRY_EXTENDED_DIR_IDX,
+			 (char *)&entry, sizeof(entry), false);
+	if ((entry.code_attribute & CODE_IMAGE_TYPE_MASK)
+		!= CODE_IMAGE_TYPE_EXTENDED_DIR ||
+	    (entry.code_attribute & CODE_IMAGE_LENGTH_MASK) == 0)
+		return res;
+
+	/* handle extended dir */
+	bnx2x_nvram_read(bp, entry.nvm_start_addr, (char *)&ext_cnt,
+			 sizeof(u32), false);
+
+	dir_offset = entry.nvm_start_addr + 8;
+	for (i = 0; i < ext_cnt && i < MAX_IMAGES_IN_EXTENDED_DIR; i++) {
+		bnx2x_nvram_read(bp, dir_offset + sizeof(entry) * i,
+				 (char *)&entry, sizeof(entry), false);
+		res |= bnx2x_test_nvram_dir(bp, &entry, buff);
+	}
+
+	return res;
+}
+
+struct crc_pair {
+	int offset;
+	int size;
+};
+
+static int bnx2x_test_nvram_tbl(struct bnx2x *bp,
+				const struct crc_pair *nvram_tbl, u8 *buf)
+{
+	int i;
+
+	for (i = 0; nvram_tbl[i].size; i++) {
+		int res = bnx2x_nvram_crc(bp, nvram_tbl[i].offset,
+					  nvram_tbl[i].size, buf);
+		if (res == -EIO) {
+			DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
+			   "nvram_tbl[%d] unable to read section\n", i);
+			return -EINVAL;
+		} else if (res) {
+			DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
+			   "nvram_tbl[%d] wrong crc value\n", i);
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
 
 static int bnx2x_test_nvram(struct bnx2x *bp)
 {
-	static const struct {
-		int offset;
-		int size;
-	} nvram_tbl[] = {
+	const struct crc_pair nvram_tbl[] = {
 		{     0,  0x14 }, /* bootstrap */
 		{  0x14,  0xec }, /* dir */
 		{ 0x100, 0x350 }, /* manuf_info */
@@ -2595,14 +2714,20 @@ static int bnx2x_test_nvram(struct bnx2x *bp)
 		{ 0x708,  0x70 }, /* manuf_key_info */
 		{     0,     0 }
 	};
+	const struct crc_pair nvram_tbl2[] = {
+		{ 0x7e8, 0x350 }, /* manuf_info2 */
+		{ 0xb38,  0xf0 }, /* feature_info */
+		{     0,     0 }
+	};
+
 	u8 *buf;
-	int i, rc;
-	u32 magic, crc;
+	int rc;
+	u32 magic;
 
 	if (BP_NOMCP(bp))
 		return 0;
 
-	buf = kmalloc(0x350, GFP_KERNEL);
+	buf = kmalloc(CRC_BUFF_SIZE, GFP_KERNEL);
 	if (!buf) {
 		DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM, "kmalloc failed\n");
 		rc = -ENOMEM;
@@ -2620,29 +2745,30 @@ static int bnx2x_test_nvram(struct bnx2x *bp)
 	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++) {
+	DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM, "Port 0 CRC test-set\n");
+	rc = bnx2x_test_nvram_tbl(bp, nvram_tbl, buf);
+	if (rc)
+		goto test_nvram_exit;
 
-		rc = bnx2x_nvram_read(bp, nvram_tbl[i].offset, buf,
-				      nvram_tbl[i].size, true);
-		if (rc) {
-			DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
-			   "nvram_tbl[%d] read data (rc %d)\n", i, rc);
-			goto test_nvram_exit;
-		}
+	if (!CHIP_IS_E1x(bp) && !CHIP_IS_57811xx(bp)) {
+		u32 hide = SHMEM_RD(bp, dev_info.shared_hw_config.config2) &
+			   SHARED_HW_CFG_HIDE_PORT1;
 
-		crc = ether_crc_le(nvram_tbl[i].size, buf);
-		if (crc != CRC32_RESIDUAL) {
+		if (!hide) {
 			DP(BNX2X_MSG_ETHTOOL | BNX2X_MSG_NVM,
-			   "nvram_tbl[%d] wrong crc value (0x%08x)\n", i, crc);
-			rc = -ENODEV;
-			goto test_nvram_exit;
+			   "Port 1 CRC test-set\n");
+			rc = bnx2x_test_nvram_tbl(bp, nvram_tbl2, buf);
+			if (rc)
+				goto test_nvram_exit;
 		}
 	}
 
+	rc = bnx2x_test_nvram_dirs(bp, buf);
+
 test_nvram_exit:
 	kfree(buf);
 	return rc;
-- 
1.7.7.2

[PATCH net-next 3/4] bnx2x: allow nvram test to run when device is down

[Dmitry Kravkov]

Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c    |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index b32a7f0..527fe4d 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -2814,9 +2814,16 @@ static void bnx2x_self_test(struct net_device *dev,
 
 	memset(buf, 0, sizeof(u64) * BNX2X_NUM_TESTS(bp));
 
+	if (bnx2x_test_nvram(bp) != 0) {
+		if (!IS_MF(bp))
+			buf[4] = 1;
+		else
+			buf[0] = 1;
+		etest->flags |= ETH_TEST_FL_FAILED;
+	}
+
 	if (!netif_running(dev)) {
-		DP(BNX2X_MSG_ETHTOOL,
-		   "Can't perform self-test when interface is down\n");
+		DP(BNX2X_MSG_ETHTOOL, "Interface is down\n");
 		return;
 	}
 
@@ -2878,13 +2885,7 @@ static void bnx2x_self_test(struct net_device *dev,
 		/* wait until link state is restored */
 		bnx2x_wait_for_link(bp, link_up, is_serdes);
 	}
-	if (bnx2x_test_nvram(bp) != 0) {
-		if (!IS_MF(bp))
-			buf[4] = 1;
-		else
-			buf[0] = 1;
-		etest->flags |= ETH_TEST_FL_FAILED;
-	}
+
 	if (bnx2x_test_intr(bp) != 0) {
 		if (!IS_MF(bp))
 			buf[5] = 1;
-- 
1.7.7.2

[PATCH net-next 4/4] bnx2x: update version to 1.78.17-0

[Dmitry Kravkov]

Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 87629fd..3dba2a7 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -26,8 +26,8 @@
  * (you will need to reboot afterwards) */
 /* #define BNX2X_STOP_ON_ERROR */
 
-#define DRV_MODULE_VERSION      "1.78.02-0"
-#define DRV_MODULE_RELDATE      "2013/01/14"
+#define DRV_MODULE_VERSION      "1.78.17-0"
+#define DRV_MODULE_RELDATE      "2013/04/11"
 #define BNX2X_BC_VER            0x040200
 
 #if defined(CONFIG_DCB)
-- 
1.7.7.2

Re: [PATCH net-next 1/4] bnx2x: refactor nvram read procedure

[Francois Romieu]

Dmitry Kravkov <dmitry@broadcom.com> :
> introduce a parameter to allow nvram read to return
> data in BE or cpu order.
[...]
> @@ -1295,10 +1296,14 @@ static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, __be32 *ret_val,
>  		if (val & MCPR_NVM_COMMAND_DONE) {
>  			val = REG_RD(bp, MCP_REG_MCPR_NVM_READ);
>  			/* we read nvram data in cpu order
> -			 * but ethtool sees it as an array of bytes
> +			 * but ethtool uses it as an array of bytes
>  			 * converting to big-endian will do the work
> +			 * if requested.

You memcpy a u32 to an array of bytes instead of copying it byte after
byte with proper shift operators and now you are paving the way for more
endianess headaches. I'd rather avoid the memcpy when readying data for
ethtool in the first place.

Nit: the true/false method parameter style in middle layers is mildly
readable when compared to usual _{be/le} kernel style (you should be
able to avoid both almost completely anyway :o) ).

-- 
Ueimor

RE: [PATCH net-next 1/4] bnx2x: refactor nvram read procedure

[Dmitry Kravkov]

> -----Original Message-----
> From: Francois Romieu [mailto:romieu@fr.zoreil.com]
> Sent: Saturday, April 13, 2013 2:09 PM
> To: Dmitry Kravkov
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Eilon Greenstein
> Subject: Re: [PATCH net-next 1/4] bnx2x: refactor nvram read procedure
> 
> Dmitry Kravkov <dmitry@broadcom.com> :
> > introduce a parameter to allow nvram read to return
> > data in BE or cpu order.
> [...]
> > @@ -1295,10 +1296,14 @@ static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, __be32 *ret_val,
> >  		if (val & MCPR_NVM_COMMAND_DONE) {
> >  			val = REG_RD(bp, MCP_REG_MCPR_NVM_READ);
> >  			/* we read nvram data in cpu order
> > -			 * but ethtool sees it as an array of bytes
> > +			 * but ethtool uses it as an array of bytes
> >  			 * converting to big-endian will do the work
> > +			 * if requested.
> 
> You memcpy a u32 to an array of bytes instead of copying it byte after
> byte with proper shift operators and now you are paving the way for more
> endianess headaches. I'd rather avoid the memcpy when readying data for
> ethtool in the first place.
In case of _be data I don't see any issue with copying _be32 it into byte array.
When internal driver function (not exposed to external usage) requests data in CPU order - it meant to bring u32 data ( or array of u32) and memcpy dword by dword also should be harmless.
 
> Nit: the true/false method parameter style in middle layers is mildly
> readable when compared to usual _{be/le} kernel style (you should be
> able to avoid both almost completely anyway :o) ).
Boolean parameter used to share pre- and post-configuring nvram for read operation.
_be/le is less suitable here, since the usage is BE or cpu order.
I was thinking about separating stream read and u32 read to separate callbacks, but this will add another level and this is less preferable than true/false param.
 
> --
> Ueimor

Re: [PATCH net-next 1/4] bnx2x: refactor nvram read procedure

[Francois Romieu]

Dmitry Kravkov <dmitry@broadcom.com> :
[...]
> > You memcpy a u32 to an array of bytes instead of copying it byte after
> > byte with proper shift operators and now you are paving the way for more
> > endianess headaches. I'd rather avoid the memcpy when readying data for
> > ethtool in the first place.
> In case of _be data I don't see any issue with copying _be32 it into byte array.

The modified code (bnx2x_nvram_read) will not be copying __be32 but u32.
It issmuggling _be data behind neutral u32 and using casting when the
kernel has provided cpu_to_{le / be} helpers for years. Think of type
checking as kind of messed up as soon as u32 *ret_val contains a _le or
_be data depending on the value of bool to_be.

I don't want to worry about the endianness of a u32. If it's a u32 (u16,
s32, whatever), I only want to think of bytes in it through '>>' or '<<'
operators. No need to remember the semantic of the last bnx2x_nvram_read_dword
parameter, if it's internal, external, nor assume that the lower layers
enforce some ability to memcpy it blindly. So I'd just favor using cpu order
as soon and as much as possible.

Of course it's your driver, whence your maintenance choices.

-- 
Ueimor

RE: [PATCH net-next 1/4] bnx2x: refactor nvram read procedure

[Dmitry Kravkov]

> -----Original Message-----
> From: Francois Romieu [mailto:romieu@fr.zoreil.com]
> Sent: Tuesday, April 16, 2013 2:32 AM
> To: Dmitry Kravkov
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Eilon Greenstein
> Subject: Re: [PATCH net-next 1/4] bnx2x: refactor nvram read procedure
> 
> Dmitry Kravkov <dmitry@broadcom.com> :
> [...]
> > > You memcpy a u32 to an array of bytes instead of copying it byte after
> > > byte with proper shift operators and now you are paving the way for more
> > > endianess headaches. I'd rather avoid the memcpy when readying data for
> > > ethtool in the first place.
> > In case of _be data I don't see any issue with copying _be32 it into byte array.
> 
> The modified code (bnx2x_nvram_read) will not be copying __be32 but u32.
> It issmuggling _be data behind neutral u32 and using casting when the
> kernel has provided cpu_to_{le / be} helpers for years. Think of type
> checking as kind of messed up as soon as u32 *ret_val contains a _le or
> _be data depending on the value of bool to_be.
> 
> I don't want to worry about the endianness of a u32. If it's a u32 (u16,
> s32, whatever), I only want to think of bytes in it through '>>' or '<<'
> operators. No need to remember the semantic of the last bnx2x_nvram_read_dword
> parameter, if it's internal, external, nor assume that the lower layers
> enforce some ability to memcpy it blindly. So I'd just favor using cpu order
> as soon and as much as possible.
> 
> Of course it's your driver, whence your maintenance choices.

I will re-work it soon! Thanks