Date:   Thu, 31 Aug 2017 12:18:50 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Mason <slash.tmp@...e.fr>
Cc:     Marc Gonzalez <marc_gonzalez@...madesigns.com>,
        David Daney <ddaney.cavm@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        David Miller <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>, Mans Rullgard <mans@...sr.com>
Subject: Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in
 phy_stop_machine()"

On 08/31/2017 12:09 PM, Mason wrote:
> On 31/08/2017 19:03, Florian Fainelli wrote:
> 
>> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>>
>>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>>
>>>> The original motivation for this change originated from Marc Gonzalez
>>>> indicating that his network driver did not have its adjust_link callback
>>>> executing with phydev->link = 0 while he was expecting it.
>>>
>>> I expect the core to call phy_adjust_link() for link changes.
>>> This used to work back in 3.4 and was broken somewhere along
>>> the way.
>>
>> If that was working correctly in 3.4 surely we can look at the diff and
>> figure out what changed, even maybe find the offending commit, can you
>> do that?
> 
> Bisecting would a be a huge pain because my platform was
> not upstream until v4.4

Then just diff the file and try to pinpoint which commit may have
changed that?

> 
> You mentioned the guarantees made by PHYLIB.
> When is the adjust_link callback guaranteed to be called?

As long as the state machine is running after a call to phy_start()
adjust_link will be called if there a change in link and/or link
settings. Once you call phy_stop() no such guarantees are made.

> 
>>>> PHYLIB has never made any such guarantees ever because phy_stop() merely
>>>> just tells the workqueue to move into PHY_HALTED state which will happen
>>>> asynchronously.
>>>
>>> My original proposal was to fix the issue in the driver.
>>> I'll try locating it in my archives.
>>
>> Yes I remember you telling that, by the way I don't think you ever
>> provided a clear explanation why this is absolutely necessary for your
>> driver though?
> 
> 1) nb8800_link_reconfigure() calls phy_print_status()
> which prints the "Link down" and "Link up" messages
> to the console. With the patch reverted, nothing is
> printed when the link goes down, and the result is
> random when the link comes up. Sometimes, we get
> down + up, sometimes just up.

Nothing printed when you bring down the network interface as a result of
not signaling the link down, there is a small nuance here.

Seeing a random message upon bringing the interface back up suggests you
may not be re-initialization your old vs. new link state book keeping
variables and state transitions are not properly detected and therefore
not printed. In fact, I don't see where priv->link is ever set to say -1
to force the comparison between phydev->link != priv->link to be true,
oversight?

> 
> 2) nb8800_link_reconfigure() does some HW init when
> the link state changes. If we miss some notifications,
> we might not perform some HW init, and stuff breaks.

Care to be more specific? What specific HW init is required during link
notification that if not done breaks the HW? There is both
nb8800_mac_config() and nb8800_pause_config() that are both called in
the adjust_link callback both could presumably be deferred until the
link is detected, so why do you need it during ndo_stop() absolutely?
-- 
Florian

Powered by blists - more mailing lists