[PATCH] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[PATCH] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[jeffd]

From: Stephen Douthit <stephend@silicom-usa.com>

Some XGS-PON SFPs have been observed ACKing I2C reads and returning
uninitialized garbage while their uC boots.  This can lead to the SFP ID
code marking an otherwise working SFP module as unsupported if a bogus
ID value is read while its internal PHY/microcontroller is still
booting.

Retry the ID read several times looking not just for NAK, but also for a
valid ID field.

Since the device isn't NAKing the trasanction the existing longer retry
code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.

Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
---
 drivers/net/ixgbe/base/ixgbe_phy.c | 31 ++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
index d8d51d2c3f..27bce066a1 100644
--- a/drivers/net/ixgbe/base/ixgbe_phy.c
+++ b/drivers/net/ixgbe/base/ixgbe_phy.c
@@ -1275,6 +1275,7 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
 	u8 cable_tech = 0;
 	u8 cable_spec = 0;
 	u16 enforce_sfp = 0;
+	u8 id_reads;
 
 	DEBUGFUNC("ixgbe_identify_sfp_module_generic");
 
@@ -1287,12 +1288,34 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
 	/* LAN ID is needed for I2C access */
 	hw->mac.ops.set_lan_id(hw);
 
-	status = hw->phy.ops.read_i2c_eeprom(hw,
-					     IXGBE_SFF_IDENTIFIER,
-					     &identifier);
+	/* Need to check this a couple of times for a sane value.
+	 *
+	 * SFPs that have a uC slaved to the I2C bus (vs. a dumb EEPROM) can be
+	 * poorly designed such that they will ACK I2C reads and return
+	 * whatever bogus data is in the SRAM (or whatever is backing the target
+	 * device) before things are truly initialized.
+	 *
+	 * In a perfect world devices would NAK I2C requests until they were
+	 * sane, but here we are.
+	 *
+	 * Give such devices a couple tries to get their act together before
+	 * marking the device as unsupported.
+	 */
+	for (id_reads = 0; id_reads < 5; id_reads++) {
+		status = hw->phy.ops.read_i2c_eeprom(hw,
+						     IXGBE_SFF_IDENTIFIER,
+						     &identifier);
 
-	if (status != IXGBE_SUCCESS)
+		DEBUGOUT("status %d, id %d\n", status, identifier);
+		if (!status &&
+		    identifier == IXGBE_SFF_IDENTIFIER_SFP)
+			break;
+	}
+
+	if (status != IXGBE_SUCCESS) {
+		DEBUGOUT("Failed SFF ID read (%d attempts)\n", id_reads);
 		goto err_read_i2c_eeprom;
+	}
 
 	if (identifier != IXGBE_SFF_IDENTIFIER_SFP) {
 		hw->phy.type = ixgbe_phy_sfp_unsupported;
-- 
2.25.1

RE: [PATCH] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[Wang, Haiyue]

> -----Original Message-----
> From: jeffd@silicom-usa.com <jeffd@silicom-usa.com>
> Sent: Tuesday, March 8, 2022 06:34
> To: dev@dpdk.org
> Cc: Stephen Douthit <stephend@silicom-usa.com>; Daly, Jeff <jeffd@silicom-usa.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: [PATCH] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs
> 
> From: Stephen Douthit <stephend@silicom-usa.com>
> 
> Some XGS-PON SFPs have been observed ACKing I2C reads and returning
> uninitialized garbage while their uC boots.  This can lead to the SFP ID
> code marking an otherwise working SFP module as unsupported if a bogus
> ID value is read while its internal PHY/microcontroller is still
> booting.
> 
> Retry the ID read several times looking not just for NAK, but also for a
> valid ID field.
> 
> Since the device isn't NAKing the trasanction the existing longer retry
> code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.
> 
> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> ---
>  drivers/net/ixgbe/base/ixgbe_phy.c | 31 ++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
> index d8d51d2c3f..27bce066a1 100644
> --- a/drivers/net/ixgbe/base/ixgbe_phy.c
> +++ b/drivers/net/ixgbe/base/ixgbe_phy.c
> @@ -1275,6 +1275,7 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
>  	u8 cable_tech = 0;
>  	u8 cable_spec = 0;
>  	u16 enforce_sfp = 0;
> +	u8 id_reads;

"u8 retries" is a better name, I think, to match the patch title.

> 
>  	DEBUGFUNC("ixgbe_identify_sfp_module_generic");
> 
> @@ -1287,12 +1288,34 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
>  	/* LAN ID is needed for I2C access */
>  	hw->mac.ops.set_lan_id(hw);
> 
> -	status = hw->phy.ops.read_i2c_eeprom(hw,
> -					     IXGBE_SFF_IDENTIFIER,
> -					     &identifier);
> +	/* Need to check this a couple of times for a sane value.
> +	 *
> +	 * SFPs that have a uC slaved to the I2C bus (vs. a dumb EEPROM) can be
> +	 * poorly designed such that they will ACK I2C reads and return
> +	 * whatever bogus data is in the SRAM (or whatever is backing the target
> +	 * device) before things are truly initialized.
> +	 *
> +	 * In a perfect world devices would NAK I2C requests until they were
> +	 * sane, but here we are.
> +	 *
> +	 * Give such devices a couple tries to get their act together before
> +	 * marking the device as unsupported.
> +	 */
> +	for (id_reads = 0; id_reads < 5; id_reads++) {
> +		status = hw->phy.ops.read_i2c_eeprom(hw,
> +						     IXGBE_SFF_IDENTIFIER,
> +						     &identifier);
> 
> -	if (status != IXGBE_SUCCESS)
> +		DEBUGOUT("status %d, id %d\n", status, identifier);
> +		if (!status &&
> +		    identifier == IXGBE_SFF_IDENTIFIER_SFP)

Let's not assume the ' IXGBE_SUCCESS' is '0', so change it to:

		if (status == IXGBE_SUCCESS &&
                identifier == IXGBE_SFF_IDENTIFIER_SFP)

> +			break;
> +	}
> +
> +	if (status != IXGBE_SUCCESS) {
> +		DEBUGOUT("Failed SFF ID read (%d attempts)\n", id_reads);
>  		goto err_read_i2c_eeprom;
> +	}
> 
>  	if (identifier != IXGBE_SFF_IDENTIFIER_SFP) {
>  		hw->phy.type = ixgbe_phy_sfp_unsupported;
> --
> 2.25.1

RE: [PATCH] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[Wang, Haiyue]

> -----Original Message-----
> From: jeffd@silicom-usa.com <jeffd@silicom-usa.com>
> Sent: Tuesday, March 8, 2022 06:34
> To: dev@dpdk.org
> Cc: Stephen Douthit <stephend@silicom-usa.com>; Daly, Jeff <jeffd@silicom-usa.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: [PATCH] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs
> 
> From: Stephen Douthit <stephend@silicom-usa.com>
> 
> Some XGS-PON SFPs have been observed ACKing I2C reads and returning
> uninitialized garbage while their uC boots.  This can lead to the SFP ID
> code marking an otherwise working SFP module as unsupported if a bogus
> ID value is read while its internal PHY/microcontroller is still
> booting.
> 
> Retry the ID read several times looking not just for NAK, but also for a
> valid ID field.
> 
> Since the device isn't NAKing the trasanction the existing longer retry
> code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.
> 
> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> ---
>  drivers/net/ixgbe/base/ixgbe_phy.c | 31 ++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
> index d8d51d2c3f..27bce066a1 100644
> --- a/drivers/net/ixgbe/base/ixgbe_phy.c
> +++ b/drivers/net/ixgbe/base/ixgbe_phy.c
> @@ -1275,6 +1275,7 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)


> +	for (id_reads = 0; id_reads < 5; id_reads++) {
> +		status = hw->phy.ops.read_i2c_eeprom(hw,
> +						     IXGBE_SFF_IDENTIFIER,
> +						     &identifier);
> 
> -	if (status != IXGBE_SUCCESS)
> +		DEBUGOUT("status %d, id %d\n", status, identifier);

DEBUGOUT("status %d, SFF identifier 0x%x\n", status, identifier);

Since "#define IXGBE_SFF_IDENTIFIER_SFP	0x3" is hex value, and the ID is too simple.

> +		if (!status &&
> +		    identifier == IXGBE_SFF_IDENTIFIER_SFP)
> +			break;
> +	}
> +
> +	if (status != IXGBE_SUCCESS) {
> +		DEBUGOUT("Failed SFF ID read (%d attempts)\n", id_reads);
>  		goto err_read_i2c_eeprom;
> +	}

This DEBUGOUT can be removed, it's redundant, every call for hw->phy.ops.read_i2c_eeprom
has added the DEBUOUT.

> 
>  	if (identifier != IXGBE_SFF_IDENTIFIER_SFP) {
>  		hw->phy.type = ixgbe_phy_sfp_unsupported;
> --
> 2.25.1

[PATCH v2] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[jeffd]

From: Stephen Douthit <stephend@silicom-usa.com>

Some XGS-PON SFPs have been observed ACKing I2C reads and returning
uninitialized garbage while their uC boots.  This can lead to the SFP ID
code marking an otherwise working SFP module as unsupported if a bogus
ID value is read while its internal PHY/microcontroller is still
booting.

Retry the ID read several times looking not just for NAK, but also for a
valid ID field.

Since the device isn't NAKing the trasanction the existing longer retry
code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.

Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
---
v2:
* Removed superfluous DEBUGOUT
* Renamed id_reads to retries
* Don't assume status == 0 means IXGBE_SUCCESS

 drivers/net/ixgbe/base/ixgbe_phy.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
index 8d4d9bbfef..657f404fe8 100644
--- a/drivers/net/ixgbe/base/ixgbe_phy.c
+++ b/drivers/net/ixgbe/base/ixgbe_phy.c
@@ -1267,6 +1267,7 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
 	u8 cable_tech = 0;
 	u8 cable_spec = 0;
 	u16 enforce_sfp = 0;
+	u8 retries;
 
 	DEBUGFUNC("ixgbe_identify_sfp_module_generic");
 
@@ -1279,12 +1280,33 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
 	/* LAN ID is needed for I2C access */
 	hw->mac.ops.set_lan_id(hw);
 
-	status = hw->phy.ops.read_i2c_eeprom(hw,
-					     IXGBE_SFF_IDENTIFIER,
-					     &identifier);
+	/* Need to check this a couple of times for a sane value.
+	 *
+	 * SFPs that have a uC slaved to the I2C bus (vs. a dumb EEPROM) can be
+	 * poorly designed such that they will ACK I2C reads and return
+	 * whatever bogus data is in the SRAM (or whatever is backing the target
+	 * device) before things are truly initialized.
+	 *
+	 * In a perfect world devices would NAK I2C requests until they were
+	 * sane, but here we are.
+	 *
+	 * Give such devices a couple tries to get their act together before
+	 * marking the device as unsupported.
+	 */
+	for (retries = 0; retries < 5; retries++) {
+		status = hw->phy.ops.read_i2c_eeprom(hw,
+						     IXGBE_SFF_IDENTIFIER,
+						     &identifier);
 
-	if (status != IXGBE_SUCCESS)
+		DEBUGOUT("status %d, SFF identifier 0x%x\n", status, identifier);
+		if (status == IXGBE_SUCCESS &&
+		identifier == IXGBE_SFF_IDENTIFIER_SFP)
+			break;
+	}
+
+	if (status != IXGBE_SUCCESS) {
 		goto err_read_i2c_eeprom;
+	}
 
 	if (identifier != IXGBE_SFF_IDENTIFIER_SFP) {
 		hw->phy.type = ixgbe_phy_sfp_unsupported;
-- 
2.25.1

RE: [PATCH v2] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[Wang, Haiyue]

> -----Original Message-----
> From: jeffd@silicom-usa.com <jeffd@silicom-usa.com>
> Sent: Tuesday, March 22, 2022 23:24
> To: dev@dpdk.org
> Cc: Stephen Douthit <stephend@silicom-usa.com>; Daly, Jeff <jeffd@silicom-usa.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: [PATCH v2] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs
> 
> From: Stephen Douthit <stephend@silicom-usa.com>
> 
> Some XGS-PON SFPs have been observed ACKing I2C reads and returning
> uninitialized garbage while their uC boots.  This can lead to the SFP ID
> code marking an otherwise working SFP module as unsupported if a bogus
> ID value is read while its internal PHY/microcontroller is still
> booting.
> 
> Retry the ID read several times looking not just for NAK, but also for a
> valid ID field.
> 
> Since the device isn't NAKing the trasanction the existing longer retry
> code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.
> 
> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> ---
> v2:
> * Removed superfluous DEBUGOUT
> * Renamed id_reads to retries
> * Don't assume status == 0 means IXGBE_SUCCESS
> 
>  drivers/net/ixgbe/base/ixgbe_phy.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
> index 8d4d9bbfef..657f404fe8 100644
> --- a/drivers/net/ixgbe/base/ixgbe_phy.c
> +++ b/drivers/net/ixgbe/base/ixgbe_phy.c
> @@ -1267,6 +1267,7 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
>  	u8 cable_tech = 0;
>  	u8 cable_spec = 0;
>  	u16 enforce_sfp = 0;
> +	u8 retries;
> 
>  	DEBUGFUNC("ixgbe_identify_sfp_module_generic");
> 
> @@ -1279,12 +1280,33 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
>  	/* LAN ID is needed for I2C access */
>  	hw->mac.ops.set_lan_id(hw);
> 
> -	status = hw->phy.ops.read_i2c_eeprom(hw,
> -					     IXGBE_SFF_IDENTIFIER,
> -					     &identifier);
> +	/* Need to check this a couple of times for a sane value.
> +	 *
> +	 * SFPs that have a uC slaved to the I2C bus (vs. a dumb EEPROM) can be
> +	 * poorly designed such that they will ACK I2C reads and return
> +	 * whatever bogus data is in the SRAM (or whatever is backing the target
> +	 * device) before things are truly initialized.
> +	 *
> +	 * In a perfect world devices would NAK I2C requests until they were
> +	 * sane, but here we are.
> +	 *
> +	 * Give such devices a couple tries to get their act together before
> +	 * marking the device as unsupported.
> +	 */
> +	for (retries = 0; retries < 5; retries++) {
> +		status = hw->phy.ops.read_i2c_eeprom(hw,
> +						     IXGBE_SFF_IDENTIFIER,
> +						     &identifier);
> 
> -	if (status != IXGBE_SUCCESS)
> +		DEBUGOUT("status %d, SFF identifier 0x%x\n", status, identifier);
> +		if (status == IXGBE_SUCCESS &&
> +		identifier == IXGBE_SFF_IDENTIFIER_SFP)
> +			break;
> +	}
> +
> +	if (status != IXGBE_SUCCESS) {
>  		goto err_read_i2c_eeprom;
> +	}
> 

Just one line, no need {}

	if (status != IXGBE_SUCCESS)
		goto err_read_i2c_eeprom;

>  	if (identifier != IXGBE_SFF_IDENTIFIER_SFP) {
>  		hw->phy.type = ixgbe_phy_sfp_unsupported;
> --
> 2.25.1

[PATCH v3] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[jeffd]

From: Stephen Douthit <stephend@silicom-usa.com>

Some XGS-PON SFPs have been observed ACKing I2C reads and returning
uninitialized garbage while their uC boots.  This can lead to the SFP ID
code marking an otherwise working SFP module as unsupported if a bogus
ID value is read while its internal PHY/microcontroller is still
booting.

Retry the ID read several times looking not just for NAK, but also for a
valid ID field.

Since the device isn't NAKing the trasanction the existing longer retry
code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.

Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
---

Notes:
    v2:
    * Removed superfluous DEBUGOUT
    * Renamed id_reads to retries
    * Don't assume status == 0 means IXGBE_SUCCESS
    
    v3:
    * Removed extra braces around single statement if

 drivers/net/ixgbe/base/ixgbe_phy.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
index 8d4d9bbfef..74c5db16fa 100644
--- a/drivers/net/ixgbe/base/ixgbe_phy.c
+++ b/drivers/net/ixgbe/base/ixgbe_phy.c
@@ -1267,6 +1267,7 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
 	u8 cable_tech = 0;
 	u8 cable_spec = 0;
 	u16 enforce_sfp = 0;
+	u8 retries;
 
 	DEBUGFUNC("ixgbe_identify_sfp_module_generic");
 
@@ -1279,9 +1280,29 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
 	/* LAN ID is needed for I2C access */
 	hw->mac.ops.set_lan_id(hw);
 
-	status = hw->phy.ops.read_i2c_eeprom(hw,
-					     IXGBE_SFF_IDENTIFIER,
-					     &identifier);
+	/* Need to check this a couple of times for a sane value.
+	 *
+	 * SFPs that have a uC slaved to the I2C bus (vs. a dumb EEPROM) can be
+	 * poorly designed such that they will ACK I2C reads and return
+	 * whatever bogus data is in the SRAM (or whatever is backing the target
+	 * device) before things are truly initialized.
+	 *
+	 * In a perfect world devices would NAK I2C requests until they were
+	 * sane, but here we are.
+	 *
+	 * Give such devices a couple tries to get their act together before
+	 * marking the device as unsupported.
+	 */
+	for (retries = 0; retries < 5; retries++) {
+		status = hw->phy.ops.read_i2c_eeprom(hw,
+						     IXGBE_SFF_IDENTIFIER,
+						     &identifier);
+
+		DEBUGOUT("status %d, SFF identifier 0x%x\n", status, identifier);
+		if (status == IXGBE_SUCCESS &&
+		identifier == IXGBE_SFF_IDENTIFIER_SFP)
+			break;
+	}
 
 	if (status != IXGBE_SUCCESS)
 		goto err_read_i2c_eeprom;
-- 
2.25.1

RE: [PATCH v3] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[Wang, Haiyue]

> -----Original Message-----
> From: jeffd@silicom-usa.com <jeffd@silicom-usa.com>
> Sent: Thursday, March 24, 2022 04:04
> To: dev@dpdk.org
> Cc: Stephen Douthit <stephend@silicom-usa.com>; Daly, Jeff <jeffd@silicom-usa.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: [PATCH v3] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs
> 
> From: Stephen Douthit <stephend@silicom-usa.com>
> 
> Some XGS-PON SFPs have been observed ACKing I2C reads and returning
> uninitialized garbage while their uC boots.  This can lead to the SFP ID
> code marking an otherwise working SFP module as unsupported if a bogus
> ID value is read while its internal PHY/microcontroller is still
> booting.
> 
> Retry the ID read several times looking not just for NAK, but also for a
> valid ID field.
> 
> Since the device isn't NAKing the trasanction the existing longer retry
> code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.
> 
> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> ---
> 
> Notes:
>     v2:
>     * Removed superfluous DEBUGOUT
>     * Renamed id_reads to retries
>     * Don't assume status == 0 means IXGBE_SUCCESS
> 
>     v3:
>     * Removed extra braces around single statement if
> 
>  drivers/net/ixgbe/base/ixgbe_phy.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 

Thanks!

Reviewed-by: Haiyue Wang <haiyue.wang@intel.com>

> --
> 2.25.1

RE: [PATCH v3] net/ixgbe: Retry SFP ID read field to handle misbehaving SFPs

[Zhang, Qi Z]

> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Thursday, March 24, 2022 9:01 AM
> To: Daly, Jeff <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: Stephen Douthit <stephend@silicom-usa.com>; Daly, Jeff
> <jeffd@silicom-usa.com>
> Subject: RE: [PATCH v3] net/ixgbe: Retry SFP ID read field to handle
> misbehaving SFPs

Slightly reword the title as below to fix check-git-log warning

net/ixgbe: retry to handle misbehaving SFPs read


> 
> > -----Original Message-----
> > From: jeffd@silicom-usa.com <jeffd@silicom-usa.com>
> > Sent: Thursday, March 24, 2022 04:04
> > To: dev@dpdk.org
> > Cc: Stephen Douthit <stephend@silicom-usa.com>; Daly, Jeff
> > <jeffd@silicom-usa.com>; Wang, Haiyue <haiyue.wang@intel.com>
> > Subject: [PATCH v3] net/ixgbe: Retry SFP ID read field to handle
> > misbehaving SFPs
> >
> > From: Stephen Douthit <stephend@silicom-usa.com>
> >
> > Some XGS-PON SFPs have been observed ACKing I2C reads and returning
> > uninitialized garbage while their uC boots.  This can lead to the SFP
> > ID code marking an otherwise working SFP module as unsupported if a
> > bogus ID value is read while its internal PHY/microcontroller is still
> > booting.
> >
> > Retry the ID read several times looking not just for NAK, but also for
> > a valid ID field.
> >
> > Since the device isn't NAKing the trasanction the existing longer
> > retry code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.
> >
> > Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > ---
> >
> > Notes:
> >     v2:
> >     * Removed superfluous DEBUGOUT
> >     * Renamed id_reads to retries
> >     * Don't assume status == 0 means IXGBE_SUCCESS
> >
> >     v3:
> >     * Removed extra braces around single statement if
> >
> >  drivers/net/ixgbe/base/ixgbe_phy.c | 27 ++++++++++++++++++++++++--
> -
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> >
> 
> Thanks!
> 
> Reviewed-by: Haiyue Wang <haiyue.wang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
> 
> > --
> > 2.25.1

[PATCH v4] net/ixgbe: retry SFP ID read to handle misbehaving SFPs

[jeffd]

From: Stephen Douthit <stephend@silicom-usa.com>

Some XGS-PON SFPs have been observed ACKing I2C reads and returning
uninitialized garbage while their uC boots.  This can lead to the SFP ID
code marking an otherwise working SFP module as unsupported if a bogus
ID value is read while its internal PHY/microcontroller is still
booting.

Retry the ID read several times looking not just for NAK, but also for a
valid ID field.

Since the device isn't NAKing the trasanction the existing longer retry
code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.

Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
---

Notes:
    v4:
    * Fixed git summary
    
    v3:
    * Removed extra braces around single statement if
    
    v2:
    * Removed superfluous DEBUGOUT
    * Renamed id_reads to retries
    * Don't assume status == 0 means IXGBE_SUCCESS

 drivers/net/ixgbe/base/ixgbe_phy.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
index 8d4d9bbfef..74c5db16fa 100644
--- a/drivers/net/ixgbe/base/ixgbe_phy.c
+++ b/drivers/net/ixgbe/base/ixgbe_phy.c
@@ -1267,6 +1267,7 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
 	u8 cable_tech = 0;
 	u8 cable_spec = 0;
 	u16 enforce_sfp = 0;
+	u8 retries;
 
 	DEBUGFUNC("ixgbe_identify_sfp_module_generic");
 
@@ -1279,9 +1280,29 @@ s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
 	/* LAN ID is needed for I2C access */
 	hw->mac.ops.set_lan_id(hw);
 
-	status = hw->phy.ops.read_i2c_eeprom(hw,
-					     IXGBE_SFF_IDENTIFIER,
-					     &identifier);
+	/* Need to check this a couple of times for a sane value.
+	 *
+	 * SFPs that have a uC slaved to the I2C bus (vs. a dumb EEPROM) can be
+	 * poorly designed such that they will ACK I2C reads and return
+	 * whatever bogus data is in the SRAM (or whatever is backing the target
+	 * device) before things are truly initialized.
+	 *
+	 * In a perfect world devices would NAK I2C requests until they were
+	 * sane, but here we are.
+	 *
+	 * Give such devices a couple tries to get their act together before
+	 * marking the device as unsupported.
+	 */
+	for (retries = 0; retries < 5; retries++) {
+		status = hw->phy.ops.read_i2c_eeprom(hw,
+						     IXGBE_SFF_IDENTIFIER,
+						     &identifier);
+
+		DEBUGOUT("status %d, SFF identifier 0x%x\n", status, identifier);
+		if (status == IXGBE_SUCCESS &&
+		identifier == IXGBE_SFF_IDENTIFIER_SFP)
+			break;
+	}
 
 	if (status != IXGBE_SUCCESS)
 		goto err_read_i2c_eeprom;
-- 
2.25.1

RE: [PATCH v4] net/ixgbe: retry SFP ID read to handle misbehaving SFPs

[Wang, Haiyue]

> -----Original Message-----
> From: jeffd@silicom-usa.com <jeffd@silicom-usa.com>
> Sent: Friday, March 25, 2022 17:54
> To: dev@dpdk.org
> Cc: Stephen Douthit <stephend@silicom-usa.com>; Daly, Jeff <jeffd@silicom-usa.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: [PATCH v4] net/ixgbe: retry SFP ID read to handle misbehaving SFPs
> 
> From: Stephen Douthit <stephend@silicom-usa.com>
> 
> Some XGS-PON SFPs have been observed ACKing I2C reads and returning
> uninitialized garbage while their uC boots.  This can lead to the SFP ID
> code marking an otherwise working SFP module as unsupported if a bogus
> ID value is read while its internal PHY/microcontroller is still
> booting.
> 
> Retry the ID read several times looking not just for NAK, but also for a
> valid ID field.
> 
> Since the device isn't NAKing the trasanction the existing longer retry
> code in ixgbe_read_i2c_byte_generic_int() doesn't apply here.
> 
> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> ---
> 
> Notes:
>     v4:
>     * Fixed git summary
> 

No need v4, have been merged by Qi. ; - )

https://git.dpdk.org/next/dpdk-next-net-intel/commit/?id=2f010a904cb06277d8710bad16ebfe9a38f61b62

>     v3:
>     * Removed extra braces around single statement if
> 
>     v2:
>     * Removed superfluous DEBUGOUT
>     * Renamed id_reads to retries
>     * Don't assume status == 0 means IXGBE_SUCCESS
> 
>  drivers/net/ixgbe/base/ixgbe_phy.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 



> --
> 2.25.1