Re: [PATCH net-next] net: phy: sfp: Cotsworks SFF module EEPROM fixup

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