Re: [PATCH net v1 1/1] net: phylink: keep initial advertising settings

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 3 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In phylink_bringup_phy() (drivers/net/phy/phylink.c:2323), the
  new linkmode_and() limits the PHY's validated advertisement to…
- [High] phylink_bringup_phy() now reads pl->link_config.advertising as
  the user's requested advertisement.
- [High] For a PHY on an SFP module, phylink_sfp_config_phy() calls
  phylink_bringup_phy() (line 3891) before phylink_sfp_set_config()…
- [Medium] The fix carries over only part of a pre-open link
  configuration.
- [Medium] phylink_bringup_phy() can now give phylib an autoneg
  advertisement with no link modes in it.

--

net: phylink: keep initial advertising settings

This patch keeps an advertisement that was set through ethtool before the
MAC driver connects the PHY in ndo_open. phylink_bringup_phy() now ANDs
the PHY's validated advertisement with pl->link_config.advertising. The
result is stored back into both pl->link_config.advertising and
phy->advertising.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e885da1c28311..145f181bc9d37 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2320,6 +2320,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
>  	pl->phy_state.duplex = DUPLEX_UNKNOWN;
>  	pl->phy_state.rate_matching = RATE_MATCH_NONE;
>  	linkmode_copy(pl->supported, supported);
> +	linkmode_and(config.advertising, config.advertising, pl->link_config.advertising);

[Severity: High]
Can this narrow the PHY advertisement on the first ndo_open, before the
user has configured anything?

If nothing has been set through ethtool, pl->link_config.advertising
still holds the default from phylink_create(), or from
phylink_parse_mode() for in-band:

    linkmode_fill(pl->supported);
    linkmode_copy(pl->link_config.advertising, pl->supported);
    phylink_validate(pl, pl->supported, &pl->link_config);

That mask is validated only for the DT phy-mode, with rate_matching left
at RATE_MATCH_NONE.

phylink_validate_phy() deliberately validates more widely. It uses
phy->possible_interfaces or the PHY's rate matching mode, and for C45
PHYs it does this:

    if (phy->is_c45 && state->rate_matching == RATE_MATCH_NONE &&
        ...)
        state->interface = PHY_INTERFACE_MODE_NA;

The AND here undoes that widening, and the narrowed result is then
copied into phy->advertising.

Two examples:

- phy-mode = "10gbase-r" with an 88X3310 or AQR class PHY: the default
  only has MAC_10000FD modes.
- phy-mode = "2500base-x" with an RTL8221B class PHY: only MAC_2500FD
  survives.

Wouldn't the PHY then advertise only 10GBASE-T (or 2500BASE-T)? Before
this patch it could link with lower speed partners; after it, those
links would fail.

The AND can't tell a user setting apart from phylink's own
interface-limited default. Would user intent need to be tracked
separately?

[Severity: Medium]
Can the intersection end up with no link modes here?

The AND runs after phylink_validate_phy(). Nothing checks for an empty
result or re-validates it before it is copied into phy->advertising.

While no PHY is attached, pl->supported holds every MAC mode for the
interface. For MAC_1000FD that includes 1000baseX_Full and
1000baseKX_Full. So "ethtool -s ethX advertise 1000baseX/Full" is
accepted while the interface is down.

At ndo_open, ANDing the copper PHY's baseT modes with that mask leaves
only Autoneg and Pause. phydev->autoneg stays enabled.

The other two configuration paths reject this state.
phylink_ethtool_ksettings_set() has:

    /* If autonegotiation is enabled, we must have an advertisement */
    if (linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
                          config.advertising) &&
        phylink_is_empty_linkmode(config.advertising))
        return -EINVAL;

phy_ethtool_ksettings_set() also returns -EINVAL for an empty
advertisement with autoneg enabled.

phylink_bringup_phy() returns 0 here, and the link silently never comes
up. Should the result be checked, or fall back to the PHY's validated
advertisement?

The missing check also turns the stale link_config and SFP cases below
into a total link loss.

>  	linkmode_copy(pl->link_config.advertising, config.advertising);

[Severity: High]
Does pl->link_config.advertising still reflect what the user asked for
once a PHY has been attached?

With a PHY present, phylink_ethtool_ksettings_set() passes the request
to phylib and returns without touching link_config:

    /* We can rely on phylib for this update; we also do not need
     * to update the pl->link_config settings:
     ...
     */
    return phy_ethtool_ksettings_set(pl->phydev, &phy_kset);

This line writes the intersection back into pl->link_config.advertising,
and phylink_disconnect_phy() never resets it. Non-genphy PHY drivers
also keep phy->advertising across phy_detach() and phy_attach(). On
macb, stmmac, r8169 and tn40xx, the mask can therefore only lose bits
across ndo_stop and ndo_open.

Two sequences show this:

(a) "ethtool -s advertise 100baseT/Full" while up, then ifdown/ifup,
leaves link_config at {100M}. A later ethtool -s with the full set
while up is reverted to {100M} at the next ifdown/ifup.

(b) "ethtool -s advertise 100baseT/Full", ifdown/ifup, then
"ethtool -s advertise 1000baseT/Full", ifdown/ifup, gives {1G} AND
{100M}.

In case (b) only the Autoneg and Pause bits are left. Wouldn't the PHY
then advertise no speed modes, so the link never comes up until the
driver is rebound?

[Severity: High]
How does this behave for PHYs on SFP modules?

phylink_sfp_config_phy() calls phylink_bringup_phy() before it updates
link_config for the new module:

    ret = phylink_bringup_phy(pl, phy, config.interface);
    ...
    phylink_sfp_set_config(pl, support, &config, true);

So the AND uses whatever pl->link_config.advertising was left by the
cage default or by a previously inserted module. Neither module removal
nor PHY disconnect resets it.

Some examples:

(1) A 1000BASE-SX module leaves {1000baseX_Full, Autoneg, Pause, FIBRE}.
If a copper SGMII module (e.g. 88E1111) is inserted next, its baseT
modes are ANDed with that mask, which has no speed bits.

(2) If an optical module's only interface has no in-band AN,
phylink_sfp_config_optical() calls linkmode_zero(config.advertising).
That leaves link_config.advertising entirely zero for the next copper
SFP PHY.

(3) A cage with managed = "in-band-status" and phy-mode 1000base-x only
gets MAC_1000FD modes from phylink_parse_mode(). Even the first copper
module inserted would lose its 10/100 modes.

Right after bringup, phylink_sfp_set_config() overwrites
link_config.advertising with the module's full mask. Doesn't that leave
phylink's recorded advertisement out of sync with what the PHY was
actually programmed with?

>  
>  	/* Restrict the phy advertisement according to the MAC support. */

[Severity: Medium]
Is only half of a forced link configuration set before open carried
over here?

Without a PHY, phylink_ethtool_ksettings_set() handles
"ethtool -s ethX autoneg off speed 100 duplex full" in two steps. It
stores the speed and duplex in link_config.speed and
link_config.duplex, and it clears the Autoneg bit:

    linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, config.advertising,
                     kset->base.autoneg == AUTONEG_ENABLE);

At bringup only the bitmap is applied, through the new AND and the
following linkmode_copy(phy->advertising, config.advertising).
phydev->autoneg, phydev->speed and phydev->duplex are never updated.

So the PHY keeps autonegotiating and ignores the forced mode, as it did
before the patch. The difference is that phy->advertising now lacks the
Autoneg bit while phydev->autoneg is still AUTONEG_ENABLE.

phy_ethtool_ksettings_set() always keeps these two consistent. Is this
a state phylib expects? ethtool would now report autoneg on while
Autoneg is not advertised.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104125509.3221743-1-thi.zirnstein%40gmail.com