netdev - Re: [PATCH RFC russell-king 2/4] net: phy: sfp: add support for multigig RollBall modules

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200811151552.GM1551@shell.armlinux.org.uk>
Date:   Tue, 11 Aug 2020 16:15:53 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Marek Behún <marek.behun@....cz>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>,
        Baruch Siach <baruch@...s.co.il>,
        Chris Healy <cphealy@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC russell-king 2/4] net: phy: sfp: add support for
 multigig RollBall modules

On Tue, Aug 11, 2020 at 12:06:43AM +0200, Marek Behún wrote:
> This adds support for multigig copper SFP modules from RollBall/Hilink.
> These modules have a specific way to access clause 45 registers of the
> internal PHY.
> 
> We also need to wait at least 25 seconds after deasserting TX disable
> before accessing the PHY. The code waits for 30 seconds just to be sure.

I wonder why it takes so long - the 88x3310 boots very quickly.

> +static int sfp_i2c_mii_probe(struct sfp *sfp)
> +{
> +	struct mii_bus *i2c_mii;
> +	int ret;
> +
> +	if (sfp->rollball_mii)
> +		i2c_mii = mdio_i2c_rollball_alloc(sfp->dev, sfp->i2c);
> +	else
> +		i2c_mii = mdio_i2c_alloc(sfp->dev, sfp->i2c);
> +

I think I'd prefer to find a way to teach the existing mdio_i2c code
about different protocols rather than having a load of different buses
that we have to select between.  Maybe we can do that via the PHY
address, or maybe we should have a call into mdio_i2c that tells it
which protocol should be used?

The reason I don't like this approach is it seems to me to be very
heavy handed... using a sledge hammer to crack a nut.

> +	rollball = (!memcmp(id.base.vendor_name, "OEM             ", 16) &&
> +		    (!memcmp(id.base.vendor_pn, "SFP-10G-T       ", 16) ||
> +		     !memcmp(id.base.vendor_pn, "RTSFP-10        ", 16) ||
> +		     !memcmp(id.base.vendor_pn, "RTSFP-2.5G      ", 16)));
> +	if (rollball) {
> +		/* TODO: try to write this to EEPROM */
> +		id.base.extended_cc = SFF8024_ECC_10GBASE_T_SFI;

Should we really be "fixing" vendors EEPROMs for them?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!