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

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

[Thilo Zirnstein]

If the net device is phylink based,
consists of a mac with a phy connected and the
mac driver does add the phy while the ndo_open callback,
initial advertising settings of the link may be overwritten
during phylink_bringup_phy.

The mode of action is as follows:

* The link was not up yet - no phy connected
* the user configures the advertisement of the interface
* the mac driver delegates the call to phylink_ethtool_ksettings_set
    * here we are not configuring the phy - it does not exist yet
* the user set up the interface
* the mac drivers calls phylink_connect_phy
    * calls phylink_bringup_phy
        * overwrites advertisement with
            * linkmode_copy(config.advertising, phy->advertising);
            * linkmode_copy(pl->link_config.advertising,
                config.advertising);

Connecting the phy while ndo_open callback seems to be a common pattern,
affected drivers are:

* macb
* r8169
* stmmac
* tn40xx

The advertising settings can be kept by using the intersection
of the phylink advert bits and the phy advert bits.

This was observed and tested on a
Xilinx Zyqnmp system with builtin macb ethernet interface.

Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
Signed-off-by: Thilo Zirnstein <thi.zirnstein@gmail.com>
---
 drivers/net/phy/phylink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e885da1c2831..145f181bc9d3 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);
 	linkmode_copy(pl->link_config.advertising, config.advertising);

 	/* Restrict the phy advertisement according to the MAC support. */
--
2.43.0

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

[Andrew Lunn]

On Wed, Nov 04, 2026 at 01:55:09PM +0100, Thilo Zirnstein wrote:
> If the net device is phylink based,
> consists of a mac with a phy connected and the
> mac driver does add the phy while the ndo_open callback,
> initial advertising settings of the link may be overwritten
> during phylink_bringup_phy.
> 
> The mode of action is as follows:
> 
> * The link was not up yet - no phy connected
> * the user configures the advertisement of the interface
> * the mac driver delegates the call to phylink_ethtool_ksettings_set
>     * here we are not configuring the phy - it does not exist yet

If it does not exist, it is not possible to validate the user request
is actually supported by the PHY.

> * the user set up the interface
> * the mac drivers calls phylink_connect_phy

At this point, is the previously configured value sanitised by what
the PHY can actually do?

Take the example of a MAC which can do up to 2500BaseX, but it is
connected to a PHY which only supports SGMII. Before up, the user
could request 2.5GBaseT, but at admin up that needs to be reduced down
to 1G.

	Andrew

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

[Thilo Zirnstein]

Hi Andrew,

Thanks for your feedback,

On Fri, Nov 06, 2026 at 02:51:18PM +0100, Andrew Lunn wrote:
...
> > The mode of action is as follows:
> > 
> > * The link was not up yet - no phy connected
> > * the user configures the advertisement of the interface
> > * the mac driver delegates the call to phylink_ethtool_ksettings_set
> >     * here we are not configuring the phy - it does not exist yet
> 
> If it does not exist, it is not possible to validate the user request
> is actually supported by the PHY.

I think my description was a bit misleading.
The mode of action describes how the phy is attached by some
net driver and what the program flow is and why there is a problem.

Typically the device driver does not set/clear any advertisment bits
during device bring up. As least what I have seen so far.
It attaches the phy and brings up the link.

> 
> > * the user set up the interface
> > * the mac drivers calls phylink_connect_phy
> 
> At this point, is the previously configured value sanitised by what
> the PHY can actually do?
> 
> Take the example of a MAC which can do up to 2500BaseX, but it is
> connected to a PHY which only supports SGMII. Before up, the user
> could request 2.5GBaseT, but at admin up that needs to be reduced down
> to 1G.

Good example, with this patch applied this will advertise 1G,
if the user does not configure exclusive 2.5G prior attaching the phy.
If the user configures exclusive 2.5G (2.5G only) the advertisment is empty.
I think this is fine because the user missconfigured the link, but maybe
i am to restrictive.

I wrote a little table for a better overview. I am not 100% sure
how your example behaves because 2.5G uses another interface then SGMII.
I have tested with a sgmii 1G half/full duplex link.

Without patch

without phy (user):     phy caps        link up advertisement
1G FD+HD                1G FD+HD        1G FD+HD
1G FD                   1G FD+HD        1G FD+HD <= bad
1G FD+HD                1G FD           1G FD

With patch

without phy (user):     phy caps    link up advertisement
1G FD+HD                1G FD+HD    1G FD+HD   
1G FD                   1G FD+HD    1G FD <= good
1G FD+HD                1G FD       1G FD

> 	Andrew

Thilo

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

[netdev-bot+sashiko]

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

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

[Anandu Padinhare]

On Wed, Nov 04, 2026 at 01:55:09PM +0100, Thilo Zirnstein wrote:
> Connecting the phy while ndo_open callback seems to be a common pattern,
> affected drivers are:
>
> * macb
> * r8169
> * stmmac
> * tn40xx

Hi Thilo,

Results from an ADSP-SC598 board (stmmac, RGMII, TI DP83867 PHY) with
a 6.18-based kernel, with and without this patch.

1. The case in the commit message is fixed, with both the dp83867 and
   the generic PHY driver:

     ip link set eth0 down
     ethtool -s eth0 speed 100 duplex full autoneg on
     ip link set eth0 up

   Without the patch the link comes up at 1000 Mb/s; with it, at
   100 Mb/s.

2. In that same case the pause bits are lost: with the patch the link
   comes up with flow control off; without it, pause stays on. Same
   with both PHY drivers. While the interface is down, ethtool shows
   no advertised modes and no pause, before and after the
   "ethtool -s" (phylink_ethtool_ksettings_get() reports none when no
   PHY is attached), and the AND then drops the pause bits the PHY
   advertises.

3. Sashiko's sequences (a) and (b) both reproduce with the dp83867
   driver:

   (a) advertise 100baseT/Full while up, ifdown/ifup: 100 Mb/s.
       advertise 10/100/1000baseT/Full while up: 1000 Mb/s.
       ifdown/ifup: back to 100 Mb/s, advertising only 100baseT/Full.

   (b) continuing, advertise 1000baseT/Full while up: 1000 Mb/s.
       ifdown/ifup: "Advertised link modes: Not reported", and the
       link stays down (NO-CARRIER).

   Without the patch, both keep the last setting: (a) comes back at
   1000 Mb/s, and (b) at 1000 Mb/s with 1000baseT/Full advertised.

Happy to test a v2 on this board.