| Message ID | 20231221180047.1924733-1-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce PHY listing and link_topology tracking | expand |
Hello: This series was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Thu, 21 Dec 2023 19:00:33 +0100 you wrote: > Hello everyone, > > Here's a V5 of the multi-PHY support series. > > At a glance, besides some minor fixes and R'd-by from Andrew, one of the > thing this series does is remove the ASSERT_RTNL() from the > topo_add_phy/del_phy operations. > > [...] Here is the summary with links: - [net-next,v5,01/13] net: phy: Introduce ethernet link topology representation https://git.kernel.org/netdev/net-next/c/02018c544ef1 - [net-next,v5,02/13] net: sfp: pass the phy_device when disconnecting an sfp module's PHY https://git.kernel.org/netdev/net-next/c/9c5625f559ad - [net-next,v5,03/13] net: phy: add helpers to handle sfp phy connect/disconnect https://git.kernel.org/netdev/net-next/c/034fcc210349 - [net-next,v5,04/13] net: sfp: Add helper to return the SFP bus name https://git.kernel.org/netdev/net-next/c/dedd702a3579 - [net-next,v5,05/13] net: ethtool: Allow passing a phy index for some commands https://git.kernel.org/netdev/net-next/c/2ab0edb505fa - [net-next,v5,06/13] netlink: specs: add phy-index as a header parameter https://git.kernel.org/netdev/net-next/c/c29451aefcb4 - [net-next,v5,07/13] net: ethtool: Introduce a command to list PHYs on an interface https://git.kernel.org/netdev/net-next/c/63d5eaf35ac3 - [net-next,v5,08/13] netlink: specs: add ethnl PHY_GET command set https://git.kernel.org/netdev/net-next/c/95132a018f00 - [net-next,v5,09/13] net: ethtool: plca: Target the command to the requested PHY https://git.kernel.org/netdev/net-next/c/7db69ec9cfb8 - [net-next,v5,10/13] net: ethtool: pse-pd: Target the command to the requested PHY https://git.kernel.org/netdev/net-next/c/345237dbc1bd - [net-next,v5,11/13] net: ethtool: cable-test: Target the command to the requested PHY https://git.kernel.org/netdev/net-next/c/fcc4b105caa4 - [net-next,v5,12/13] net: ethtool: strset: Allow querying phy stats by index https://git.kernel.org/netdev/net-next/c/d078d480639a - [net-next,v5,13/13] Documentation: networking: document phy_link_topology https://git.kernel.org/netdev/net-next/c/32bb4515e344 You are awesome, thank you!
... and I haven't reviewed this yet. I guess it's now pointless to review. On Mon, Jan 01, 2024 at 06:40:27PM +0000, patchwork-bot+netdevbpf@kernel.org wrote: > Hello: > > This series was applied to netdev/net-next.git (main) > by David S. Miller <davem@davemloft.net>: > > On Thu, 21 Dec 2023 19:00:33 +0100 you wrote: > > Hello everyone, > > > > Here's a V5 of the multi-PHY support series. > > > > At a glance, besides some minor fixes and R'd-by from Andrew, one of the > > thing this series does is remove the ASSERT_RTNL() from the > > topo_add_phy/del_phy operations. > > > > [...] > > Here is the summary with links: > - [net-next,v5,01/13] net: phy: Introduce ethernet link topology representation > https://git.kernel.org/netdev/net-next/c/02018c544ef1 > - [net-next,v5,02/13] net: sfp: pass the phy_device when disconnecting an sfp module's PHY > https://git.kernel.org/netdev/net-next/c/9c5625f559ad > - [net-next,v5,03/13] net: phy: add helpers to handle sfp phy connect/disconnect > https://git.kernel.org/netdev/net-next/c/034fcc210349 > - [net-next,v5,04/13] net: sfp: Add helper to return the SFP bus name > https://git.kernel.org/netdev/net-next/c/dedd702a3579 > - [net-next,v5,05/13] net: ethtool: Allow passing a phy index for some commands > https://git.kernel.org/netdev/net-next/c/2ab0edb505fa > - [net-next,v5,06/13] netlink: specs: add phy-index as a header parameter > https://git.kernel.org/netdev/net-next/c/c29451aefcb4 > - [net-next,v5,07/13] net: ethtool: Introduce a command to list PHYs on an interface > https://git.kernel.org/netdev/net-next/c/63d5eaf35ac3 > - [net-next,v5,08/13] netlink: specs: add ethnl PHY_GET command set > https://git.kernel.org/netdev/net-next/c/95132a018f00 > - [net-next,v5,09/13] net: ethtool: plca: Target the command to the requested PHY > https://git.kernel.org/netdev/net-next/c/7db69ec9cfb8 > - [net-next,v5,10/13] net: ethtool: pse-pd: Target the command to the requested PHY > https://git.kernel.org/netdev/net-next/c/345237dbc1bd > - [net-next,v5,11/13] net: ethtool: cable-test: Target the command to the requested PHY > https://git.kernel.org/netdev/net-next/c/fcc4b105caa4 > - [net-next,v5,12/13] net: ethtool: strset: Allow querying phy stats by index > https://git.kernel.org/netdev/net-next/c/d078d480639a > - [net-next,v5,13/13] Documentation: networking: document phy_link_topology > https://git.kernel.org/netdev/net-next/c/32bb4515e344 > > You are awesome, thank you! > -- > Deet-doot-dot, I am a bot. > https://korg.docs.kernel.org/patchwork/pwbot.html > > >
On Tue, 2 Jan 2024 11:57:09 +0000 Russell King (Oracle) wrote: > ... and I haven't reviewed this yet. I guess it's now pointless to > review. I guess the shutdown was only a partial success. Nobody cleaned out pending stuff on the 23rd, and old things got applied now before we even officially reopened :( It is what it is, please review anyway, we'll be reverting things which shouldn't have been applied..
Hi Russell, Jakub, On Tue, 2 Jan 2024 10:51:25 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 2 Jan 2024 11:57:09 +0000 Russell King (Oracle) wrote: > > ... and I haven't reviewed this yet. I guess it's now pointless to > > review. > > I guess the shutdown was only a partial success. Nobody cleaned out > pending stuff on the 23rd, and old things got applied now before we > even officially reopened :( It is what it is, please review anyway, > we'll be reverting things which shouldn't have been applied.. I've submitted the ethtool counterpart of that work a few seconds ago : https://lore.kernel.org/netdev/20240103142950.235888-1-maxime.chevallier@bootlin.com/T/#m334b7cec4be1c78d399d0899a30d522ab57b4622 I think this could help in reviewing the overall design and identifying any glaring issue with this. Thanks, Maxime
On Wed, 3 Jan 2024 15:33:36 +0100 Maxime Chevallier wrote: > I think this could help in reviewing the overall design and identifying > any glaring issue with this. The netlink handling looks a bit wobbly to me. I commented best I could in the 20min I had to look at this code :( I think it'd be best to revert, if you don't mind, because reviewing incremental fixes will get even harder.
On 1/4/24 15:47, Jakub Kicinski wrote: > On Wed, 3 Jan 2024 15:33:36 +0100 Maxime Chevallier wrote: >> I think this could help in reviewing the overall design and identifying >> any glaring issue with this. > > The netlink handling looks a bit wobbly to me. > I commented best I could in the 20min I had to look at this code :( > I think it'd be best to revert, if you don't mind, because reviewing > incremental fixes will get even harder. +1
On Thu, 4 Jan 2024 15:50:40 -0800 Florian Fainelli wrote: > > The netlink handling looks a bit wobbly to me. > > I commented best I could in the 20min I had to look at this code :( > > I think it'd be best to revert, if you don't mind, because reviewing > > incremental fixes will get even harder. > > +1 Okay, let's say that the three of us - Florian, Russell and I are a quorum? :) Reverted.
Hi Jakub, On Thu, 4 Jan 2024 16:56:09 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 4 Jan 2024 15:50:40 -0800 Florian Fainelli wrote: > > > The netlink handling looks a bit wobbly to me. > > > I commented best I could in the 20min I had to look at this code :( > > > I think it'd be best to revert, if you don't mind, because reviewing > > > incremental fixes will get even harder. > > > > +1 > > Okay, let's say that the three of us - Florian, Russell > and I are a quorum? :) Reverted. No worries, I'd rather make sure we get this right especially regarding the API. Thanks for taking a look, Maxime
Hello everyone, Here's a V5 of the multi-PHY support series. At a glance, besides some minor fixes and R'd-by from Andrew, one of the thing this series does is remove the ASSERT_RTNL() from the topo_add_phy/del_phy operations. These operations will take a PHY device and put it into the list of devices associated to a netdevice. The main thing to protect here is the list itself, but since we use xarrays, my naive understanding of it is that it contains its own protection scheme. There shouldn't be a need for more locking, as the insertion/deletion paths are already hooked into the PHY connection to a netdev, or disconnection from it. Now for the rest of the cover : As a remainder, this ongoing work aims ultimately at supporting complex link topologies that involve multiplexing multiple PHYs/SFPs on a single netdevice. As a first step, it's required that we are able to enumerate the PHYs on a given ethernet interface. By just doing so, we also improve already-existing use-cases, namely the copper SFP modules support when a media-converter is used (as we have 2 PHYs on the link, but only one is referenced by net_device.phydev, which is used on a variety of netlink commands). The series is architectured as follows : - The first patch adds the notion of phy_link_topology, which tracks all PHYs attached to a netdevice. - Patches 2, 3 and 4 adds some plumbing into SFP and phylib to be able to connect the dots when building the topology tree, to know which PHY is connected to which SFP bus, trying not to be too invasive on phylib. - Patch 5 allows passing a PHY_INDEX to ethnl commands. I'm uncertain about this, as there are at least 4 netlink commands ( 5 with the one introduced in patch 7 ) that targets PHYs directly or indirectly, which to me makes it worth-it to have a generic way to pass a PHY index to commands, however the approach taken may be too generic. - Patch 6 is the netlink spec update + ethtool-user.c|h autogenerated code update (the autogenerated code triggers checkpatch warning though) - Patch 7 introduces a new netlink command set to list PHYs on a netdevice. It implements a custom DUMP and GET operation to allow filtered dumps, that lists all PHYs on a given netdevice. I couldn't use most of ethnl's plumbing though. - Patch 8 is the netlink spec update + ethtool-user.c|h update for that new command - Patch 8,9,10 and 11 updates the PLCA, strset, cable-test and pse netlink commands to use the user-provided PHY instead of net_device.phydev. - Finally patch 12 adds some documentation for this whole work. Examples ======== Here's a short overview of the kind of operations you can have regarding the PHY topology. These tests were performed on a MacchiatoBin, which has 3 interfaces : eth0 and eth1 have the following layout: MAC - PHY - SFP eth2 has this more classic topology : MAC - PHY - RJ45 finally eth3 has the following topology : MAC - SFP When performing a dump with all interfaces down, we don't get any result, as no PHY has been attached to their respective net_device : # ./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml --dump phy-get None The following output is with eth0, eth2 and eth3 up, but no SFP module inserted in none of the interfaces : # ./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml --dump phy-get [{'downstream-sfp-name': 'sfp-eth0', 'drvname': 'mv88x3310', 'header': {'dev-index': 2, 'dev-name': 'eth0'}, 'id': 0, 'index': 1, 'name': 'f212a600.mdio-mii:00', 'upstream-type': 'mac'}, {'drvname': 'Marvell 88E1510', 'header': {'dev-index': 4, 'dev-name': 'eth2'}, 'id': 21040593, 'index': 1, 'name': 'f212a200.mdio-mii:00', 'upstream-type': 'mac'}] And now is a dump operation with a copper SFP in the eth0 port : # ./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml --dump phy-get [{'downstream-sfp-name': 'sfp-eth0', 'drvname': 'mv88x3310', 'header': {'dev-index': 2, 'dev-name': 'eth0'}, 'id': 0, 'index': 1, 'name': 'f212a600.mdio-mii:00', 'upstream-type': 'mac'}, {'drvname': 'Marvell 88E1111', 'header': {'dev-index': 2, 'dev-name': 'eth0'}, 'id': 21040322, 'index': 2, 'name': 'i2c:sfp-eth0:16', 'upstream': {'index': 1, 'sfp-name': 'sfp-eth0'}, 'upstream-type': 'phy'}, {'drvname': 'Marvell 88E1510', 'header': {'dev-index': 4, 'dev-name': 'eth2'}, 'id': 21040593, 'index': 1, 'name': 'f212a200.mdio-mii:00', 'upstream-type': 'mac'}] -- Note that this shouldn't actually work as the 88x3310 PHY doesn't allow a 1G SFP to be connected to its SFP interface, and I don't have a 10G copper SFP, so for the sake of the demo I applied the following modification, which of courses gives a non-functionnal link, but the PHY attach still works, which is what I want to demonstrate : @@ -488,7 +488,7 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id) if (iface != PHY_INTERFACE_MODE_10GBASER) { dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n"); - return -EINVAL; + //return -EINVAL; } return 0; } Finally an example of the filtered DUMP operation that Jakub suggested in V1 : # ./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml \ # --dump phy-get --json '{"header" : {"dev-name" : "eth0"}}' [{'downstream-sfp-name': 'sfp-eth0', 'drvname': 'mv88x3310', 'header': {'dev-index': 2, 'dev-name': 'eth0'}, 'id': 0, 'index': 1, 'name': 'f212a600.mdio-mii:00', 'upstream-type': 'mac'}, {'drvname': 'Marvell 88E1111', 'header': {'dev-index': 2, 'dev-name': 'eth0'}, 'id': 21040322, 'index': 2, 'name': 'i2c:sfp-eth0:16', 'upstream': {'index': 1, 'sfp-name': 'sfp-eth0'}, 'upstream-type': 'phy'}] And a classic GET operation allows querying a single PHY's info : # ./cli.py --spec specs/ethtool.yaml --schema genetlink-legacy.yaml \ # --do phy-get --json '{"header" : {"dev-name" : "eth0", "phy-index" : 2}}' {'drvname': 'Marvell 88E1111', 'header': {'dev-index': 2, 'dev-name': 'eth0'}, 'id': 21040322, 'index': 2, 'name': 'i2c:sfp-eth0:16', 'upstream': {'index': 1, 'sfp-name': 'sfp-eth0'}, 'upstream-type': 'phy'} Changed in V5: - Removed the RTNL assertion in the topology ops - Made the phy_topo_get_phy inline - Fixed the PSE-PD multi-PHY support by re-adding a wrongly dropped check - Fixed some typos in the documentation - Fixed reverse xmas trees Changes in V4: - Dropped the RFC flag - Made the net_device integration independent to having phylib enabled - Removed the autogenerated ethtool-user code for the YNL specs Changes in V3: - Added RTNL assertions where needed - Fixed issues in the DUMP code for PHY_GET, which crashed when running it twice in a row - Added the documentation, and moved in-source docs around - renamed link_topology to phy_link_topology Changes in V2: - Added the DUMP operation - Added much more information in the reported data, to be able to reconstruct precisely the topology tree - renamed phy_list to link_topology Maxime Chevallier (13): net: phy: Introduce ethernet link topology representation net: sfp: pass the phy_device when disconnecting an sfp module's PHY net: phy: add helpers to handle sfp phy connect/disconnect net: sfp: Add helper to return the SFP bus name net: ethtool: Allow passing a phy index for some commands netlink: specs: add phy-index as a header parameter net: ethtool: Introduce a command to list PHYs on an interface netlink: specs: add ethnl PHY_GET command set net: ethtool: plca: Target the command to the requested PHY net: ethtool: pse-pd: Target the command to the requested PHY net: ethtool: cable-test: Target the command to the requested PHY net: ethtool: strset: Allow querying phy stats by index Documentation: networking: document phy_link_topology Documentation/netlink/specs/ethtool.yaml | 68 ++++ Documentation/networking/ethtool-netlink.rst | 51 +++ Documentation/networking/index.rst | 1 + .../networking/phy-link-topology.rst | 121 +++++++ MAINTAINERS | 2 + drivers/net/phy/Makefile | 2 +- drivers/net/phy/at803x.c | 2 + drivers/net/phy/marvell-88x2222.c | 2 + drivers/net/phy/marvell.c | 2 + drivers/net/phy/marvell10g.c | 2 + drivers/net/phy/phy_device.c | 55 ++++ drivers/net/phy/phy_link_topology.c | 66 ++++ drivers/net/phy/phylink.c | 3 +- drivers/net/phy/sfp-bus.c | 15 +- include/linux/netdevice.h | 4 +- include/linux/phy.h | 6 + include/linux/phy_link_topology.h | 67 ++++ include/linux/phy_link_topology_core.h | 19 ++ include/linux/sfp.h | 8 +- include/uapi/linux/ethtool.h | 16 + include/uapi/linux/ethtool_netlink.h | 30 ++ net/core/dev.c | 3 + net/ethtool/Makefile | 2 +- net/ethtool/cabletest.c | 12 +- net/ethtool/netlink.c | 33 ++ net/ethtool/netlink.h | 12 +- net/ethtool/phy.c | 306 ++++++++++++++++++ net/ethtool/plca.c | 13 +- net/ethtool/pse-pd.c | 9 +- net/ethtool/strset.c | 15 +- 30 files changed, 912 insertions(+), 35 deletions(-) create mode 100644 Documentation/networking/phy-link-topology.rst create mode 100644 drivers/net/phy/phy_link_topology.c create mode 100644 include/linux/phy_link_topology.h create mode 100644 include/linux/phy_link_topology_core.h create mode 100644 net/ethtool/phy.c