Re: [PATCH net-next v4 4/4] net: sfp: add support for multigig RollBall transceivers

["Pali Rohár" <pali@kernel.org>]

On Wednesday 13 January 2021 11:08:52 Russell King - ARM Linux admin wrote:
> On Wed, Jan 13, 2021 at 11:49:36AM +0100, Pali Rohár wrote:
> > On Monday 11 January 2021 06:00:44 Marek Behún wrote:
> > > @@ -1453,7 +1459,7 @@ static int sfp_sm_probe_phy(struct sfp *sfp, bool is_c45)
> > >  	struct phy_device *phy;
> > >  	int err;
> > >  
> > > -	phy = get_phy_device(sfp->i2c_mii, SFP_PHY_ADDR, is_c45);
> > > +	phy = get_phy_device(sfp->i2c_mii, sfp->phy_addr, is_c45);
> > >  	if (phy == ERR_PTR(-ENODEV))
> > >  		return PTR_ERR(phy);
> > >  	if (IS_ERR(phy)) {
> > > @@ -1835,6 +1841,23 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
> > >  
> > >  	sfp->mdio_protocol = MDIO_I2C_DEFAULT;
> > >  
> > > +	sfp->phy_addr = SFP_PHY_ADDR;
> > > +	sfp->module_t_wait = T_WAIT;
> > > +
> > > +	if (((!memcmp(id.base.vendor_name, "OEM             ", 16) ||
> > > +	      !memcmp(id.base.vendor_name, "Turris          ", 16)) &&
> > > +	     (!memcmp(id.base.vendor_pn, "SFP-10G-T       ", 16) ||
> > > +	      !memcmp(id.base.vendor_pn, "RTSFP-10", 8)))) {
> > > +		sfp->mdio_protocol = MDIO_I2C_ROLLBALL;
> > > +		sfp->phy_addr = SFP_PHY_ADDR_ROLLBALL;
> > > +		sfp->module_t_wait = T_WAIT_ROLLBALL;
> > > +
> > > +		/* RollBall SFPs may have wrong (zero) extended compliacne code
> 
> Spelling error - "compliance"
> 
> > > +		 * burned in EEPROM. For PHY probing we need the correct one.
> > > +		 */
> > > +		id.base.extended_cc = SFF8024_ECC_10GBASE_T_SFI;
> > 
> > Should not we rather in sfp_sm_probe_for_phy() function in "default"
> > section try to probe also for clause 45 PHY when clause 22 fails?
> 
> Why? That's opening the possibilities for more problems - remember,
> the access method is vendor defined, and we already have the situation
> where I2C address 0x56 is used in two different styles that are
> indistinguishable:
> 
> - Clause 22 write:
> 	Write register address, value high, value low.
> - Clause 22 read:
> 	Write register address.
> 	Read value high, low.
> - Clause 45 write:
> 	Write devad, register address high, register address low,
> 		value high, value low.
> - Clause 45 read:
> 	Write devad, register address high, register address low.
> 	Read value high, low.
> 
> Look closely at the similarities of Clause 22 write and Clause 45
> read, you'll see that if you issue a clause 45 read to a SFP module
> that implements Clause 22, you actually end up issuing a write to it.
> 
> Sending random MDIO cycles to a SFP is a really bad idea.

I see, thank you for explanation. Incorrect data in SFP EEPROM may cause
lot of other issues :-(