| Message ID | 20220725153730.2604096-1-sean.anderson@seco.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: Add support for rate adaptation | expand |
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@seco.com/T/#t > [2] https://lore.kernel.org/netdev/1579701573-6609-1-git-send-email-madalin.bucur@oss.nxp.com/ > [3] https://lore.kernel.org/netdev/1580137671-22081-1-git-send-email-madalin.bucur@oss.nxp.com/ > [4] https://lore.kernel.org/netdev/20200116181933.32765-1-olteanv@gmail.com/ > > 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
On Thu, Aug 18, 2022 at 11:21:10AM -0400, Sean Anderson wrote: > > > 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. > ping? > > Are there any comments on this series other than about the tags for patch 6? Anything that old is going to need a rebase. So you may as well repost rather than ping. Andrew
On 8/18/22 11:41 AM, Andrew Lunn wrote: > On Thu, Aug 18, 2022 at 11:21:10AM -0400, Sean Anderson wrote: >> >> >> 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. > >> ping? >> >> Are there any comments on this series other than about the tags for patch 6? > > Anything that old is going to need a rebase. So you may as well repost > rather than ping. OK I have some other series [1,2] which have also received little-to-no feedback. Should I resend those as well? I didn't ping about these earlier because there was a merge window open, and e.g. gregkh has told me not to ping at that time (since series will be reviewed afterwards). Should I be pinging sooner on netdev? --Sean [1] https://lore.kernel.org/linux-phy/20220804220602.477589-1-sean.anderson@seco.com/ [2] https://lore.kernel.org/netdev/20220725151039.2581576-1-sean.anderson@seco.com/
> I have some other series [1,2] which have also received little-to-no feedback. Should > I resend those as well? I didn't ping about these earlier because there was a merge > window open, and e.g. gregkh has told me not to ping at that time (since series will > be reviewed afterwards). Should I be pinging sooner on netdev? For netdev, you need to resend after the merge window. Other subsystems might look at older patch, and if they apply cleanly might merge them. But not netdev. Andrew
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@seco.com/T/#t [2] https://lore.kernel.org/netdev/1579701573-6609-1-git-send-email-madalin.bucur@oss.nxp.com/ [3] https://lore.kernel.org/netdev/1580137671-22081-1-git-send-email-madalin.bucur@oss.nxp.com/ [4] https://lore.kernel.org/netdev/20200116181933.32765-1-olteanv@gmail.com/ 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(-)