| Message ID | 20231214170659.868759-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create() | expand |
I'll say this for the benefit of the netdev developers - my time is currently being spent elsewhere (e.g. ARM64 VCPU hotplug) so I don't have a lot of time to review netdev patches. With only a few days left to Christmas vacations and my intention not to work over that period, this means that I'm unlikely to be able to review changes such as this - and they do need review. If I get some spare time, then I will review them. However, probably best to wait until the new year before sending patches that touch phylink - which will involve adding stress on my side. Thanks. On Thu, Dec 14, 2023 at 07:06:59PM +0200, Vladimir Oltean wrote: > The direct phylink_validate() call from phylink_create() is partly > redundant, since there will be subsequent calls to phylink_validate() > issued by phylink_parse_mode() for MLO_AN_INBAND, and by > phylink_parse_fixedlink() for MLO_AN_FIXED. These will overwrite what > the initial phylink_validate() call already did. > > The only exception is MLO_AN_PHY, for which phylink_validate() might be > called with a slight delay (the timing of the phylink_bringup_phy() call > is not under phylink's control). User space could issue > phylink_ethtool_ksettings_get() calls before phylink_bringup_phy(), and > could thus see an empty pl->supported, which this early phylink_validate() > call prevents. > > So we can delay the direct phylink_create() -> phylink_validate() call > until after phylink_parse_mode() and phylink_parse_fixedlink(), and execute > it only for the mode where it makes any difference at all - MLO_AN_PHY. > > This has the benefit that we issue one phylink_validate() call less, for > some deployments. The visible output remains unchanged in all cases. > > Link: https://lore.kernel.org/netdev/20231004222523.p5t2cqaot6irstwq@skbuf/ > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > The other, non-immediate benefit has to do with potential future API > extensions. With this change, pl->cfg_link_an_mode is now parsed and > available to phylink every time phylink_validate() is called. So it is > possible to pass it to pcs_validate(), if that ever becomes necessary, > for example with the introduction of a separate MLO_AN_* mode for clause > 73 auto-negotiation (i.e. in-band selection of state->interface). > > I don't think this extra information should go into the commit message, > since these plans may or may not materialize. They are just extra > information to give reviewers context. The change is useful even if the > plans do not materialize. > > drivers/net/phy/phylink.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 4adf8ff3ac31..65bff93b1bd8 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -1620,10 +1620,6 @@ struct phylink *phylink_create(struct phylink_config *config, > __set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state); > timer_setup(&pl->link_poll, phylink_fixed_poll, 0); > > - linkmode_fill(pl->supported); > - linkmode_copy(pl->link_config.advertising, pl->supported); > - phylink_validate(pl, pl->supported, &pl->link_config); > - > ret = phylink_parse_mode(pl, fwnode); > if (ret < 0) { > kfree(pl); > @@ -1636,6 +1632,17 @@ struct phylink *phylink_create(struct phylink_config *config, > kfree(pl); > return ERR_PTR(ret); > } > + } else if (pl->cfg_link_an_mode == MLO_AN_PHY) { > + /* phylink_bringup_phy() will recalculate pl->supported with > + * information from the PHY, but it may take a while until it > + * is called, and we should report something to user space > + * until then rather than nothing at all, to avoid issues. > + * Just report all link modes supportable by the current > + * phy_interface_t and the MAC capabilities. > + */ > + linkmode_fill(pl->supported); > + linkmode_copy(pl->link_config.advertising, pl->supported); > + phylink_validate(pl, pl->supported, &pl->link_config); > } > > pl->cur_link_an_mode = pl->cfg_link_an_mode; > -- > 2.34.1 > >
On Thu, Dec 14, 2023 at 05:18:35PM +0000, Russell King (Oracle) wrote: > I'll say this for the benefit of the netdev developers - my time is > currently being spent elsewhere (e.g. ARM64 VCPU hotplug) so I don't > have a lot of time to review netdev patches. With only a few days > left to Christmas vacations and my intention not to work over that > period, this means that I'm unlikely to be able to review changes > such as this - and they do need review. > > If I get some spare time, then I will review them. > > However, probably best to wait until the new year before sending > patches that touch phylink - which will involve adding stress on my > side. So... getting back to this as I have platforms I can test with in front of me now... On October 5th, you asked by email what the purpose of this was. I replied same day, stating the purpose of it, citing mvneta. I did state in that email about reporting "absolutely nothing" to userspace, but in fact it's the opposite - without this validate() call, we report absolutely everything. With it removed, booting my Armada 388 Clearfog platform with the ethernet interface not having been brought up, and then running ethtool on it, this is what I get: # ethtool eno0 Settings for eno0: Supported ports: [ TP AUI MII FIBRE BNC Backplane ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Half 1000baseT/Full 10000baseT/Full 2500baseX/Full 1000baseKX/Full 10000baseKX4/Full 10000baseKR/Full 10000baseR_FEC 20000baseMLD2/Full 20000baseKR2/Full 40000baseKR4/Full 40000baseCR4/Full 40000baseSR4/Full 40000baseLR4/Full 56000baseKR4/Full 56000baseCR4/Full 56000baseSR4/Full 56000baseLR4/Full 25000baseCR/Full 25000baseKR/Full 25000baseSR/Full 50000baseCR2/Full 50000baseKR2/Full 100000baseKR4/Full 100000baseSR4/Full 100000baseCR4/Full 100000baseLR4_ER4/Full 50000baseSR2/Full 1000baseX/Full 10000baseCR/Full 10000baseSR/Full 10000baseLR/Full 10000baseLRM/Full 10000baseER/Full 2500baseT/Full 5000baseT/Full 50000baseKR/Full 50000baseSR/Full 50000baseCR/Full 50000baseLR_ER_FR/Full 50000baseDR/Full 100000baseKR2/Full 100000baseSR2/Full 100000baseCR2/Full 100000baseLR2_ER2_FR2/Full 100000baseDR2/Full 200000baseKR4/Full 200000baseSR4/Full 200000baseLR4_ER4_FR4/Full 200000baseDR4/Full 200000baseCR4/Full 100baseT1/Full 1000baseT1/Full 400000baseKR8/Full 400000baseSR8/Full 400000baseLR8_ER8_FR8/Full 400000baseDR8/Full 400000baseCR8/Full 100000baseKR/Full 100000baseSR/Full 100000baseLR_ER_FR/Full 100000baseCR/Full 100000baseDR/Full 200000baseKR2/Full 200000baseSR2/Full 200000baseLR2_ER2_FR2/Full 200000baseDR2/Full 200000baseCR2/Full 400000baseKR4/Full 400000baseSR4/Full 400000baseLR4_ER4_FR4/Full 400000baseDR4/Full 400000baseCR4/Full 100baseFX/Half 100baseFX/Full 10baseT1L/Full 800000baseCR8/Full 800000baseKR8/Full 800000baseDR8/Full 800000baseDR8_2/Full 800000baseSR8/Full 800000baseVR8/Full 10baseT1S/Full 10baseT1S/Half 10baseT1S_P2MP/Half Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: None RS BASER LLRS Advertised link modes: Not reported Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: Unknown! Duplex: Half Auto-negotiation: off Port: MII PHYAD: 0 Transceiver: internal Supports Wake-on: d Wake-on: d Link detected: no Does that look like sensible output for a network interface that can only do up to 2.5G speeds to you? The phylink_validate() there serves to restrict the link modes to something sensible in the case where we are expecting a PHY to be attached to the interface, but the MAC driver attaches the PHY at open time, but the network interface has yet to be opened. So... I completely disagree with removing this phylink_validate() call - it's there so we report something sane to userspace for network drivers that attach PHYs at open time before that PHY is attached.
Hello Russell, On Tue, Jan 02, 2024 at 02:24:17PM +0000, Russell King (Oracle) wrote: > So... getting back to this as I have platforms I can test with in front > of me now... On October 5th, you asked by email what the purpose of > this was. I replied same day, stating the purpose of it, citing mvneta. > I did state in that email about reporting "absolutely nothing" to > userspace, but in fact it's the opposite - without this validate() > call, we report absolutely everything. > > With it removed, booting my Armada 388 Clearfog platform with the > ethernet interface not having been brought up, and then running ethtool > on it, this is what I get: > > Does that look like sensible output for a network interface that can > only do up to 2.5G speeds to you? I wish you a happy and healthy new year. No, it does not look like sensible output to me. But I have a very simple question. Did you test "with phylink_validate() removed", or with my patch applied as-is? Because there is a very big difference between these 2 things. My patch does not intend to change the behavior, and I have tested it on a mvneta-based system that it does not. It should not produce the behavior you present above. Before patch: [root@mox:~] # ethtool end0 Settings for end0: Supported ports: [ TP AUI MII FIBRE BNC Backplane ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full 1000baseKX/Full 1000baseX/Full 100baseT1/Full 1000baseT1/Full 100baseFX/Half 100baseFX/Full 10baseT1L/Full 10baseT1S/Full 10baseT1S/Half 10baseT1S_P2MP/Half Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: Not reported Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: Unknown! Duplex: Half Auto-negotiation: off Port: MII PHYAD: 0 Transceiver: internal Supports Wake-on: d Wake-on: d Link detected: no [root@mox:~] # ip link set end0 up [ 105.603188] mvneta d0030000.ethernet end0: PHY [d0032004.mdio-mii:01] driver [Marvell 88E1510] (irq=POLL) [ 105.615130] mvneta d0030000.ethernet end0: configuring for phy/rgmii-id link mode [root@mox:~] # [ 108.706615] mvneta d0030000.ethernet end0: Link is Up - 1Gbps/Full - flow control rx/tx [root@mox:~] # ethtool end0 Settings for end0: Supported ports: [ TP MII FIBRE ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Link partner advertised pause frame use: Symmetric Receive-only Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Auto-negotiation: on Port: Twisted Pair PHYAD: 1 Transceiver: external MDI-X: Unknown Supports Wake-on: pg Wake-on: d Link detected: yes After patch: [root@mox:~] # ethtool end0 Settings for end0: Supported ports: [ TP AUI MII FIBRE BNC Backplane ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full 1000baseKX/Full 1000baseX/Full 100baseT1/Full 1000baseT1/Full 100baseFX/Half 100baseFX/Full 10baseT1L/Full 10baseT1S/Full 10baseT1S/Half 10baseT1S_P2MP/Half Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: Not reported Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: Unknown! Duplex: Half Auto-negotiation: off Port: MII PHYAD: 0 Transceiver: internal Supports Wake-on: d Wake-on: d Link detected: no [root@mox:~] # ip link set end0 up [ 35.890744] mvneta d0030000.ethernet end0: PHY [d0032004.mdio-mii:01] driver [Marvell 88E1510] (irq=POLL) [ 35.900834] mvneta d0030000.ethernet end0: configuring for phy/rgmii-id link mode [root@mox:~] # [ 40.002662] mvneta d0030000.ethernet end0: Link is Up - 1Gbps/Full - flow control rx/tx [root@mox:~] # ethtool end0 Settings for end0: Supported ports: [ TP MII FIBRE ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Link partner advertised pause frame use: Symmetric Receive-only Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Auto-negotiation: on Port: Twisted Pair PHYAD: 1 Transceiver: external MDI-X: Unknown Supports Wake-on: pg Wake-on: d Link detected: yes Putting the "before" and "after" logs through the "meld" program, I see the result is identical.
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 4adf8ff3ac31..65bff93b1bd8 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1620,10 +1620,6 @@ struct phylink *phylink_create(struct phylink_config *config, __set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state); timer_setup(&pl->link_poll, phylink_fixed_poll, 0); - linkmode_fill(pl->supported); - linkmode_copy(pl->link_config.advertising, pl->supported); - phylink_validate(pl, pl->supported, &pl->link_config); - ret = phylink_parse_mode(pl, fwnode); if (ret < 0) { kfree(pl); @@ -1636,6 +1632,17 @@ struct phylink *phylink_create(struct phylink_config *config, kfree(pl); return ERR_PTR(ret); } + } else if (pl->cfg_link_an_mode == MLO_AN_PHY) { + /* phylink_bringup_phy() will recalculate pl->supported with + * information from the PHY, but it may take a while until it + * is called, and we should report something to user space + * until then rather than nothing at all, to avoid issues. + * Just report all link modes supportable by the current + * phy_interface_t and the MAC capabilities. + */ + linkmode_fill(pl->supported); + linkmode_copy(pl->link_config.advertising, pl->supported); + phylink_validate(pl, pl->supported, &pl->link_config); } pl->cur_link_an_mode = pl->cfg_link_an_mode;
The direct phylink_validate() call from phylink_create() is partly redundant, since there will be subsequent calls to phylink_validate() issued by phylink_parse_mode() for MLO_AN_INBAND, and by phylink_parse_fixedlink() for MLO_AN_FIXED. These will overwrite what the initial phylink_validate() call already did. The only exception is MLO_AN_PHY, for which phylink_validate() might be called with a slight delay (the timing of the phylink_bringup_phy() call is not under phylink's control). User space could issue phylink_ethtool_ksettings_get() calls before phylink_bringup_phy(), and could thus see an empty pl->supported, which this early phylink_validate() call prevents. So we can delay the direct phylink_create() -> phylink_validate() call until after phylink_parse_mode() and phylink_parse_fixedlink(), and execute it only for the mode where it makes any difference at all - MLO_AN_PHY. This has the benefit that we issue one phylink_validate() call less, for some deployments. The visible output remains unchanged in all cases. Link: https://lore.kernel.org/netdev/20231004222523.p5t2cqaot6irstwq@skbuf/ Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- The other, non-immediate benefit has to do with potential future API extensions. With this change, pl->cfg_link_an_mode is now parsed and available to phylink every time phylink_validate() is called. So it is possible to pass it to pcs_validate(), if that ever becomes necessary, for example with the introduction of a separate MLO_AN_* mode for clause 73 auto-negotiation (i.e. in-band selection of state->interface). I don't think this extra information should go into the commit message, since these plans may or may not materialize. They are just extra information to give reviewers context. The change is useful even if the plans do not materialize. drivers/net/phy/phylink.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)