netdev - Re: [PATCH] net: macb: do not disable MDIO bus when closing interface

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:   Mon, 13 Aug 2018 12:08:10 +0300
From:   Claudiu Beznea <Claudiu.Beznea@...rochip.com>
To:     Anssi Hannula <anssi.hannula@...wise.fi>,
        Andrew Lunn <andrew@...n.ch>
CC:     Nicolas Ferre <nicolas.ferre@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: macb: do not disable MDIO bus when closing interface

On 10.08.2018 09:22, Anssi Hannula wrote:
> On 9.8.2018 18:14, Andrew Lunn wrote:
>> Hi Anssi
> 
> Hi!
> 
>>> macb_reset_hw() is called in init path too,

I only see it in macb_close() and macb_open() called from macb_init_hw().

though, so maybe clearing
>>> all bits is intentional / wanted to get the controller to a known state,

As far as I know the NCR is used in this driver only to control transmit
enable, receive enable, MPE and tx start.

Not clearing NCR in the init phase should not have any impact on IP. Same
for not clearing Tx start on "if down" path (if tx is in progress).

>>> even though the comment only mentions TX/RX?
>> You need to be careful here. Once of_mdiobus_register() is called, the
>> MDIO should be usable. If you happen to have an Ethernet switch on the
>> bus, it could be probed then. The DSA driver will start using the bus.
>> Or if you have a second PHY, connected to some other MAC, it could be
>> used by the other MAC.  This all happens in the macb_probe function.
>>
>> Sometime later, the interface will be up'ed. At this point macb_open()
>> is called, which calls macb_init_hw(), which calls
>> macb_reset_hw(). What you don't want happening is changes to the NCR
>> at this point breaking an MDIO transaction which might be going on.
>>
>> Ideally, the MPE should be enabled before of_mdiobus_register(), and
>> left alone until mdiobus_unregister() is called in macb_remove().
> 
> Yep, fixing the use case of having PHYs of other MACs is why I wrote the
> patch :)
> 
> Currently the reset code disables MPE while other MACs are using PHYs on
> the bus.

MPE is set in the early phase of initialization, in macb_mii_init(), called
from probe() function. Due to the fact that NCR is cleared in
macb_reset_hw() it is set again at the end of macb_init_hw(). So, as per
Andrew comments in this thread, setting it in macb_mii_init() and letting
it unchanged until remove would be the best. So, if you clear only TE and
RE bits in macb_reset_hw() please also remove MACB_BIT(MPE) from below line
at the end of macb_init_hw():

        macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));

Thank you,
Claudiu Beznea

>