From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
Date: Wed, 4 Jul 2018 16:46:17 +0200
Message-ID: <20180704144617.GF12405@lunn.ch>
References: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
 <fb72a450-8f15-2f34-38ae-26c7c3a5d159@gmail.com>
 <20180702212150.GG12564@lunn.ch>
 <30dcf9f0-b5a0-1690-6964-a0afff6c3fec@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Miller <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Realtek linux nic maintainers <nic_swsd@realtek.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
To: Heiner Kallweit <hkallweit1@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from vps0.lunn.ch ([185.16.172.187]:51822 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752862AbeGDOqW (ORCPT <rfc822;netdev@vger.kernel.org>);
        Wed, 4 Jul 2018 10:46:22 -0400
Content-Disposition: inline
In-Reply-To: <30dcf9f0-b5a0-1690-6964-a0afff6c3fec@gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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