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(-)

-- 
2.46.0

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.
-- 
Florian

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