| Message ID | 20240911212713.2178943-1-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Allow controlling PHY loopback and isolate modes | expand |
On Wed, 11 Sep 2024 23:27:04 +0200 Maxime Chevallier <maxime.chevallier@bootlin.com> wrote: > Hello everyone, > > This series brings support for controlling the isolation and loopback > modes for PHY devices, through a netlink interface. > > The isolation support work is made in preparation for the support of > interfaces that posesses multiple PHYs attached to the same MAC, on the > same MII bus. In this configuration, the isolation mode for the PHY is > used to avoid interferences on the MII bus, which doesn't support > multidrop topologies. > > This mode is part of the 802.3 spec, however rarely used. It was > discovered that some PHYs don't implement that mode correctly, and will > continue being active on the MII interface when isolated. This series > supports that case, and flags the LXT973 as having such a broken > isolation mode. The Marvell 88x3310/3340 PHYs also don't support this > mdoe, and are also flagged accordingly. > > The main part needed for the upcomping multi-PHY support really is the > internal kernel API to support this. > > The second part of the series (patches 5, 6 and 7) focus on allowing > userspace to control that mode. The only real benefit of controlling this > from userspace is to make it easier to find out if this mode really > works or not on the PHY being used. > > This relies on a new set of ethtool_phy_ops, set_config and get_config, > to toggle these modes. > > The loopback control from that API is added as it fits the API > well, and having the ability to easily set the PHY in MII-loopback > mode is a helpful tool to have when bringing-up a new device and > troubleshooting the link setup. > > The netlink API is an extension of the existing PHY_GET, reporting 2 new > attributes (one for isolate, one for loopback). A PHY_SET command is > introduced as well, allowing to configure the loopback and isolation. One thing I forgot to mention is that the phy-tunable API could also possibly be a place to set these parameters instead of this new command. Maybe this would be the preferred way ? Thanks, Maxime
> The loopback control from that API is added as it fits the API > well, and having the ability to easily set the PHY in MII-loopback > mode is a helpful tool to have when bringing-up a new device and > troubleshooting the link setup. We might want to take a step back and think about loopback some more. Loopback can be done at a number of points in the device(s). Some Marvell PHYs can do loopback in the PHY PCS layer. Some devices also support loopback in the PHY SERDES layer. I've not seen it for Marvell devices, but maybe some PHYs allow loopback much closer to the line? And i expect some MAC PCS allow loopback. So when talking about loopback, we might also want to include the concept of where the loopback occurs, and maybe it needs to be a NIC wide concept, not a PHY concept? Andrew
On 9/12/24 11:21, Andrew Lunn wrote: >> The loopback control from that API is added as it fits the API >> well, and having the ability to easily set the PHY in MII-loopback >> mode is a helpful tool to have when bringing-up a new device and >> troubleshooting the link setup. > > We might want to take a step back and think about loopback some more. > > Loopback can be done at a number of points in the device(s). Some > Marvell PHYs can do loopback in the PHY PCS layer. Some devices also > support loopback in the PHY SERDES layer. I've not seen it for Marvell > devices, but maybe some PHYs allow loopback much closer to the line? > And i expect some MAC PCS allow loopback. > > So when talking about loopback, we might also want to include the > concept of where the loopback occurs, and maybe it needs to be a NIC > wide concept, not a PHY concept? Agreed, you can loop pretty much anywhere in the data path, assuming the hardware allows it. For the hardware I maintain, we can loop back within the MAC as close as possible from the interface to DRAM, or as "far" as possible, within the MII signals, but without actually involving the PHY. Similarly, the PHY can loop as close as possible from the electrical data lines, or as far as possible by looping the *MII pins, before hitting the MAC. So if nothing else, we have at least 4 kinds of loopbacks that could be supported, it is not clear whether we want to define all of those as standardized "modes" within Linux, and let drivers implement the ones they can, or if we just let drivers implement the mode they have, and advertise those. Meaning your user experience could vary.
Hello Andrew, Florian, On Thu, 12 Sep 2024 11:26:41 -0700 Florian Fainelli <f.fainelli@gmail.com> wrote: > On 9/12/24 11:21, Andrew Lunn wrote: > >> The loopback control from that API is added as it fits the API > >> well, and having the ability to easily set the PHY in MII-loopback > >> mode is a helpful tool to have when bringing-up a new device and > >> troubleshooting the link setup. > > > > We might want to take a step back and think about loopback some more. > > > > Loopback can be done at a number of points in the device(s). Some > > Marvell PHYs can do loopback in the PHY PCS layer. Some devices also > > support loopback in the PHY SERDES layer. I've not seen it for Marvell > > devices, but maybe some PHYs allow loopback much closer to the line? > > And i expect some MAC PCS allow loopback. > > > > So when talking about loopback, we might also want to include the > > concept of where the loopback occurs, and maybe it needs to be a NIC > > wide concept, not a PHY concept? > > Agreed, you can loop pretty much anywhere in the data path, assuming the > hardware allows it. For the hardware I maintain, we can loop back within > the MAC as close as possible from the interface to DRAM, or as "far" as > possible, within the MII signals, but without actually involving the PHY. > > Similarly, the PHY can loop as close as possible from the electrical > data lines, or as far as possible by looping the *MII pins, before > hitting the MAC. > > So if nothing else, we have at least 4 kinds of loopbacks that could be > supported, it is not clear whether we want to define all of those as > standardized "modes" within Linux, and let drivers implement the ones > they can, or if we just let drivers implement the mode they have, and > advertise those. Meaning your user experience could vary. Oleksji identified some loopback modes in TI PHYs, the PHYs have access to have even different sets of loopback modes / locations as well, to me it's hard to come-up with a list of all the possible loopback locations indeed. However, I don't think it's inconceivable to come-up with a list - that can be extented - of possible loopback spots. Making the loopback a NIC concept would indeed make sense here, where we would aggregate all possible loopback points within the NIC and PHY(s) combined, and having ways for MAC/PHYS to enumerate their loopback modes through a set of ethtoop ops. With that being said, is it OK if I split the loopback part out of that series ? From the comments, it looks like a complex-enough topic to be covered on its own, and if we consider the loopback as a NIC feature, then it doesn't really fit into the current work anymore. I am however happy to continue discussing that topic. Using loopback has proven to be most helpful several times for me when bringing-up devices. Thanks, Maxime
> With that being said, is it OK if I split the loopback part out of that > series ? From the comments, it looks like a complex-enough topic to be > covered on its own, and if we consider the loopback as a NIC feature, > then it doesn't really fit into the current work anymore. > > I am however happy to continue discussing that topic. Using loopback > has proven to be most helpful several times for me when bringing-up > devices. I agree Loopback is a useful facility, and is something we should support. But i see it as being a topic of its own. So please do split it out of this patchset. Andrew
Hello everyone, This series brings support for controlling the isolation and loopback modes for PHY devices, through a netlink interface. The isolation support work is made in preparation for the support of interfaces that posesses multiple PHYs attached to the same MAC, on the same MII bus. In this configuration, the isolation mode for the PHY is used to avoid interferences on the MII bus, which doesn't support multidrop topologies. This mode is part of the 802.3 spec, however rarely used. It was discovered that some PHYs don't implement that mode correctly, and will continue being active on the MII interface when isolated. This series supports that case, and flags the LXT973 as having such a broken isolation mode. The Marvell 88x3310/3340 PHYs also don't support this mdoe, and are also flagged accordingly. The main part needed for the upcomping multi-PHY support really is the internal kernel API to support this. The second part of the series (patches 5, 6 and 7) focus on allowing userspace to control that mode. The only real benefit of controlling this from userspace is to make it easier to find out if this mode really works or not on the PHY being used. This relies on a new set of ethtool_phy_ops, set_config and get_config, to toggle these modes. The loopback control from that API is added as it fits the API well, and having the ability to easily set the PHY in MII-loopback mode is a helpful tool to have when bringing-up a new device and troubleshooting the link setup. The netlink API is an extension of the existing PHY_GET, reporting 2 new attributes (one for isolate, one for loopback). A PHY_SET command is introduced as well, allowing to configure the loopback and isolation. All in all, the userspace part is a bonus here, let me know if you think is just doesn't make sense, although the loopback feature can be useful and sent as a standalone series. In such case, I'll include the kernel-only support for isolation (so, patches 1 to 5) as part of the multi-PHY support series when it comes out. Thanks, Maxime Maxime Chevallier (7): net: phy: allow isolating PHY devices net: phy: Allow flagging PHY devices that can't isolate their MII net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode net: phy: marvell10g: 88x3310 and 88x3340 don't support isolate mode net: phy: introduce ethtool_phy_ops to get and set phy configuration net: ethtool: phy: allow reporting and setting the phy isolate status netlink: specs: introduce phy-set command along with configurable attributes Documentation/netlink/specs/ethtool.yaml | 20 +++++ Documentation/networking/ethtool-netlink.rst | 2 + drivers/net/phy/lxt.c | 2 + drivers/net/phy/marvell10g.c | 2 + drivers/net/phy/phy.c | 59 +++++++++++++ drivers/net/phy/phy_device.c | 89 ++++++++++++++++++-- include/linux/ethtool.h | 8 ++ include/linux/phy.h | 26 ++++++ include/uapi/linux/ethtool_netlink.h | 3 + net/ethtool/netlink.c | 8 ++ net/ethtool/netlink.h | 1 + net/ethtool/phy.c | 75 +++++++++++++++++ 12 files changed, 289 insertions(+), 6 deletions(-)