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]

Date: Thu, 13 Jul 2023 11:14:23 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Stefan Eichenberger <eichest@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
	hkallweit1@...il.com, 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

On Thu, Jul 13, 2023 at 11:42:12AM +0200, Stefan Eichenberger wrote:
> 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:

I've never really understood this kind of reaction from people. A bit
of example code gets suggested, and then the code gets sort of used
as a half-hearted template...

> 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;

Both genphy_update_link() and genphy_c45_read_link() check here whether
the link is up, and if it is, it bypasses the second read (because
there's no point re-reading it.) I've no idea why you dropped that
optimisation.

MDIO accesses aren't the cheapest thing...

> 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.

Why do you think that's better? "Link" only latches low, and the
entire point of that behaviour is so that management software can
detect when the link has failed at some point since it last read
the link status.

There is only any point in re-reading the status register if on the
first read it reports that the link has failed, and only then if we
already knew that the link has failed.

If we're using interrupt mode, then we need the current link status
but that's only because of the way phylib has been written.

> 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.

With copper PHYs, link up status means that the link is ready to pass
data. Is this not the case with T1 PHYs?

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