netdev - Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

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: <ZK/G9FMPSabQCGNk@eichest-laptop>
Date: Thu, 13 Jul 2023 11:42:12 +0200
From: Stefan Eichenberger <eichest@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, hkallweit1@...il.com, linux@...linux.org.uk,
	francesco.dolcini@...adex.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver
 for the Marvell 88Q2110 PHY

Hi Andrew,

Thanks a lot for the review and all the hints, I have one short question
below.

> > +static int mv88q2xxx_read_link(struct phy_device *phydev)
> > +{
> > +	u16 ret1, ret2;
> > +
> > +	/* The 88Q2XXX PHYs do not have the PMA/PMD status register available,
> > +	 * therefore we need to read the link status from the vendor specific
> > +	 * registers.
> > +	 */
> > +	if (phydev->speed == SPEED_1000) {
> > +		/* Read twice to clear the latched status */
> > +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> > +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> 
> This is generally wrong. See for example genphy_update_link() and
> genphy_c45_read_link().
> 

Would something like this look fine to you? The issue is that I mix
realtime data with latched data because the local and remote rx status
is only available in realtime from what I can find in the datasheet.
This would be for gbit, I split that up compared to the last version:

static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
{
	int ret1, ret2;

	/* The link state is latched low so that momentary link drops can be
	 * detected. Do not double-read the status in polling mode to detect
	 * such short link drops except the link was already down. In case we
	 * are not polling, we always read the realtime status.
	 */
	if (!phy_polling_mode(phydev) || !phydev->link) {
		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
		if (ret1 < 0)
			return ret1;
	}

	ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
	if (ret1 < 0)
		return ret1;

	/* Read vendor specific Auto-Negotiation status register to get local
	 * and remote receiver status according to software initialization
	 * guide.
	 */
	ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STATUS);
	if (ret2 < 0)
		return ret2;

	/* Check if we have link and if the remote and local receiver are ok */
	return (ret1 & MDIO_PCS_1000BT1_STAT_LINK) &&
	       (ret2 & MDIO_MMD_AN_MV_STATUS_LOCAL_RX) &&
	       (ret2 & MDIO_MMD_AN_MV_STATUS_REMOTE_RX);
}

With this we will detect link loss in polling mode and read the realtime
status in non-polling mode. Compared to genphy_c45_read_link we will not
immediately return "link up" in non polling mode but always do the
second read to get the realtime link status.

If we are only interested in the link status we could also skip the
remote and local receiver check. However, as I understand the software
initialization guide it could be that the receivers are not ready in
that moment.

Regards,
Stefan