* [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create()
@ 2023-12-14 17:06 Vladimir Oltean
2023-12-14 17:18 ` Russell King (Oracle)
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Oltean @ 2023-12-14 17:06 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, Heiner Kallweit, Russell King
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create()
2023-12-14 17:06 [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create() Vladimir Oltean
@ 2023-12-14 17:18 ` Russell King (Oracle)
2024-01-02 14:24 ` Russell King (Oracle)
0 siblings, 1 reply; 4+ messages in thread
From: Russell King (Oracle) @ 2023-12-14 17:18 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Lunn, Heiner Kallweit
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
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create()
2023-12-14 17:18 ` Russell King (Oracle)
@ 2024-01-02 14:24 ` Russell King (Oracle)
2024-01-02 16:26 ` Vladimir Oltean
0 siblings, 1 reply; 4+ messages in thread
From: Russell King (Oracle) @ 2024-01-02 14:24 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Lunn, Heiner Kallweit
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create()
2024-01-02 14:24 ` Russell King (Oracle)
@ 2024-01-02 16:26 ` Vladimir Oltean
0 siblings, 0 replies; 4+ messages in thread
From: Vladimir Oltean @ 2024-01-02 16:26 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Lunn, Heiner Kallweit
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-02 16:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-14 17:06 [PATCH net-next] net: phylink: avoid one unnecessary phylink_validate() call during phylink_create() Vladimir Oltean
2023-12-14 17:18 ` Russell King (Oracle)
2024-01-02 14:24 ` Russell King (Oracle)
2024-01-02 16:26 ` Vladimir Oltean
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).