From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps
Date: Sun, 25 Nov 2018 17:45:13 +0100
Message-ID: <20181125164513.GD18663@lunn.ch>
References: <7851597d-e582-379f-f158-8d103a895720@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Florian Fainelli <f.fainelli@gmail.com>,
        David Miller <davem@davemloft.net>,
        "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]:48398 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1726336AbeKZDgo (ORCPT <rfc822;netdev@vger.kernel.org>);
        Sun, 25 Nov 2018 22:36:44 -0500
Content-Disposition: inline
In-Reply-To: <7851597d-e582-379f-f158-8d103a895720@gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Sun, Nov 25, 2018 at 03:23:42PM +0100, Heiner Kallweit wrote:
> I wondered why ethtool suddenly reports that link partner doesn't
> support aneg and GBit modes. It turned out that this is caused by two
> bugs in conversion to linkmode bitmaps.
> 
> 1. In genphy_read_status the value of phydev->lp_advertising is
>    overwritten, thus GBit modes aren't reported any longer.
> 2. In mii_lpa_to_linkmode_lpa_t the aneg bit was overwritten by the
>    call to mii_adv_to_linkmode_adv_t.

Hi Heiner

Thanks for looking into this.

There are more bugs :-(

static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
                                             u32 lpa)
{
        if (lpa & LPA_LPACK)
                linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
                                 lp_advertising);

        mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
}

But

static inline void mii_adv_to_linkmode_adv_t(unsigned long *advertising,
                                             u32 adv)
{
        linkmode_zero(advertising);

        if (adv & ADVERTISE_10HALF)
                linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
                                 advertising);
 
So the Autoneg_BIT gets cleared.

I think the better fix is to take the linkmode_zero() out from here.

Then:

        if (adv & ADVERTISE_10HALF)
               linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
                                advertising);
+	else
+              linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+                                 advertising);

for all the bits mii_adv_to_linkmode_adv_t() looks at.

So mii_adv_to_linkmode_adv_t() only modifies bits it is responsible
for, and leaves the others alone.

    Andrew