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: <2aefce6d-5009-491b-b797-ca318e8bad4e@nvidia.com>
Date: Tue, 30 Jul 2024 10:36:12 +0100
From: Jon Hunter <jonathanh@...dia.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 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.

Jon

-- 
nvpublic