linux-kernel - Re: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

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: <Yw3/vIDAr9W7zZwv@lunn.ch>
Date:   Tue, 30 Aug 2022 14:17:00 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     "Jamaluddin, Aminuddin" <aminuddin.jamaluddin@...el.com>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "Ismail, Mohammad Athari" <mohammad.athari.ismail@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "Tan, Tee Min" <tee.min.tan@...el.com>,
        "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
Subject: Re: [PATCH net 1/1] net: phy: marvell: add link status check before
 enabling phy loopback

> > > @@ -2015,14 +2016,23 @@ static int m88e1510_loopback(struct
> > phy_device *phydev, bool enable)
> > >  		if (err < 0)
> > >  			return err;
> > >
> > > -		/* FIXME: Based on trial and error test, it seem 1G need to
> > have
> > > -		 * delay between soft reset and loopback enablement.
> > > -		 */
> > > -		if (phydev->speed == SPEED_1000)
> > > -			msleep(1000);
> > > +		if (phydev->speed == SPEED_1000) {
> > > +			err = phy_read_poll_timeout(phydev, MII_BMSR,
> > val, val & BMSR_LSTATUS,
> > > +						    PHY_LOOP_BACK_SLEEP,
> > > +
> > PHY_LOOP_BACK_TIMEOUT, true);
> > 
> > Is this link with itself?
>  
> Its required cabled plug in, back to back connection.

Loopback should not require that. The whole point of loopback in the
PHY is you can do it without needing a cable.

> > 
> > Have you tested this with the cable unplugged?
> 
> Yes we have and its expected to have the timeout. But the self-test required the link
> to be up first before it can be run.

So you get an ETIMEDOUT, and then skip the code which actually sets
the LOOPBACK bit?

Please look at this again, and make it work without a cable.

Maybe you are addressing the wrong issue? Is the PHY actually
performing loopback, but reporting the link is down? Maybe you need to
fake a link up? Maybe you need the self test to not care about the
link state, all it really needs is that packets get looped?

       Andrew