linux-kernel - Re: [PATCH net-next] net: phy: microchip

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: <dba796b1-bb59-4d90-b592-1d56e3fba758@lunn.ch>
Date: Thu, 5 Sep 2024 14:38:52 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Tarun Alle <tarun.alle@...rochip.com>
Cc: arun.ramadoss@...rochip.com, UNGLinuxDriver@...rochip.com,
	hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: microchip_t1: SQI support for LAN887x

> +	/* Get 200 SQI raw readings */
> +	for (int i = 0; i < 200; i++) {

Please replace all the hard coded 200 with ARRAY_SIZE(rawtable). That
makes it easier to tune the size of the table without causing buffer
overrun bugs.

> +		rc = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +				   LAN887X_POKE_PEEK_100,
> +				   LAN887X_POKE_PEEK_100_EN);
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> +				  LAN887X_SQI_MSE_100);
> +		if (rc < 0)
> +			return rc;
> +
> +		rawtable[i] = rc;
> +		rc = genphy_c45_read_link(phydev);
> +		if (rc < 0)
> +			return rc;
> +
> +		if (!phydev->link)
> +			return -ENETDOWN;
> +	}

How long does this take?

genphy_c45_read_link() takes a few MDIO transaction, plus the two you
see here. So maybe 1000 MDIO bus transactions? Which could be
3000-4000 if it needs to use C45 over C22.

Do you have any data on the accuracy, with say 10, 20, 40, 80, 160
samples?

Can the genphy_c45_read_link() be moved out of the loop? If the link
is lost, is the sample totally random, or does it have a well defined
value? Looking at the link status every iteration, rather than before
and after collecting the samples, you are trying to protect against
the link going down and back up again. If it is taking a couple of
seconds to collect all the samples, i suppose that is possible, but if
its 50ms, do you really have to worry?

> +static int lan887x_get_sqi(struct phy_device *phydev)
> +{
> +	int rc, val;
> +
> +	if (phydev->speed != SPEED_1000 &&
> +	    phydev->speed != SPEED_100) {
> +		return -EINVAL;
> +	}

Can that happen? Does the PHY support SPEED_10? Or are you trying to
protect against SPEED_UNKOWN because the link is down? ENETDOWN might
be more appropriate that EINVAL.

	Andrew