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

[Jon Hunter <jonathanh@nvidia.com>]

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