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
