diff mbox series

What is the purpose of the first phylink_validate() call from phylink_create()?

Message ID 20231004222523.p5t2cqaot6irstwq@skbuf (mailing list archive)
State Not Applicable
Headers show
Series What is the purpose of the first phylink_validate() call from phylink_create()? | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1339 this patch: 1339
netdev/cc_maintainers warning 6 maintainers not CCed: hkallweit1@gmail.com andrew@lunn.ch pabeni@redhat.com davem@davemloft.net edumazet@google.com kuba@kernel.org
netdev/build_clang success Errors and warnings before: 1363 this patch: 1363
netdev/verify_signedoff fail author Signed-off-by missing
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1363 this patch: 1363
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 10 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vladimir Oltean Oct. 4, 2023, 10:25 p.m. UTC
Hi Russell,

In phylink_create() we have this code which populates pl->supported with
a maximal link mode configuration and then makes a best-effort attempt
to reduce it to what the physical port actually supports:


However:

- in MLO_AN_FIXED mode, the later call to phylink_parse_fixedlink() will
  overwrite this pl->supported and pl->link_config.advertising with
  another set

- in MLO_AN_INBAND mode, the later call to phylink_parse_mode() will
  also overwrite pl->supported and pl->link_config.advertising

- with a PHY (either in MLO_AN_INBAND or MLO_AN_PHY modes),
  phylink_bringup_phy() will overwrite pl->supported and
  pl->link_config.advertising with stuff from the PHY

Of these 3 cases, phylink_bringup_phy() is the only one which
potentially does not come immediately after phylink_create().
So, the effect of the phylink_validate() from phylink_create() will be
visible only when it's not overwritten, for example when phylink_connect_phy()
(or one of variants) isn't called at probe time but is delayed until
ndo_open().

Since mvneta calls phylink_of_phy_connect() from mvneta_open() and I can
test that, I'm comparing the "ethtool" output produced before running
"ip link set dev eth0 up", in 2 cases:

- With the phylink_validate() from phylink_create() kept in place:

$ ethtool eth0
Settings for eth0:
        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

- And with it removed (the diff from the beginning):

$ ethtool eth0
Settings for eth0:
        Supported ports: [  ]
        Supported link modes:   Not reported
        Supported pause frame use: No
        Supports auto-negotiation: No
        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

But I'm not sure that this ethtool output is very valuable to user space?
At this stage it is essentially just the output of phylink_generic_validate()
for the MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD capabilities using a
gigabit phy_interface_t. It is subject to change as more link modes get
introduced. The output with no link modes reported until the PHY connects
seems at least equally reasonable, given that the PHY dictates the link modes.

So what is the purpose of the early phylink_validate() call and the
associated population of pl->supported? Is it just to report some link
modes until we have a PHY and we're not in-band, or am I missing something?

On a visual inspection, this code structure exists since commit
9525ae83959b ("phylink: add phylink infrastructure"), but I cannot test
as far back as that to be absolutely sure.

For reference, here is the ethtool output when the port has been brought up:

$ ethtool eth0
Settings for eth0:
        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

Comments

Russell King (Oracle) Oct. 5, 2023, 9:16 a.m. UTC | #1
On Thu, Oct 05, 2023 at 01:25:23AM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> In phylink_create() we have this code which populates pl->supported with
> a maximal link mode configuration and then makes a best-effort attempt
> to reduce it to what the physical port actually supports:
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3951e5af8cb5..1e89634ec8ae 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1677,10 +1677,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);
>  
> -	bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> -	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);
> 
> However:
> 
> - in MLO_AN_FIXED mode, the later call to phylink_parse_fixedlink() will
>   overwrite this pl->supported and pl->link_config.advertising with
>   another set
> 
> - in MLO_AN_INBAND mode, the later call to phylink_parse_mode() will
>   also overwrite pl->supported and pl->link_config.advertising
> 
> - with a PHY (either in MLO_AN_INBAND or MLO_AN_PHY modes),
>   phylink_bringup_phy() will overwrite pl->supported and
>   pl->link_config.advertising with stuff from the PHY
> 
> Of these 3 cases, phylink_bringup_phy() is the only one which
> potentially does not come immediately after phylink_create().
> So, the effect of the phylink_validate() from phylink_create() will be
> visible only when it's not overwritten, for example when phylink_connect_phy()
> (or one of variants) isn't called at probe time but is delayed until
> ndo_open().
> 
> Since mvneta calls phylink_of_phy_connect() from mvneta_open() and I can
> test that, I'm comparing the "ethtool" output produced before running
> "ip link set dev eth0 up", in 2 cases:
> 
> - With the phylink_validate() from phylink_create() kept in place:
> 
> $ ethtool eth0
> Settings for eth0:
>         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
> 
> - And with it removed (the diff from the beginning):
> 
> $ ethtool eth0
> Settings for eth0:
>         Supported ports: [  ]
>         Supported link modes:   Not reported
>         Supported pause frame use: No
>         Supports auto-negotiation: No
>         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

You've found the exact reason for it - so that we report something that
seems at least reasonable to userspace, rather than reporting absolutely
nothing which may cause issues.

The original code in mvneta would've done this:

int mvneta_ethtool_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
{
        struct mvneta_port *pp = netdev_priv(dev);

        if (!pp->phy_dev)
                return -ENODEV;

        return phy_ethtool_gset(pp->phy_dev, cmd);
}

Thus making the call fail if the device wasn't up - and that may be
an alternative if we're expecting a PHY but we have none.
Vladimir Oltean Oct. 5, 2023, 10:44 a.m. UTC | #2
On Thu, Oct 05, 2023 at 10:16:48AM +0100, Russell King (Oracle) wrote:
> You've found the exact reason for it - so that we report something that
> seems at least reasonable to userspace, rather than reporting absolutely
> nothing which may cause issues.

Thanks for confirming. I don't need to change the user-observable behavior,
I think I can work my way around it. I just wanted to know what to look for,
and I deleted the phylink_validate() call just to exaggerate the effect.

> The original code in mvneta would've done this:
> 
> int mvneta_ethtool_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> {
>         struct mvneta_port *pp = netdev_priv(dev);
> 
>         if (!pp->phy_dev)
>                 return -ENODEV;
> 
>         return phy_ethtool_gset(pp->phy_dev, cmd);
> }
> 
> Thus making the call fail if the device wasn't up - and that may be
> an alternative if we're expecting a PHY but we have none.

Ok, but I admit I don't know how to make phylink_ethtool_ksettings_get()
return -ENODEV just for this case. For example, I'm thinking of the
situation of a copper SFP module with an inaccessible PHY, using SGMII.
My understanding is that phylink_expects_phy() would return true, so
that helper couldn't be used to discern this kind of SFP from a PHY
which is accessible but phylink_bringup_phy() wasn't yet called on it.

In any case, please consider the question answered for now with no other
actionable item.

Also, I now see that patchwork thinks this question is a patch
(https://patchwork.kernel.org/project/netdevbpf/patch/20231004222523.p5t2cqaot6irstwq@skbuf/),
so:

pw-bot: not-applicable
diff mbox series

Patch

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 3951e5af8cb5..1e89634ec8ae 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1677,10 +1677,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);
 
-	bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
-	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);