From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
Date: Wed, 4 Jul 2018 20:13:05 +0200
Message-ID: <c2d3f0d0-3082-a96a-3dbb-02d9a90117a7@gmail.com>
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>
 <20180704144617.GF12405@lunn.ch>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
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: Andrew Lunn <andrew@lunn.ch>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-wm0-f68.google.com ([74.125.82.68]:36085 "EHLO
        mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752251AbeGDSNM (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 4 Jul 2018 14:13:12 -0400
Received: by mail-wm0-f68.google.com with SMTP id s14-v6so7223653wmc.1
        for <netdev@vger.kernel.org>; Wed, 04 Jul 2018 11:13:12 -0700 (PDT)
In-Reply-To: <20180704144617.GF12405@lunn.ch>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 04.07.2018 16:46, Andrew Lunn wrote:
> 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().
> 
Thanks for the clarification. I think beginning of next week I can
provide a v2 of the patch series.

Heiner


>        Andrew
>