netdev - 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <BY5PR11MB4337F996C04AFCDC219EF9C181339@BY5PR11MB4337.namprd11.prod.outlook.com>
Date:   Thu, 27 Oct 2022 04:35:49 +0000
From:   "Jamaluddin, Aminuddin" <aminuddin.jamaluddin@...el.com>
To:     Andrew Lunn <andrew@...n.ch>
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

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Tuesday, 20 September, 2022 7:46 AM
> 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; linux-
> kernel@...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
> 
> > > > 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?
> >
> > If cable unplugged, test result will be displayed as 1. See comments below.
> >
> > >
> > > Please look at this again, and make it work without a cable.
> >
> > Related to this the flow without cable, what we see in the codes during
> debugging.
> > After the phy loopback bit was set.
> > The test will be run through this __stmmac_test_loopback()
> > https://elixir.bootlin.com/linux/v5.19.8/source/drivers/net/ethernet/s
> > tmicro/stmmac/stmmac_selftests.c#L320
> > Here, it will have another set of checking in dev_direct_xmit(),
> __dev_direct_xmit().
> > returning value 1(NET_XMIT_DROP)
> > https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4288
> > Which means the interface is not available or the interface link status is not
> up.
> > For this case the interface link status is not up.
> > Thus failing the phy loopback test.
> > https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4296
> > Since we don't own this __stmmac_test_loopback(), we conclude the
> behaviour was as expected.
> >
> > >
> > > 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?
> >
> > When bit 14 was set, the link will be broken.
> > But before the self-test was triggered it requires link to be up as stated
> above comments.
> 
> You have not said anything about my comment:
> 
> > Maybe you need to fake a link up?
> 
> My guess is, some PHYs are going to report link up when put into loopback.
> Others might not. For the Marvell PHY, it looks like you need to make
> marvell_read_status() return that the link is up if loopback is enabled.
> 

We able to do the PHY loopback test, after fake link up without 
cable plugged on as suggested above. 
We will provide version 2 patch with minimum code changes 
without having the status link check.
Only need to increase the msleep(200) to msleep(1000) inside
m88e1510_loopback() function.

Amin