netdev - Re: Convention regarding SGMII in-band autonegotiation

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: <ZCv5Awt1tQic2Ygj@shell.armlinux.org.uk>
Date:   Tue, 4 Apr 2023 11:16:35 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Daniel Golle <daniel@...rotopia.org>, netdev@...r.kernel.org,
        Vladimir Oltean <vladimir.oltean@....com>,
        Alexander 'lynxis' Couzens <lynxis@...0.eu>,
        Chukun Pan <amadeus@....edu.cn>,
        John Crispin <john@...ozen.org>
Subject: Re: Convention regarding SGMII in-band autonegotiation

On Tue, Apr 04, 2023 at 11:56:45AM +0200, Heiner Kallweit wrote:
> On 04.04.2023 11:13, Daniel Golle wrote:
> > On Tue, Apr 04, 2023 at 08:31:12AM +0200, Heiner Kallweit wrote:
> >> Ideas from the patches can be re-used. Some patches itself are not ready
> >> for mainline (replace magic numbers with proper constants (as far as
> >> documented by Realtek), inappropriate use of phy_modify_mmd_changed,
> >> read_status() being wrong place for updating interface mode).
> > 
> > Where is updating the interface mode supposed to happen?
> > 
> > I was looking at drivers/net/phy/mxl-gpy.c which calls its
> > gpy_update_interface() function also from gpy_read_status(). If that is
> > wrong it should probably be fixed in mxl-gpy.c as well...
> > 
> 
> Right, several drivers use the read_status() callback for this, I think
> the link_change_notify() callback would be sufficient.

Sorry, but that's too late.

The problem is that phy_check_link_status() reads the link status, and
then immediately acts on it calling phy_link_up() or phy_link_down()
as appropriate. While the phy state changes at the same time, we're
still in the state machine switch() here, and it's only after that
switch() that we then call link_change_notify() - and that will be
_after_ phylink or MAC driver's adjust_link callback has happened.

So, using link_change_notify() would mean that the phylib user will
be informed of the new media parameters except for the interface mode,
_then_ link_change_notify() will be called, and only then would
phydev->interface change - and there's no callback to the phylib
user to inform them that something changed.

In any case, we do _not_ want two callbacks into the phylib user for
the state change, especially not one where the first is "link is up"
and the second is "oh by the way the interface changed".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!