From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 421C6C2BB86 for ; Sat, 11 Apr 2020 23:28:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1C1AC21744 for ; Sat, 11 Apr 2020 23:28:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1586647715; bh=VJVMs1ty4aklYcknxXSyG4+HJQ94axF1UzhRTrFSIW4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=CabCOanSJriRoW5lNAfKHRfvdOA5Gk/Q+cBKmaotM3lJabrO+s8ia02fwB4Cm1Gj1 zo3UR7conBvESuRuvOUDQs3hkCmdXkRFbl7ay8DceBXbdIPndl+lTnd2X7LibvJOut VFJKk8Yc0obQYSKNVCGozlf3z2zt5diYSeZ3L/Qw= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730517AbgDKX23 (ORCPT ); Sat, 11 Apr 2020 19:28:29 -0400 Received: from mail.kernel.org ([198.145.29.99]:40494 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728821AbgDKX21 (ORCPT ); Sat, 11 Apr 2020 19:28:27 -0400 Received: from kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com (unknown [163.114.132.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9276720787; Sat, 11 Apr 2020 23:28:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1586647706; bh=VJVMs1ty4aklYcknxXSyG4+HJQ94axF1UzhRTrFSIW4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=BNMeb6gh6oZRFmHIvDKwnglHNapVVyGPi3dcxR0Swnshxs6nODmwJMN02UuRHnVZL EJtXrd+uaB/rM7humfEMsBIefc4aJGdzX6/BdcdUNLsWJqxWWdn2T0IK/Ke379Xlp4 7xouNBvYkutFYjAFMx/MbiTswjpVdlyUlo2Z+e2U= Date: Sat, 11 Apr 2020 16:28:24 -0700 From: Jakub Kicinski To: Clemens Gruber Cc: netdev@vger.kernel.org, Russell King , Andrew Lunn , Florian Fainelli , Heiner Kallweit , "David S . Miller" , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] net: phy: marvell: Fix pause frame negotiation Message-ID: <20200411162824.59791b84@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> In-Reply-To: <20200411165125.1091-1-clemens.gruber@pqgruber.com> References: <20200411165125.1091-1-clemens.gruber@pqgruber.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, 11 Apr 2020 18:51:25 +0200 Clemens Gruber wrote: > The negotiation of flow control / pause frame modes was broken since > commit fcf1f59afc67 ("net: phy: marvell: rearrange to use > genphy_read_lpa()") moved the setting of phydev->duplex below the > phy_resolve_aneg_pause call. Due to a check of DUPLEX_FULL in that > function, phydev->pause was no longer set. > > Fix it by moving the parsing of the status variable before the blocks > dealing with the pause frames. > > As the Marvell 88E1510 datasheet does not specify the timing between the > link status and the "Speed and Duplex Resolved" bit, we have to force > the link down as long as the resolved bit is not set, to avoid reporting > link up before we even have valid Speed/Duplex. > > Tested with a Marvell 88E1510 (RGMII to Copper/1000Base-T) > > Fixes: fcf1f59afc67 ("net: phy: marvell: rearrange to use genphy_read_lpa()") > Signed-off-by: Clemens Gruber > --- > Changes since v1: > - Force link to 0 if resolved bit is not set as suggested by Russell King > > drivers/net/phy/marvell.c | 46 ++++++++++++++++++++------------------- > 1 file changed, 24 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c > index 9a8badafea8a..561df5e33f65 100644 > --- a/drivers/net/phy/marvell.c > +++ b/drivers/net/phy/marvell.c > @@ -1278,6 +1278,30 @@ static int marvell_read_status_page_an(struct phy_device *phydev, > int lpa; > int err; > > + if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) { > + phydev->link = 0; > + return 0; > + } This doesn't address my comment, so was I wrong? What I was trying to say is that the function updates the established link info as well as autoneg advertising info. If the link is not resolved we can't read the link info, but we should still report the advertising modes. No?