Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 12 May 2020 20:37:38 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Doug Berger <opendmb@gmail.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        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: <20200512183738.GD453318@lunn.ch>
References: <1589243050-18217-1-git-send-email-opendmb@gmail.com>
 <1589243050-18217-2-git-send-email-opendmb@gmail.com>
 <20200512004714.GD409897@lunn.ch>
 <ae63b295-b6e3-6c34-c69d-9e3e33bf7119@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ae63b295-b6e3-6c34-c69d-9e3e33bf7119@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

My real question is, do you think this should be back ported in
stable?  If so, it should be against net. If this is only intended for
new kernels, don't add a Fixes: tag.

> 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.

We can at least improve the error message when using netlink
ethtool. Using extack, we can pass back a string, saying why this
configuration is invalid, that link autoneg is off.

	Andrew