[PATCH 0/2] add module EEPROM ops for ice

[PATCH 0/2] add module EEPROM ops for ice

[Steve Yang]

Added the following 2 items of ice_eth_dev_ops for ice:
- ice_get_module_info
- ice_get_module_eeprom

Fixed stack overflow error when displaying a large size info.

Steve Yang (2):
  net/ice: add module EEPROM ops for ice
  app/testpmd: fix stack overflow for EEPROM display

 app/test-pmd/config.c        |  22 ++++-
 drivers/net/ice/ice_ethdev.c | 160 +++++++++++++++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  25 ++++++
 3 files changed, 203 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH 1/2] net/ice: add module EEPROM ops for ice

[Steve Yang]

Add new callbacks for eth_dev_ops of ice to get the information
and data of plugin module EEPROM.

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 160 +++++++++++++++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  25 ++++++
 2 files changed, 185 insertions(+)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 13a7a9702a..70c162d0a8 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -139,6 +139,10 @@ static int ice_vlan_pvid_set(struct rte_eth_dev *dev,
 static int ice_get_eeprom_length(struct rte_eth_dev *dev);
 static int ice_get_eeprom(struct rte_eth_dev *dev,
 			  struct rte_dev_eeprom_info *eeprom);
+static int ice_get_module_info(struct rte_eth_dev *dev,
+			       struct rte_eth_dev_module_info *modinfo);
+static int ice_get_module_eeprom(struct rte_eth_dev *dev,
+				 struct rte_dev_eeprom_info *info);
 static int ice_stats_get(struct rte_eth_dev *dev,
 			 struct rte_eth_stats *stats);
 static int ice_stats_reset(struct rte_eth_dev *dev);
@@ -238,6 +242,8 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
 	.tx_burst_mode_get            = ice_tx_burst_mode_get,
 	.get_eeprom_length            = ice_get_eeprom_length,
 	.get_eeprom                   = ice_get_eeprom,
+	.get_module_info              = ice_get_module_info,
+	.get_module_eeprom            = ice_get_module_eeprom,
 	.stats_get                    = ice_stats_get,
 	.stats_reset                  = ice_stats_reset,
 	.xstats_get                   = ice_xstats_get,
@@ -4934,6 +4940,160 @@ ice_get_eeprom(struct rte_eth_dev *dev,
 	return 0;
 }
 
+static int
+ice_get_module_info(struct rte_eth_dev *dev,
+		    struct rte_eth_dev_module_info *modinfo)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	enum ice_status status;
+	u8 sff8472_comp = 0;
+	u8 sff8472_swap = 0;
+	u8 sff8636_rev = 0;
+	u8 value = 0;
+
+	status = ice_aq_sff_eeprom(hw, 0, ICE_I2C_EEPROM_DEV_ADDR, 0x00, 0x00,
+				   0, &value, 1, 0, NULL);
+	if (status)
+		return -EIO;
+
+	switch (value) {
+	case ICE_MODULE_TYPE_SFP:
+		status = ice_aq_sff_eeprom(hw, 0, ICE_I2C_EEPROM_DEV_ADDR,
+					   ICE_MODULE_SFF_8472_COMP, 0x00, 0,
+					   &sff8472_comp, 1, 0, NULL);
+		if (status)
+			return -EIO;
+		status = ice_aq_sff_eeprom(hw, 0, ICE_I2C_EEPROM_DEV_ADDR,
+					   ICE_MODULE_SFF_8472_SWAP, 0x00, 0,
+					   &sff8472_swap, 1, 0, NULL);
+		if (status)
+			return -EIO;
+
+		if (sff8472_swap & ICE_MODULE_SFF_ADDR_MODE) {
+			modinfo->type = ICE_MODULE_SFF_8079;
+			modinfo->eeprom_len = ICE_MODULE_SFF_8079_LEN;
+		} else if (sff8472_comp &&
+			   (sff8472_swap & ICE_MODULE_SFF_DIAG_CAPAB)) {
+			modinfo->type = ICE_MODULE_SFF_8472;
+			modinfo->eeprom_len = ICE_MODULE_SFF_8472_LEN;
+		} else {
+			modinfo->type = ICE_MODULE_SFF_8079;
+			modinfo->eeprom_len = ICE_MODULE_SFF_8079_LEN;
+		}
+		break;
+	case ICE_MODULE_TYPE_QSFP_PLUS:
+	case ICE_MODULE_TYPE_QSFP28:
+		status = ice_aq_sff_eeprom(hw, 0, ICE_I2C_EEPROM_DEV_ADDR,
+					   ICE_MODULE_REVISION_ADDR, 0x00, 0,
+					   &sff8636_rev, 1, 0, NULL);
+		if (status)
+			return -EIO;
+		/* Check revision compliance */
+		if (sff8636_rev > 0x02) {
+			/* Module is SFF-8636 compliant */
+			modinfo->type = ICE_MODULE_SFF_8636;
+			modinfo->eeprom_len = ICE_MODULE_QSFP_MAX_LEN;
+		} else {
+			modinfo->type = ICE_MODULE_SFF_8436;
+			modinfo->eeprom_len = ICE_MODULE_QSFP_MAX_LEN;
+		}
+		break;
+	default:
+		PMD_DRV_LOG(WARNING, "SFF Module Type not recognized.\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int
+ice_get_module_eeprom(struct rte_eth_dev *dev,
+		      struct rte_dev_eeprom_info *info)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+#define SFF_READ_BLOCK_SIZE 8
+#define I2C_BUSY_TRY_TIMES 4
+#define I2C_USLEEP_MIN_TIME 1500
+#define I2C_USLEEP_MAX_TIME 2500
+	uint8_t value[SFF_READ_BLOCK_SIZE] = {0};
+	uint8_t addr = ICE_I2C_EEPROM_DEV_ADDR;
+	uint8_t *data = info->data;
+	enum ice_status status;
+	bool is_sfp = false;
+	uint32_t i, j;
+	uint32_t offset = 0;
+	uint8_t page = 0;
+
+	if (!info || !info->length || !data)
+		return -EINVAL;
+
+	status = ice_aq_sff_eeprom(hw, 0, addr, offset, page, 0, value, 1, 0,
+				   NULL);
+	if (status)
+		return -EIO;
+
+	if (value[0] == ICE_MODULE_TYPE_SFP)
+		is_sfp = true;
+
+	memset(data, 0, info->length);
+	for (i = 0; i < info->length; i += SFF_READ_BLOCK_SIZE) {
+		offset = i + info->offset;
+		page = 0;
+
+		/* Check if we need to access the other memory page */
+		if (is_sfp) {
+			if (offset >= ICE_MODULE_SFF_8079_LEN) {
+				offset -= ICE_MODULE_SFF_8079_LEN;
+				addr = ICE_I2C_EEPROM_DEV_ADDR2;
+			}
+		} else {
+			while (offset >= ICE_MODULE_SFF_8436_LEN) {
+				/* Compute memory page number and offset. */
+				offset -= ICE_MODULE_SFF_8436_LEN / 2;
+				page++;
+			}
+		}
+
+		/* Bit 2 of eeprom address 0x02 declares upper
+		 * pages are disabled on QSFP modules.
+		 * SFP modules only ever use page 0.
+		 */
+		if (page == 0 || !(data[0x2] & 0x4)) {
+			/* If i2c bus is busy due to slow page change or
+			 * link management access, call can fail.
+			 * This is normal. So we retry this a few times.
+			 */
+			for (j = 0; j < I2C_BUSY_TRY_TIMES; j++) {
+				status = ice_aq_sff_eeprom(hw, 0, addr, offset,
+							   page, !is_sfp, value,
+							   SFF_READ_BLOCK_SIZE,
+							   0, NULL);
+				PMD_DRV_LOG(DEBUG, "SFF %02X %02X %02X %X = "
+					"%02X%02X%02X%02X."
+					"%02X%02X%02X%02X (%X)\n",
+					addr, offset, page, is_sfp,
+					value[0], value[1],
+					value[2], value[3],
+					value[4], value[5],
+					value[6], value[7],
+					status);
+				if (status) {
+					usleep_range(I2C_USLEEP_MIN_TIME,
+						     I2C_USLEEP_MAX_TIME);
+					memset(value, 0, SFF_READ_BLOCK_SIZE);
+					continue;
+				}
+				break;
+			}
+
+			/* Make sure we have enough room for the new block */
+			if ((i + SFF_READ_BLOCK_SIZE) < info->length)
+				memcpy(data + i, value, SFF_READ_BLOCK_SIZE);
+		}
+	}
+
+	return 0;
+}
+
 static void
 ice_stat_update_32(struct ice_hw *hw,
 		   uint32_t reg,
diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
index 1242177b42..3ed580d438 100644
--- a/drivers/net/ice/ice_ethdev.h
+++ b/drivers/net/ice/ice_ethdev.h
@@ -143,6 +143,31 @@
 /* Max number of flexible descriptor rxdid */
 #define ICE_FLEX_DESC_RXDID_MAX_NUM 64
 
+#define ICE_I2C_EEPROM_DEV_ADDR		0xA0
+#define ICE_I2C_EEPROM_DEV_ADDR2	0xA2
+#define ICE_MODULE_TYPE_SFP		0x03
+#define ICE_MODULE_TYPE_QSFP_PLUS	0x0D
+#define ICE_MODULE_TYPE_QSFP28		0x11
+#define ICE_MODULE_SFF_ADDR_MODE	0x04
+#define ICE_MODULE_SFF_DIAG_CAPAB	0x40
+#define ICE_MODULE_REVISION_ADDR	0x01
+#define ICE_MODULE_SFF_8472_COMP	0x5E
+#define ICE_MODULE_SFF_8472_SWAP	0x5C
+#define ICE_MODULE_QSFP_MAX_LEN		640
+
+/* EEPROM Standards for plug in modules */
+#define ICE_MODULE_SFF_8079		0x1
+#define ICE_MODULE_SFF_8079_LEN		256
+#define ICE_MODULE_SFF_8472		0x2
+#define ICE_MODULE_SFF_8472_LEN		512
+#define ICE_MODULE_SFF_8636		0x3
+#define ICE_MODULE_SFF_8636_LEN		256
+#define ICE_MODULE_SFF_8636_MAX_LEN     640
+#define ICE_MODULE_SFF_8436		0x4
+#define ICE_MODULE_SFF_8436_LEN		256
+#define ICE_MODULE_SFF_8436_MAX_LEN     640
+
+
 /* Per-channel register definitions */
 #define GLTSYN_AUX_OUT(_chan, _idx)     (GLTSYN_AUX_OUT_0(_idx) + ((_chan) * 8))
 #define GLTSYN_CLKO(_chan, _idx)        (GLTSYN_CLKO_0(_idx) + ((_chan) * 8))
-- 
2.27.0

[PATCH 2/2] app/testpmd: fix stack overflow for EEPROM display

[Steve Yang]

When the size of EEPROM exceeds the default thread stack size(8MB),
e.g.: 10Mb size, it will be cashed with stack overflow.

Allocate the data of EPPROM information on the heap.

Fixes: 6b67721dee2a ("app/testpmd: add EEPROM command")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 app/test-pmd/config.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1722d6c8f8..e812f57151 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -912,10 +912,15 @@ port_eeprom_display(portid_t port_id)
 		return;
 	}
 
-	char buf[len_eeprom];
 	einfo.offset = 0;
 	einfo.length = len_eeprom;
-	einfo.data = buf;
+	einfo.data = calloc(1, len_eeprom);
+	if (!einfo.data) {
+		fprintf(stderr,
+			"Allocation of port %u eeprom data failed\n",
+			port_id);
+		return;
+	}
 
 	ret = rte_eth_dev_get_eeprom(port_id, &einfo);
 	if (ret != 0) {
@@ -933,10 +938,12 @@ port_eeprom_display(portid_t port_id)
 			fprintf(stderr, "Unable to get EEPROM: %d\n", ret);
 			break;
 		}
+		free(einfo.data);
 		return;
 	}
 	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
 	printf("Finish -- Port: %d EEPROM length: %d bytes\n", port_id, len_eeprom);
+	free(einfo.data);
 }
 
 void
@@ -972,10 +979,15 @@ port_module_eeprom_display(portid_t port_id)
 		return;
 	}
 
-	char buf[minfo.eeprom_len];
 	einfo.offset = 0;
 	einfo.length = minfo.eeprom_len;
-	einfo.data = buf;
+	einfo.data = calloc(1, minfo.eeprom_len);
+	if (!einfo.data) {
+		fprintf(stderr,
+			"Allocation of port %u eeprom data failed\n",
+			port_id);
+		return;
+	}
 
 	ret = rte_eth_dev_get_module_eeprom(port_id, &einfo);
 	if (ret != 0) {
@@ -994,11 +1006,13 @@ port_module_eeprom_display(portid_t port_id)
 				ret);
 			break;
 		}
+		free(einfo.data);
 		return;
 	}
 
 	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
 	printf("Finish -- Port: %d MODULE EEPROM length: %d bytes\n", port_id, einfo.length);
+	free(einfo.data);
 }
 
 int
-- 
2.27.0

RE: [PATCH 1/2] net/ice: add module EEPROM ops for ice

[Zhang, Qi Z]

> -----Original Message-----
> From: Yang, SteveX <stevex.yang@intel.com>
> Sent: Thursday, January 20, 2022 11:00 AM
> To: dev@dpdk.org
> Cc: Zhang, Yuying <yuying.zhang@intel.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>; Yang, SteveX <stevex.yang@intel.com>
> Subject: [PATCH 1/2] net/ice: add module EEPROM ops for ice
> 
> Add new callbacks for eth_dev_ops of ice to get the information and data of
> plugin module EEPROM.
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

Re: [PATCH 1/2] net/ice: add module EEPROM ops for ice

[Ferruh Yigit]

On 1/20/2022 2:59 AM, Steve Yang wrote:
> Add new callbacks for eth_dev_ops of ice to get the information
> and data of plugin module EEPROM.
> 
> Signed-off-by: Steve Yang<stevex.yang@intel.com>
> ---
>   drivers/net/ice/ice_ethdev.c | 160 +++++++++++++++++++++++++++++++++++
>   drivers/net/ice/ice_ethdev.h |  25 ++++++
>   2 files changed, 185 insertions(+)

In the 'doc/guides/nics/features/ice.ini' file, the "Module EEPROM dump"
feature already documented as supported, but it is this patch add that
support.

It looks like documenting "Module EEPROM dump" was mistake [1], intention
was to document "EEPROM dump" feature.

Steve, can you please send another patch to fix it?


[1]
Fixes: d0dd1c8e1997 ("net/ice: support EEPROM information getting")



Thanks,
ferruh

RE: [PATCH 1/2] net/ice: add module EEPROM ops for ice

[Yang, SteveX]

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Tuesday, January 25, 2022 10:49 PM
> To: Yang, SteveX <stevex.yang@intel.com>; dev@dpdk.org
> Cc: Zhang, Yuying <yuying.zhang@intel.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [PATCH 1/2] net/ice: add module EEPROM ops for ice
> 
> On 1/20/2022 2:59 AM, Steve Yang wrote:
> > Add new callbacks for eth_dev_ops of ice to get the information and
> > data of plugin module EEPROM.
> >
> > Signed-off-by: Steve Yang<stevex.yang@intel.com>
> > ---
> >   drivers/net/ice/ice_ethdev.c | 160
> +++++++++++++++++++++++++++++++++++
> >   drivers/net/ice/ice_ethdev.h |  25 ++++++
> >   2 files changed, 185 insertions(+)
> 
> In the 'doc/guides/nics/features/ice.ini' file, the "Module EEPROM dump"
> feature already documented as supported, but it is this patch add that
> support.
> 
> It looks like documenting "Module EEPROM dump" was mistake [1],
> intention was to document "EEPROM dump" feature.
> 
> Steve, can you please send another patch to fix it?

Ok, I'll add "EEPROM dump" line into the ice.ini to fix it within another patch later.
Thanks.

> 
> 
> [1]
> Fixes: d0dd1c8e1997 ("net/ice: support EEPROM information getting")
> 
> 
> 
> Thanks,
> ferruh

Re: [PATCH 2/2] app/testpmd: fix stack overflow for EEPROM display

[Singh, Aman Deep]

[-- Attachment #1: Type: text/plain, Size: 2388 bytes --]

Hi Steve
The patch looks ok to me.

On 1/20/2022 8:29 AM, Steve Yang wrote:
> When the size of EEPROM exceeds the default thread stack size(8MB),
> e.g.: 10Mb size, it will be cashed with stack overflow.
Just spelling: 10Mb/10MB, cashed/crashed
Can even rephrase, like: it will crash due to stack overflow
> Allocate the data of EPPROM information on the heap.
>
> Fixes: 6b67721dee2a ("app/testpmd: add EEPROM command")
>
> Signed-off-by: Steve Yang<stevex.yang@intel.com>

Acked-by: Aman Singh <aman.deep.singh@intel.com>

> ---
>   app/test-pmd/config.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 1722d6c8f8..e812f57151 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -912,10 +912,15 @@ port_eeprom_display(portid_t port_id)
>   		return;
>   	}
>   
> -	char buf[len_eeprom];
>   	einfo.offset = 0;
>   	einfo.length = len_eeprom;
> -	einfo.data = buf;
> +	einfo.data = calloc(1, len_eeprom);
> +	if (!einfo.data) {
> +		fprintf(stderr,
> +			"Allocation of port %u eeprom data failed\n",
> +			port_id);
> +		return;
> +	}
>   
>   	ret = rte_eth_dev_get_eeprom(port_id, &einfo);
>   	if (ret != 0) {
> @@ -933,10 +938,12 @@ port_eeprom_display(portid_t port_id)
>   			fprintf(stderr, "Unable to get EEPROM: %d\n", ret);
>   			break;
>   		}
> +		free(einfo.data);
>   		return;
>   	}
>   	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
>   	printf("Finish -- Port: %d EEPROM length: %d bytes\n", port_id, len_eeprom); + free(einfo.data); } void @@ -972,10 +979,15 
> @@ port_module_eeprom_display(portid_t port_id) return; } - char 
> buf[minfo.eeprom_len]; einfo.offset = 0; einfo.length = 
> minfo.eeprom_len; - einfo.data = buf; + einfo.data = calloc(1, 
> minfo.eeprom_len); + if (!einfo.data) { + fprintf(stderr, + "Allocation of port %u eeprom data failed\n",
> +			port_id);
> +		return;
> +	}
>   
>   	ret = rte_eth_dev_get_module_eeprom(port_id, &einfo);
>   	if (ret != 0) {
> @@ -994,11 +1006,13 @@ port_module_eeprom_display(portid_t port_id)
>   				ret);
>   			break;
>   		}
> +		free(einfo.data);
>   		return;
>   	}
>   
>   	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
>   	printf("Finish -- Port: %d MODULE EEPROM length: %d bytes\n", port_id, einfo.length);
> +	free(einfo.data);
>   }
>   
>   int

[-- Attachment #2: Type: text/html, Size: 3661 bytes --]

Re: [PATCH 2/2] app/testpmd: fix stack overflow for EEPROM display

[Ferruh Yigit]

On 2/2/2022 10:00 AM, Singh, Aman Deep wrote:
> Hi Steve
> The patch looks ok to me.
> 
> On 1/20/2022 8:29 AM, Steve Yang wrote:
>> When the size of EEPROM exceeds the default thread stack size(8MB),
>> e.g.: 10Mb size, it will be cashed with stack overflow.
> Just spelling: 10Mb/10MB, cashed/crashed
> Can even rephrase, like: it will crash due to stack overflow

fixed while merging

>> Allocate the data of EPPROM information on the heap.
>>
>> Fixes: 6b67721dee2a ("app/testpmd: add EEPROM command")
>>
>> Signed-off-by: Steve Yang<stevex.yang@intel.com>
> 
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
> 

Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.