> -----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