netdev - Re: [PATCH net-next v2 1/1] net: dsa: fix fixed-link port registration

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]

Message-ID: <20190811160404.06450685@nic.cz>
Date:   Sun, 11 Aug 2019 16:04:04 +0200
From:   Marek Behun <marek.behun@....cz>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        Sebastian Reichel <sebastian.reichel@...labora.co.uk>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v2 1/1] net: dsa: fix fixed-link port
 registration

OK guys, something is terribly wrong here.

I bisected to the commit mentioned (88d6272acaaa), looked around at the
genphy functions, tried adding the link=0 workaround and it did work,
so I though this was the issue.

What I realized now is that before the commit 88d6272acaaa things
worked because of two bugs, which negated each other. This commit caused
one of this bugs not to fire, and thus the second bug was not negated.

What actually happened before the commit that broke it is this:
  - after the fixed_phy is created, the parameters are corrent
  - genphy_read_status breaks the parameters:
     - first it sets the parameters to unknown (SPEED_UNKNOWN,
       DUPLEX_UNKNOWN)
     - then read the registers, which are simulated for fixed_phy
     - then it uses phy-core.c:phy_resolve_aneg_linkmode function, which
       looks for correct settings by bit-anding the ->advertising and
       ->lp_advertigins bit arrays. But in fixed_phy, ->lp_advertising
       is set to zero, so the parameters are left at SPEED_UNKNOWN, ...
       (this is the first bug)
  - then adjust_link is called, which then goes to
    mv88e6xxx_port_setup_mac, where there is a test if it should change
    something:
       if (state.link == link && state.speed == speed &&
           state.duplex == duplex)
               return 0;
  - since current speed on the switch port (state.speed) is SPEED_1000,
    and new speed is SPEED_UNKNOWN, this test fails, and so the rest of
    this function is called, which makes the port work
    (the if test is the second bug)

After the commit that broke things:
  - after the fixed_phy is created, the parameters are corrent
  - genphy_read_status doesn't change them
  - mv88e6xxx_port_setup_mac does nothing, since the if condition above
    is true

So, there are two things that are broken:
 - the test in mv88e6xxx_port_setup_mac whether there is to be a change
   should be more sophisticated
 - fixed_phy should also simulate the lp_advertising register

What do you think of this?

Marek

On Sun, 11 Aug 2019 13:35:20 +0200
Heiner Kallweit <hkallweit1@...il.com> wrote:

> On 11.08.2019 05:39, Andrew Lunn wrote:
> > On Sun, Aug 11, 2019 at 05:18:57AM +0200, Marek Behún wrote:  
> >> Commit 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in
> >> genphy_read_status") broke fixed link DSA port registration in
> >> dsa_port_fixed_link_register_of: the genphy_read_status does not do what
> >> it is supposed to and the following adjust_link is given wrong
> >> parameters.  
> > 
> > Hi Marek
> > 
> > Which parameters are incorrect?
> > 
> > In fixed_phy.c, __fixed_phy_register() there is:
> > 
> >         /* propagate the fixed link values to struct phy_device */
> >         phy->link = status->link;
> >         if (status->link) {
> >                 phy->speed = status->speed;
> >                 phy->duplex = status->duplex;
> >                 phy->pause = status->pause;
> >                 phy->asym_pause = status->asym_pause;
> >         }
> > 
> > Are we not initialising something? Or is the initialisation done here
> > getting reset sometime afterwards?
> >   
> In addition to Andrew's question:
> We talk about this DT config: armada-385-turris-omnia.dts ?
> Which kernel version are you using?
> 
> > Thanks
> > 	Andrew
> >   
> Heiner