netdev - Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

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: <Zqi1O88vXK3Uonr1@shell.armlinux.org.uk>
Date: Tue, 30 Jul 2024 10:41:15 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jon Hunter <jonathanh@...dia.com>
Cc: Revanth Kumar Uppala <ruppala@...dia.com>,
	"andrew@...n.ch" <andrew@...n.ch>,
	"hkallweit1@...il.com" <hkallweit1@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system
 side

On Tue, Jul 30, 2024 at 10:36:12AM +0100, Jon Hunter wrote:
> 
> On 29/07/2024 11:47, Russell King (Oracle) wrote:
> 
> ...
> 
> > > Apologies for not following up before on this and now that is has been a
> > > year I am not sure if it is even appropriate to dig this up as opposed to
> > > starting a new thread completely.
> > > 
> > > However, I want to resume this conversation because we have found that this
> > > change does resolve a long-standing issue where we occasionally see our
> > > ethernet controller fail to get an IP address.
> > > 
> > > I understand that your objection to the above change is that (per Revanth's
> > > feedback) this change assumes interface has the link. However, looking at
> > > the aqr107_read_status() function where this change is made the function has
> > > the following ...
> > > 
> > > static int aqr107_read_status(struct phy_device *phydev)
> > > {
> > >          int val, ret;
> > > 
> > >          ret = aqr_read_status(phydev);
> > >          if (ret)
> > >                  return ret;
> > > 
> > >          if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
> > >                  return 0;
> > > 
> > > 
> > > So my understanding is that if we don't have the link, then the above test
> > > will return before we attempt to poll the TX ready status. If that is the
> > > case, then would the change being proposed be OK?
> > 
> > Here, phydev->link will be the _media_ side link. This is fine - if the
> > media link is down, there's no point doing anything further. However,
> > if the link is up, then we need the PHY to update phydev->interface
> > _and_ report that the link was up (phydev->link is true).
> > 
> > When that happens, the layers above (e.g. phylib, phylink, MAC driver)
> > then know that the _media_ side interface has come up, and they also
> > know the parameters that were negotiated. They also know what interface
> > mode the PHY is wanting to use.
> > 
> > At that point, the MAC driver can then reconfigure its PHY facing
> > interface according to what the PHY is using. Until that point, there
> > is a very real chance that the PHY <--> MAC connection will remain
> > _down_.
> > 
> > The patch adds up to a _two_ _second_ wait for the PHY <--> MAC
> > connection to come up before aqr107_read_status() will return. This
> > is total nonsense - because waiting here means that the MAC won't
> > get the notification of which interface mode the PHY is expecting
> > to use, therefore the MAC won't configure its PHY facing hardware
> > for that interface mode, and therefore the PHY <--> MAC connection
> > will _not_ _come_ _up_.
> > 
> > You can not wait for the PHY <--> MAC connection to come up in the
> > phylib read_status method. Ever.
> > 
> > This is non-negotiable because it is just totally wrong to do this
> > and leads to pointless two second delays.
> 
> 
> Thanks for the feedback! We will go away, review this and see if we can
> figure out a good/correct way to resolve our ethernet issue.

Which ethernet driver is having a problem?

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