Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

From: Doug Berger <opendmb@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>
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
Date: Tue, 12 May 2020 11:31:39 -0700	[thread overview]
Message-ID: <ae63b295-b6e3-6c34-c69d-9e3e33bf7119@gmail.com> (raw)
In-Reply-To: <20200512004714.GD409897@lunn.ch>

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.

The network driver could attempt to change one in response to the other,
but it might be difficult to reach consensus and it adds more
"worthless" code to the network driver in opposition to the intent.

I would like to get more feedback about the approach of the patch set as
a whole before I resubmit, and would be happy to drop this commit at
that time.

But there is still a question of how the comment "Drivers should reject
a non-zero setting of @autoneg when autoneogotiation is disabled (or not
supported) for the link" in ethtool.h should be interpreted.

Should that comment be removed?

Thanks for taking the time to look at this,
    Doug