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: <20190811151830.GA14290@lunn.ch>
Date:   Sun, 11 Aug 2019 17:18:30 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Marek Behun <marek.behun@....cz>
Cc:     Heiner Kallweit <hkallweit1@...il.com>, 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

On Sun, Aug 11, 2019 at 04:04:04PM +0200, Marek Behun 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?

Marek

This is the sort of information i like. I was having trouble
understanding what was really wrong and how it was fixed by your
previous patch.

So setting the emulated lp_advertise to advertise makes a lot of sense
for fixed phy. And it is something worthy of stable.

As for mv88e6xxx_port_setup_mac(), which parameter is actually
important here? My assumption was, if one of the other parameters
changes, it would not happen alone. The link would also go down, and
later come up again, etc. But it seems that assumption is wrong.

At a guess, it is the RGMII delays. That would explain CRC errors in
frames received by the master interface.

       Andrew