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: <cea96aa7-0073-4d81-6265-666c81809a0e@gmail.com>
Date:   Tue, 4 Apr 2023 16:13:36 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
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 04.04.2023 12:16, Russell King (Oracle) wrote:
> 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.
> 
Indeed, my bad. It's been some time ago that I last looked deeper
into this corner of phylib.

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