Message-Id: <1589243050-18217-1-git-send-email-opendmb@gmail.com>
Date:   Mon, 11 May 2020 17:24:06 -0700
From:   Doug Berger <opendmb@...il.com>
To:     "David S. Miller" <davem@...emloft.net>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Doug Berger <opendmb@...il.com>
Subject: [PATCH net-next 0/4] Extend phylib implementation of pause support

This commit set extends the implementation introduced by
commit 5652b46e4e80 ("Merge branch 'Pause-updates-for-phylib-and-phylink'")
to support the less problematic behavior alluded to by Russell King's
comment in commit f904f15ea9b5 ("net: phylink: allow ethtool -A to
change flow control advertisement").

+	/*
+	 * See the comments for linkmode_set_pause(), wrt the deficiencies
+	 * with the current implementation.  A solution to this issue would
+	 * be:
+	 * ethtool  Local device
+	 *  rx  tx  Pause AsymDir
+	 *  0   0   0     0
+	 *  1   0   1     1
+	 *  0   1   0     1
+	 *  1   1   1     1
+	 * and then use the ethtool rx/tx enablement status to mask the
+	 * rx/tx pause resolution.
+	 */

Specifically, the linkmode_set_pause() function is extended to support
both the existing Pause/AsymPause mapping and the mapping specified by
the IEEE standard (and Russell). A phy_set_pause() function is added to
the phylib that can make use of this extension based on the value of
the pause autoneg parameter. The bcmgenet driver adds support for the
ethtool pauseparam ops based on these phylib services and uses "the
ethtool rx/tx enablement status to mask the rx/tx pause resolution".

The first commit in this set addresses a small deficiency in the 
phy_validate_pause() function.

The second extends linkmode_set_pause() with an autoneg parameter to
allow selection of the desired mapping for advertisement.

The third introduces the phy_set_pause() function based on the existing
phy_set_asym_pause() implementation. One aberration here is the direct
manipulation of the phy state machine to allow a new link up event to
notify the MAC that the pause parameters may have changed. This is a
convenience to simplify the MAC driver by allowing one implementation
of the pause configuration logic to be located in its adjust_link
callback. Otherwise, the MAC driver would need to handle configuring
the pause parameters for an already active PHY link which would likely
require additional synchronization logic to protect the logic from
asynchronous changes in the PHY state.

The logic in phy_set_asym_pause() that looks for a change in
advertising is not particularly helpful here since now a change from
tx=1 rx=1 to tx=0 rx=1 no longer changes the advertising if autoneg is
enabled so phy_start_aneg() would not be called. I took the alternate
approach of unconditionally calling phy_start_aneg() since it
accommodates both manual and autoneg configured links. The "aberrant"
logic allows manually configured and autonegotiated links that don't
change their advertised parameters to receive an adjust_link call to
act on pause parameters that have no effect on the PHY layer.

It seemed excessive to bring the PHY down and back up when nogotiation
is not necessary, but that could be an alternative approach. I am
certainly open to any suggestions on how to improve that portion of
the code if it is controversial and a consensus can be reached.

The last commit is a reference implementation of pause support by the
bcmgenet driver based on my preferences for the functionality. It is my
desire that other network drivers prefer this behavior and the changes
to the phylib will make it easier for them to support.

Many thanks to Russell King and Andrew Lunn for their efforts to clean
up and centralize support for pause and to document its shortcommings.

Doug Berger (4):
  net: ethernet: validate pause autoneg setting
  net: add autoneg parameter to linkmode_set_pause
  net: ethernet: introduce phy_set_pause
  net: bcmgenet: add support for ethtool flow control

 drivers/net/ethernet/broadcom/genet/bcmgenet.c |  54 +++++++++++++
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |   4 +
 drivers/net/ethernet/broadcom/genet/bcmmii.c   |  38 +++++++--
 drivers/net/phy/linkmode.c                     | 104 +++++++++++++++++++------
 drivers/net/phy/phy_device.c                   |  37 ++++++++-
 drivers/net/phy/phylink.c                      |   6 +-
 include/linux/linkmode.h                       |   3 +-
 include/linux/phy.h                            |   1 +
 8 files changed, 212 insertions(+), 35 deletions(-)

-- 
2.7.4

Powered by blists - more mailing lists