netdev - Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed

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: <20180704144617.GF12405@lunn.ch>
Date:   Wed, 4 Jul 2018 16:46:17 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii

On Mon, Jul 02, 2018 at 11:54:54PM +0200, Heiner Kallweit wrote:
> On 02.07.2018 23:21, Andrew Lunn wrote:
> >> -		auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
> > 
> > This bit you probably want to keep. The PHY never says it support
> > Pause. The MAC needs to enable pause if the MAC supports pause.
> > 
> Actually I assumed that phylib would do this for me. But:
> In phy_probe() first phydev->supported is copied to
> phydev->advertising, and only after this both pause flags are added
> to phydev->supported. Therefore I think they are not advertised.
> Is this intentional? It sounds a little weird to me to add the
> pause flags to the supported features per default, but not
> advertise them.

phylib has no idea if the MAC supports Pause. So it should not enable
it by default. The MAC needs to enable it. And a lot of MAC drivers
get this wrong...

> Except e.g. we call by chance phy_set_max_speed(), which copies
> phydev->supported to phydev->advertising after having adjusted
> the supported speeds.

As you correctly pointed out, phy_set_max_speed() is masking out too
much.

> If this is not a bug, then where would be the right place to add
> the pause flags to phydev->advertising?

Before you call phy_start().

       Andrew