Re: [PATCH v3 00/11] net: phy: Add support for rate adaptation

[Sean Anderson]

On 7/25/22 11:37 AM, Sean Anderson wrote:
> This adds support for phy rate adaptation: when a phy adapts between
> differing phy interface and link speeds. It was originally submitted as
> part of [1], which is considered "v1" of this series.
> 
> We need support for rate adaptation for two reasons. First, the phy
> consumer needs to know if the phy will perform rate adaptation in order to
> program the correct advertising. An unaware consumer will only program
> support for link modes at the phy interface mode's native speed. This
> will cause autonegotiation to fail if the link partner only advertises
> support for lower speed link modes. Second, to reduce packet loss it may
> be desirable to throttle packet throughput.
> 
> There have been several past discussions [2-4] around adding rate
> adaptation support. One point is that we must be certain that rate
> adaptation is possible before enabling it. It is the opinion of some
> developers that it is the responsibility of the system integrator or end
> user to set the link settings appropriately for rate adaptation. In
> particular, it was argued that (due to differing firmware) it might not
> be clear if a particular phy has rate adaptation enabled. Additionally,
> upper-layer protocols must already be tolerant of packet loss caused by
> differing rates. Packet loss may happen anyway, such as if a faster link
> is used with a slower switch or repeater. So adjusting pause settings
> for rate adaptation is not strictly necessary.
> 
> I believe that our current approach is limiting, especially when
> considering that rate adaptation (in two forms) has made it into IEEE
> standards. In general, when we have appropriate information we should
> set sensible defaults. To consider use a contrasting example, we enable
> pause frames by default for link partners which autonegotiate for them.
> When it's the phy itself generating these frames, we don't even have to
> autonegotiate to know that we should enable pause frames.
> 
> Our current approach also encourages workarounds, such as commit
> 73a21fa817f0 ("dpaa_eth: support all modes with rate adapting PHYs").
> These workarounds are fine for phylib drivers, but phylink drivers cannot
> use this approach (since there is no direct access to the phy).
> 
> Although in earlier versions of this series, userspace could disable
> rate adaptation, now it is only possible to determine the current rate
> adaptation type. Disabling or otherwise configuring rate adaptation has
> been left for future work. However, because currently only
> RATE_ADAPT_PAUSE is implemented, it is possible to disable rate
> adaptation by modifying the advertisement appropriately.
> 
> [1] https://lore.kernel.org/netdev/20220715215954.1449214-1-sean.anderson@xxxxxxxx/T/#t
> [2] https://lore.kernel.org/netdev/1579701573-6609-1-git-send-email-madalin.bucur@xxxxxxxxxxx/
> [3] https://lore.kernel.org/netdev/1580137671-22081-1-git-send-email-madalin.bucur@xxxxxxxxxxx/
> [4] https://lore.kernel.org/netdev/20200116181933.32765-1-olteanv@xxxxxxxxx/
> 
> Changes in v3:
> - Document MAC_(A)SYM_PAUSE
> - Add some helpers for working with mac caps
> - Modify link settings directly in phylink_link_up, instead of doing
>   things more indirectly via link_*.
> - Add phylink_cap_from_speed_duplex to look up the mac capability
>   corresponding to the interface's speed.
> - Include RATE_ADAPT_CRS; it's a few lines and it doesn't hurt.
> - Move unused defines to next commit (where they will be used)
> - Remove "Support differing link/interface speed/duplex". It has been
>   rendered unnecessary due to simplification of the rate adaptation
>   patches. Thanks Russell!
> - Rewrite cover letter to better reflect the opinions of the developers
>   involved
> 
> Changes in v2:
> - Use int/defines instead of enum to allow for use in ioctls/netlink
> - Add locking to phy_get_rate_adaptation
> - Add (read-only) ethtool support for rate adaptation
> - Move part of commit message to cover letter, as it gives a good
>   overview of the whole series, and allows this patch to focus more on
>   the specifics.
> - Use the phy's rate adaptation setting to determine whether to use its
>   link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
> - Always use the rate adaptation setting to determine the interface
>   speed/duplex (instead of sometimes using the interface mode).
> - Determine the interface speed and max mac speed directly instead of
>   guessing based on the caps.
> - Add comments clarifying the register defines
> - Reorder variables in aqr107_read_rate
> 
> Sean Anderson (11):
>   net: dpaa: Fix <1G ethernet on LS1046ARDB
>   net: phy: Add 1000BASE-KX interface mode
>   net: phylink: Document MAC_(A)SYM_PAUSE
>   net: phylink: Export phylink_caps_to_linkmodes
>   net: phylink: Generate caps and convert to linkmodes separately
>   net: phylink: Add some helpers for working with mac caps
>   net: phy: Add support for rate adaptation
>   net: phylink: Adjust link settings based on rate adaptation
>   net: phylink: Adjust advertisement based on rate adaptation
>   net: phy: aquantia: Add some additional phy interfaces
>   net: phy: aquantia: Add support for rate adaptation
> 
>  Documentation/networking/ethtool-netlink.rst  |   2 +
>  .../net/ethernet/freescale/dpaa/dpaa_eth.c    |   6 +-
>  drivers/net/phy/aquantia_main.c               |  68 +++-
>  drivers/net/phy/phy-core.c                    |  15 +
>  drivers/net/phy/phy.c                         |  28 ++
>  drivers/net/phy/phylink.c                     | 302 ++++++++++++++++--
>  include/linux/phy.h                           |  26 +-
>  include/linux/phylink.h                       |  29 +-
>  include/uapi/linux/ethtool.h                  |  18 +-
>  include/uapi/linux/ethtool_netlink.h          |   1 +
>  net/ethtool/ioctl.c                           |   1 +
>  net/ethtool/linkmodes.c                       |   5 +
>  12 files changed, 466 insertions(+), 35 deletions(-)
> 

ping?

Are there any comments on this series other than about the tags for patch 6?

--Sean