Message-ID: <22cd777b-ffda-439b-b2e5-866235aba05e@nvidia.com>
Date: Tue, 30 Jul 2024 11:02:07 +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 30/07/2024 10:41, Russell King (Oracle) wrote:
> 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?
> 

It is the drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c driver. It 
works most of the time, but on occasion it fails to get a valid IP 
address.

Thanks
Jon
-- 
nvpublic

Powered by blists - more mailing lists