Messages in this thread | | | Subject | Re: [PATCH net-next] net: phy: sfp: Cotsworks SFF module EEPROM fixup | From | Florian Fainelli <> | Date | Wed, 15 Jul 2020 20:37:13 -0700 |
| |
On 7/15/2020 8:32 PM, Chris Healy wrote: > > > On Wed, Jul 15, 2020 at 8:10 PM Florian Fainelli <f.fainelli@gmail.com > <mailto:f.fainelli@gmail.com>> wrote: > > > > On 7/14/2020 10:59 AM, Chris Healy wrote: > > Some Cotsworks SFF have invalid data in the first few bytes of the > > module EEPROM. This results in these modules not being detected as > > valid modules. > > > > Address this by poking the correct EEPROM values into the module > > EEPROM when the model/PN match and the existing module EEPROM contents > > are not correct. > > > > Signed-off-by: Chris Healy <cphealy@gmail.com > <mailto:cphealy@gmail.com>> > > --- > > drivers/net/phy/sfp.c | 44 > +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > > index 73c2969f11a4..2737d9b6b0ae 100644 > > --- a/drivers/net/phy/sfp.c > > +++ b/drivers/net/phy/sfp.c > > @@ -1632,10 +1632,43 @@ static int sfp_sm_mod_hpower(struct sfp > *sfp, bool enable) > > return 0; > > } > > > > +static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct > sfp_eeprom_id *id) > > +{ > > + u8 check; > > + int err; > > + > > + if (id->base.phys_id != SFF8024_ID_SFF_8472 || > > + id->base.phys_ext_id != SFP_PHYS_EXT_ID_SFP || > > + id->base.connector != SFF8024_CONNECTOR_LC) { > > + dev_warn(sfp->dev, "Rewriting fiber module EEPROM > with corrected values\n"); > > + id->base.phys_id = SFF8024_ID_SFF_8472; > > + id->base.phys_ext_id = SFP_PHYS_EXT_ID_SFP; > > + id->base.connector = SFF8024_CONNECTOR_LC; > > + err = sfp_write(sfp, false, SFP_PHYS_ID, &id->base, 3); > > + if (err != 3) { > > + dev_err(sfp->dev, "Failed to rewrite module > EEPROM: %d\n", err); > > + return err; > > + } > > + > > + /* Cotsworks modules have been found to require a > delay between write operations. */ > > + mdelay(50); > > + > > + /* Update base structure checksum */ > > + check = sfp_check(&id->base, sizeof(id->base) - 1); > > + err = sfp_write(sfp, false, SFP_CC_BASE, &check, 1); > > + if (err != 1) { > > + dev_err(sfp->dev, "Failed to update base > structure checksum in fiber module EEPROM: %d\n", err); > > + return err; > > + } > > + } > > + return 0; > > +} > > + > > static int sfp_sm_mod_probe(struct sfp *sfp, bool report) > > { > > /* SFP module inserted - read I2C data */ > > struct sfp_eeprom_id id; > > + bool cotsworks_sfbg; > > bool cotsworks; > > u8 check; > > int ret; > > @@ -1657,6 +1690,17 @@ static int sfp_sm_mod_probe(struct sfp > *sfp, bool report) > > * serial number and date code. > > */ > > cotsworks = !memcmp(id.base.vendor_name, "COTSWORKS ", > 16); > > + cotsworks_sfbg = !memcmp(id.base.vendor_pn, "SFBG", 4); > > + > > + /* Cotsworks SFF module EEPROM do not always have valid phys_id, > > + * phys_ext_id, and connector bytes. Rewrite SFF EEPROM > bytes if > > + * Cotsworks PN matches and bytes are not correct. > > + */ > > + if (cotsworks && cotsworks_sfbg) { > > + ret = sfp_cotsworks_fixup_check(sfp, &id); > > + if (ret < 0) > > + return ret; > > + } > > So with the fixup you introduce, should we ever go into a situation > where: > > EPROM extended structure checksum failure > > is printed? > > > From what I've been told, Cotsworks had an ordering problem where both > the base and extended checksums were being programmed before other > fields were programmed during manufacturing resulting in both the base > and extended checksums being incorrect. (I've also heard that Cotsworks > has resolved this issue late last year for all new units but units > manufactured before late last year will have incorrect checksums.) > > Given that I was touching the base structure in this patch, I felt that > updating the base checksum was warranted. I did not consider updating > the extended structure checksum as I wasn't changing anything else with > the extended structure. As such, we would still have an invalid > extended structure checksum and get the associated error message.
That makes sense and thanks for providing the context here! -- Florian
| |