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

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

[Chris Healy]

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>
---
 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;
+	}
 
 	/* Validate the checksum over the base structure */
 	check = sfp_check(&id.base, sizeof(id.base) - 1);
-- 
2.21.3

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

[Florian Fainelli]

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

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

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

[David Miller]

From: Chris Healy <cphealy@gmail.com>
Date: Tue, 14 Jul 2020 10:59:10 -0700

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

Applied, thank you.