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: <CA+h21hoOZQ79rj0SLZGLnkSjrKD3aLNos0GcnRjre-Ls=Tq=4w@mail.gmail.com>
Date:   Sun, 11 Aug 2019 18:16:11 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Marek Behun <marek.behun@....cz>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>, netdev <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

Hi Marek,

On Sun, 11 Aug 2019 at 17:06, Marek Behun <marek.behun@....cz> wrote:
>
> 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?
>

I don't know. But I think Heiner was asking you what kernel you're on
because of what you said here:

> Hopefully DSA fixed-link port functionality will be converted to phylink
> API soon.

The DSA fixed-link port functionality *has* been converted to phylink.
See:
- https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=0e27921816ad9
- https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=7fb5a711545d7d25fe9726a9ad277474dd83bd06


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

Regards,
-Vladimir