netdev - Re: [PATCH net-next v5 4/5] net: sfp: create/destroy I2C mdiobus before PHY probe/after PHY release

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: <20210114160719.GV1551@shell.armlinux.org.uk>
Date:   Thu, 14 Jan 2021 16:07:19 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Marek Behún <kabel@...nel.org>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net,
        pali@...nel.org
Subject: Re: [PATCH net-next v5 4/5] net: sfp: create/destroy I2C mdiobus
 before PHY probe/after PHY release

On Thu, Jan 14, 2021 at 05:43:30AM +0100, Marek Behún wrote:
> Instead of configuring the I2C mdiobus when SFP driver is probed,
> create/destroy the mdiobus before the PHY is probed for/after it is
> released.
> 
> This way we can tell the mdio-i2c code which protocol to use for each
> SFP transceiver.

I've been thinking a bit more about this. It looks like it will
allocate and free the MDIO bus each time any module is inserted or
removed, even a fiber module that wouldn't ever have a PHY. This adds
unnecessary noise to the kernel message log.

We only probe for a PHY if one of:

- id.base.extended_cc is SFF8024_ECC_10GBASE_T_SFI,
  SFF8024_ECC_10GBASE_T_SR, SFF8024_ECC_5GBASE_T, or
  SFF8024_ECC_2_5GBASE_T.
- id.base.e1000_base_t is set.

So, we only need the MDIO bus to be registered if one of those is true.

As you are introducing "enum mdio_i2c_proto", I'm wondering whether
that should include "MDIO_I2C_NONE", and we should only register the
bus and probe for a PHY if it is not MDIO_I2C_NONE.

Maybe we should have:

enum mdio_i2c_proto {
	MDIO_I2C_NONE,
	MDIO_I2C_MARVELL_C22,
	MDIO_I2C_C45,
	MDIO_I2C_ROLLBALL,
	...
};

with:

	sfp->mdio_protocol = MDIO_I2C_NONE;
	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->module_t_wait = T_WAIT_ROLLBALL;
	} else {
		switch (id.base.extended_cc) {
		...
		}
	}

static int sfp_sm_add_mdio_bus(struct sfp *sfp)
{
	int err = 0;

	if (sfp->mdio_protocol != MDIO_I2C_NONE)
		err = sfp_i2c_mdiobus_create(sfp);

	return err;
}

called from the place you call sfp_i2c_mdiobus_create(), and
sfp_sm_probe_for_phy() becomes:

static int sfp_sm_probe_for_phy(struct sfp *sfp)
{
	int err = 0;

	switch (sfp->mdio_protocol) {
	case MDIO_I2C_NONE:
		break;

	case MDIO_I2C_MARVELL_C22:
		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, false);
		break;

	case MDIO_I2C_C45:
		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, true);
		break;

	case MDIO_I2C_ROLLBALL:
		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR_ROLLBALL, true);
		break;
	}

	return err;
}

This avoids having to add the PHY address, as well as fudge around with
id.base.extended_cc to get the PHY probed.

Thoughts?

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