netdev - Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop

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]

Date:   Thu, 31 Aug 2017 21:09:28 +0200
From:   Mason <slash.tmp@...e.fr>
To:     Florian Fainelli <f.fainelli@...il.com>
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 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

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

>>> 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.

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.

Regards.