> -----Original Message-----
> From: je...@silicom-usa.com <je...@silicom-usa.com>
> Sent: Tuesday, March 22, 2022 23:24
> To: dev@dpdk.org
> Cc: Stephen Douthit <steph...@silicom-usa.com>; Daly, Jeff 
> <je...@silicom-usa.com>; Wang, Haiyue
> <haiyue.w...@intel.com>
> Subject: [PATCH v2] net/ixgbe: Retry SFP ID read field to handle misbehaving 
> SFPs
> 
> From: Stephen Douthit <steph...@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 <steph...@silicom-usa.com>
> Signed-off-by: Jeff Daly <je...@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

Reply via email to