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.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,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 47EADCA90AF for ; Tue, 12 May 2020 19:09:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2B7152492B for ; Tue, 12 May 2020 19:09:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730927AbgELTI7 (ORCPT ); Tue, 12 May 2020 15:08:59 -0400 Received: from mx2.suse.de ([195.135.220.15]:34620 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730610AbgELTI7 (ORCPT ); Tue, 12 May 2020 15:08:59 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 6683FAF9F; Tue, 12 May 2020 19:08:59 +0000 (UTC) Received: by lion.mk-sys.cz (Postfix, from userid 1000) id E7954604F3; Tue, 12 May 2020 21:08:55 +0200 (CEST) Date: Tue, 12 May 2020 21:08:55 +0200 From: Michal Kubecek To: netdev@vger.kernel.org Cc: Doug Berger , Andrew Lunn , "David S. Miller" , Florian Fainelli , Heiner Kallweit , Russell King , bcm-kernel-feedback-list@broadcom.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting Message-ID: <20200512190855.GB9071@lion.mk-sys.cz> 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: > On 5/11/2020 5:47 PM, Andrew Lunn wrote: > > On Mon, May 11, 2020 at 05:24:07PM -0700, Doug Berger wrote: > >> A comment in uapi/linux/ethtool.h states "Drivers should reject a > >> non-zero setting of @autoneg when autoneogotiation is disabled (or > >> not supported) for the link". > >> > >> That check should be added to phy_validate_pause() to consolidate > >> the code where possible. > >> > >> Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported") > > > > Hi Doug > > > > If this is a real fix, please submit this to net, not net-next. > > > > Andrew > > > 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 would be indeed unfortunate. We could use extack to make the error message easier to understand but the real problem IMHO is that ethtool_ops::get_pauseparam() returns value which is rejected by ethtool_ops::set_pauseparam(). This is something we should avoid. If we really wanted to reject ethtool_pauseparam::autoneg on when general autonegotiation is off, we would have to disable pause autonegotiation whenever general autonegotiation is disabled. I don't like that idea, however, as that would mean that ethtool -s dev autoneg off ... ethtool -s dev autoneg on ... would reset the setting of pause autonegotiation. Therefore I believe the comment should be rather replaced by a warning that even if ethtool_pauseparam::autoneg is enabled, pause autonegotiation is only active if general autonegotiation is also enabled. Michal