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

[Florian Fainelli]

On 7/15/2020 8:32 PM, Chris Healy wrote:
> 
> 
> On Wed, Jul 15, 2020 at 8:10 PM Florian Fainelli <f.fainelli@xxxxxxxxx
> <mailto:f.fainelli@xxxxxxxxx>> 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@xxxxxxxxx
>     <mailto:cphealy@xxxxxxxxx>>
>     > ---
>     >Â 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