Received-SPF: neutral (google.com: 209.132.180.67 is neither permitted nor denied by best guess record for domain of linux-kernel-owner@vger.kernel.org) client-ip=209.132.180.67;
Date:   Sun, 12 Apr 2020 19:54:47 +0100
From:   Russell King - ARM Linux admin <linux@armlinux.org.uk>
To:     Clemens Gruber <clemens.gruber@pqgruber.com>
Cc:     Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: phy: marvell: Fix pause frame negotiation
Message-ID: <20200412185447.GN25745@shell.armlinux.org.uk>
References: <20200408214326.934440-1-clemens.gruber@pqgruber.com>
 <20200410174304.22f812fd@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <20200411091705.GG25745@shell.armlinux.org.uk>
 <20200411132401.GA273086@workstation.tuxnet>
 <20200411134344.GI25745@shell.armlinux.org.uk>
 <20200412170336.GA1826@workstation.tuxnet>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200412170336.GA1826@workstation.tuxnet>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Apr 12, 2020 at 07:03:36PM +0200, Clemens Gruber wrote:
> On Sat, Apr 11, 2020 at 02:43:44PM +0100, Russell King - ARM Linux admin wrote:
> > The fiber code is IMHO very suspect; the decoding of the pause status
> > seems to be completely broken. However, I'm not sure whether anyone
> > actually uses that or not, so I've been trying not to touch it.
> 
> If the following table for the link partner advertisement is correct..
> PAUSE           ASYM_PAUSE      MEANING
> 0               0               Link partner has no pause frame support
> 0               1               <-  Link partner can TX pause frames
> 1               0               <-> Link partner can RX and TX pauses
> 1               1                -> Link partner can RX pause frames
> 
> ..then I think both pause and asym_pause have to be assigned
> independently, like this:
> phydev->pause = !!(lpa & LPA_1000XPAUSE);
> phydev->asym_pause = !!(lpa & LPA_1000XPAUSE_ASYM);

Yes, that's how it should be, because the pause and asym pause bits
correspond exactly with the phydev members.

> (Using the defines from uapi mii.h instead of the redundant/combined
> LPA_PAUSE_FIBER etc. which can then be removed from marvell.c)
> 
> Currently, if LPA_1000XPAUSE_ASYM is set we do pause=1 and asym_pause=1
> no matter if LPA_1000XPAUSE is set. This could lead us to mistake a link
> partner who can only send for one who can only receive pause frames.
> ^ Was this the problem you meant?

Exactly, but given that I've no way to actually test anything with
regard to 1G Marvell PHYs using 1000BASE-X, I have to assume that
whoever contributed this code tested it and it worked for them. So,
it should not be changed just because it looks wrong - there may be
some subtle issues in the hardware that we don't know about that
makes this code "do the best it can". We need someone who can
actually do some tests to solve this.

> Does anybody have access to a Marvell PHY with 1000base-X Ethernet?
> (I only have a 88E1510 + 1000Base-T at the home office)

Yes, that's what we need... this isn't the first time I've mentioned
the problem, and so far no one has stepped forward.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up