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=-2.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no 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 2B6B3CA90AF for ; Tue, 12 May 2020 18:55:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CA9FD23126 for ; Tue, 12 May 2020 18:55:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=armlinux.org.uk header.i=@armlinux.org.uk header.b="axN1tHeY" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730980AbgELSzU (ORCPT ); Tue, 12 May 2020 14:55:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51336 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725554AbgELSzT (ORCPT ); Tue, 12 May 2020 14:55:19 -0400 Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:3201:214:fdff:fe10:1be6]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 55A95C061A0C; Tue, 12 May 2020 11:55:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=DuQnObfma1WkHbZTabF7H6jt3JFEZVYaxX/fV7nd448=; b=axN1tHeYZOvpIpOkUd9+uCAo6 qPudqbtLx7aB8HRwCO1SUYvxdvgSs13lLUn73AP/d4yPNbDnNxhAzT8Gxluo3LQwl4yEzEUeNJHjc c4QPOo4TThM90ylJhfmyIEMullmaS2E2x9waVkDBwZzi21sh2msQQnMkdvk/aLEDS5pSXRXL4jgjR 5taA7oIzbvUBgWr/89+vLjOdC0Ho0PloE0E1Oxjx6ofqcwnb4Qz0NXUE6X9zXU7HH1WOQgNN1jIfZ SHOjKDnp7byR+fp5rtU0Wb/+Fy8JMDOQhRJ0ZUGOJVsRAodVINVY6pF2Hx5Zgshgbx0TRaDJpJ4gP JnoAUeCIg==; Received: from shell.armlinux.org.uk ([2002:4e20:1eda:1:5054:ff:fe00:4ec]:57156) by pandora.armlinux.org.uk with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jYa3Y-0002OC-Vh; Tue, 12 May 2020 19:55:09 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.92) (envelope-from ) id 1jYa3T-0006yT-FF; Tue, 12 May 2020 19:55:03 +0100 Date: Tue, 12 May 2020 19:55:03 +0100 From: Russell King - ARM Linux admin To: Doug Berger Cc: Andrew Lunn , "David S. Miller" , Florian Fainelli , Heiner Kallweit , bcm-kernel-feedback-list@broadcom.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting Message-ID: <20200512185503.GD1551@shell.armlinux.org.uk> References: <1589243050-18217-1-git-send-email-opendmb@gmail.com> <1589243050-18217-2-git-send-email-opendmb@gmail.com> <20200512004714.GD409897@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote: > This was intended as a fix, but I thought it would be better to keep it > as part of this set for context and since net-next is currently open. > > The context is trying to improve the phylib support for offloading > ethtool pause configuration and this is something that could be checked > in a single location rather than by individual drivers. > > I included it here to get feedback about its appropriateness as a common > behavior. I should have been more explicit about that. > > Personally, I'm actually not that fond of this change since it can > easily be a source of confusion with the ethtool interface because the > link autonegotiation and the pause autonegotiation are controlled by > different commands. > > Since the ethtool -A command performs a read/modify/write of pause > parameters, you can get strange results like these: > # ethtool -s eth0 speed 100 duplex full autoneg off > # ethtool -A eth0 tx off > Cannot set device pause parameters: Invalid argument > # > Because, the get read pause autoneg as enabled and only the tx_pause > member of the structure was updated. This looks like the same argument I've been having with Heiner over the EEE interface, except there's a difference here. # ethtool -A eth0 autoneg on # ethtool -s eth0 autoneg off speed 100 duplex full After those two commands, what is the state of pause mode? The answer is, it's disabled. # ethtool -A eth0 autoneg off rx on tx on is perfectly acceptable, as we are forcing pause modes at the local end of the link. # ethtool -A eth0 autoneg on Now, the question is whether that should be allowed or not - but this is merely restoring the "pause" settings that were in effect prior to the previous command. It does not enable pause negotiation, because autoneg as a whole is disabled, but it _allows_ pause negotiation to occur when autoneg is enabled at some point in the future. Also, allowing "ethtool -A eth0 autoneg on" when "ethtool -s eth0 autoneg off" means you can configure the negotiation parameters _before_ triggering a negotiation cycle on the link. In other words, it would avoid: # ethtool -s eth0 autoneg on # # Link renegotiates # ethtool -A eth0 autoneg on # # Link renegotiates a second time and it also means that if stuff has already been scripted to avoid this, nothing breaks. If we start rejecting ethtool -A because autoneg is disabled, then things get difficult to configure - we would need ethtool documentation to state that autoneg must be enabled before configuration of pause and EEE can be done. IMHO, that hurts usability, and adds confusion. -- 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