[PATCH net] net: explicitly check in of_phy_is_fixed_link() for managed = "in-band-status"

[PATCH net] net: explicitly check in of_phy_is_fixed_link() for managed = "in-band-status"

[Vladimir Oltean]

In phylib-based systems, there are at least two ways of handling a link
to a PHY whose registers are unavailable over MDIO. The better known
implementation is phylink, the lesser known one was added by Stas
Sergeev two years prior, in commit 898b2970e2c9 ("mvneta: implement
SGMII-based in-band link state signaling") and its various follow-ups.

There are two sub-cases of the MDIO-less PHY to consider. First is the
case where the PHY would at least emit in-band autoneg code groups.
The firmware description of this case looks like below (a):

	mac {
		managed = "in-band-status";
		phy-mode = "sgmii";
	};

And the other sub-case is when the MDIO-less PHY is also silent on the
in-band autoneg front. In that case, the firmware description would look
like this (b):

	mac {
		phy-mode = "sgmii";

		fixed-link {
			speed = <10000>;
			full-duplex;
		};
	};

(side note: phylink would probably have something to object against the
PHY not reporting its state in any way, and would consider the setup
invalid, even if in some cases it would work. This is because its
configuration may not be fixed, and there would be no way to be notified
of updates)

Concentrating on sub-case (a), Stas Sergeev's mvneta implementation
differs from the later phylink implementation which also took over in
mvneta.

In the well known phylink model, the phylib PHY is completely optional,
and the pl->cfg_link_an_mode will be placed in MLO_AN_INBAND.

Whereas Stas Sergeev admittedly took "the path of least resistance" and
worked with what was available, i.e. the fixed PHY software emulation:
https://lore.kernel.org/lkml/55156730.5030807@list.ru/

Commit 4cba5c210365 ("of_mdio: add new DT property 'managed' to specify
the PHY management type") made of_phy_is_fixed_link() return true for
sub-case (a), so that the fixed PHY driver would handle it. From
forensic evidence, I believe that was done to have unified phylib driver
handling with sub-case (b).

We want to preserve that behavior, but if other values for the "managed"
property have to be introduced, it means of_phy_is_fixed_link() will
automatically return true for them. As a general rule, that doesn't make
any sense. For example, managed = "c73" may be added to mean that the
operating interface of a port is selected through IEEE 802.3 clause 73
(backplane) auto-negotiation.

So, we need to be explicit about the check.

Documentation/devicetree/bindings/net/ethernet-controller.yaml makes it
clear that the 2 allowed values for "managed" are "auto" and
"in-band-status". So, given the current binding, strcmp(managed, "auto") != 0
should be exactly equivalent with strcmp(managed, "in-band-status") == 0.
The difference is made for new additions.

The Fixes: tag and backport to stable is justified by the fact that new
device trees need to do something reasonable with old kernels.

Fixes: 4cba5c210365 ("of_mdio: add new DT property 'managed' to specify the PHY management type")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/mdio/of_mdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 98f667b121f7..8e8a34293a8b 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -401,7 +401,7 @@ bool of_phy_is_fixed_link(struct device_node *np)
 	}
 
 	err = of_property_read_string(np, "managed", &managed);
-	if (err == 0 && strcmp(managed, "auto") != 0)
+	if (err == 0 && strcmp(managed, "in-band-status") == 0)
 		return true;
 
 	/* Old binding */
-- 
2.34.1

Re: [PATCH net] net: explicitly check in of_phy_is_fixed_link() for managed = "in-band-status"

[Russell King (Oracle)]

On Tue, Aug 12, 2025 at 01:59:28PM +0300, Vladimir Oltean wrote:
> And the other sub-case is when the MDIO-less PHY is also silent on the
> in-band autoneg front. In that case, the firmware description would look
> like this (b):
> 
> 	mac {
> 		phy-mode = "sgmii";
> 
> 		fixed-link {
> 			speed = <10000>;
> 			full-duplex;
> 		};
> 	};
> 
> (side note: phylink would probably have something to object against the
> PHY not reporting its state in any way, and would consider the setup
> invalid, even if in some cases it would work. This is because its
> configuration may not be fixed, and there would be no way to be notified
> of updates)

Both of these are fully supported by phylink, and your side note is
incorrect. Phylink provides all the functionality.

With the description in (b), if a MAC driver wishes to, it can provide
phylink_config->get_fixed_state() and override the speed, duplex and
pause in the same way that is possible with fixed PHY.

So, unless I missed something, I don't think your commit description
is correct. If it is correct, it is ambiguous.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: explicitly check in of_phy_is_fixed_link() for managed = "in-band-status"

[Russell King (Oracle)]

On Tue, Aug 12, 2025 at 12:22:40PM +0100, Russell King (Oracle) wrote:
> On Tue, Aug 12, 2025 at 01:59:28PM +0300, Vladimir Oltean wrote:
> > And the other sub-case is when the MDIO-less PHY is also silent on the
> > in-band autoneg front. In that case, the firmware description would look
> > like this (b):
> > 
> > 	mac {
> > 		phy-mode = "sgmii";
> > 
> > 		fixed-link {
> > 			speed = <10000>;
> > 			full-duplex;
> > 		};
> > 	};
> > 
> > (side note: phylink would probably have something to object against the
> > PHY not reporting its state in any way, and would consider the setup
> > invalid, even if in some cases it would work. This is because its
> > configuration may not be fixed, and there would be no way to be notified
> > of updates)
> 
> Both of these are fully supported by phylink, and your side note is
> incorrect. Phylink provides all the functionality.
> 
> With the description in (b), if a MAC driver wishes to, it can provide
> phylink_config->get_fixed_state() and override the speed, duplex and
> pause in the same way that is possible with fixed PHY.
> 
> So, unless I missed something, I don't think your commit description
> is correct. If it is correct, it is ambiguous.

As an additional point, I'm not sure what has broken that justifies
this change for the net tree. You mention at one point in the commit
description about wanting to use "c73" as a string for "managed",
which suggests new development, and thus shouldn't this be targetting
net-next?

Note that at present, all dts files in the kernel either omit the
managed property, or have it present with value "in-band-status".

Thus, I think the commit makes sense for net-next.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: explicitly check in of_phy_is_fixed_link() for managed = "in-band-status"

[Vladimir Oltean]

On Tue, Aug 12, 2025 at 12:22:39PM +0100, Russell King (Oracle) wrote:
> On Tue, Aug 12, 2025 at 01:59:28PM +0300, Vladimir Oltean wrote:
> > And the other sub-case is when the MDIO-less PHY is also silent on the
> > in-band autoneg front. In that case, the firmware description would look
> > like this (b):
> > 
> > 	mac {
> > 		phy-mode = "sgmii";
> > 
> > 		fixed-link {
> > 			speed = <10000>;
> > 			full-duplex;
> > 		};
> > 	};
> > 
> > (side note: phylink would probably have something to object against the
> > PHY not reporting its state in any way, and would consider the setup
> > invalid, even if in some cases it would work. This is because its
> > configuration may not be fixed, and there would be no way to be notified
> > of updates)
> 
> Both of these are fully supported by phylink, and your side note is
> incorrect. Phylink provides all the functionality.
> 
> With the description in (b), if a MAC driver wishes to, it can provide
> phylink_config->get_fixed_state() and override the speed, duplex and
> pause in the same way that is possible with fixed PHY.
> 
> So, unless I missed something, I don't think your commit description
> is correct. If it is correct, it is ambiguous.

Hmm, ok, it seems I was missing something very fundamental, thanks for
pointing me in the correct direction. I'll remove the side note altogether.

I didn't make the connection with phylink_config->get_fixed_state()
because I didn't realize it can overwrite more than state->link.

So I see the txgbe driver implements get_fixed_state() to report the
status of the firmware-managed PHY to phylink, in MLO_AN_FIXED mode.
That's interesting. I think dpaa2 could also do that for its
DPMAC_LINK_TYPE_FIXED mode. Currently it avoids instantiating a phylink.

(I realize we're diverging, but is the initial speed passed to
phylink_set_fixed_link() particularly relevant, or can it be arbitrary
as long as it's supported by the mac_capabilities, and as long as
get_fixed_state() can overwrite it? Can the speed be SPEED_UNKNOWN?
I don't understand why txgbe passes SPEED_25000 if it provides
get_fixed_state())

By the way, while reviewing implementations, I found bcm_sf2_sw_fixed_state()
to manually call netif_carrier_off(). Is that valid, to alter the
phylink-managed carrier state? Doesn't phylink_resolve() ->
phylink_link_down() call netif_carrier_off() already?

Re: [PATCH net] net: explicitly check in of_phy_is_fixed_link() for managed = "in-band-status"

[Vladimir Oltean]

On Tue, Aug 12, 2025 at 12:33:36PM +0100, Russell King (Oracle) wrote:
> As an additional point, I'm not sure what has broken that justifies
> this change for the net tree. You mention at one point in the commit
> description about wanting to use "c73" as a string for "managed",
> which suggests new development, and thus shouldn't this be targetting
> net-next?
> 
> Note that at present, all dts files in the kernel either omit the
> managed property, or have it present with value "in-band-status".
> 
> Thus, I think the commit makes sense for net-next.

Correct, thanks for challenging this, nothing seems to be broken if
stable kernels are left alone in their thinking that managed = "c73"
would be a fixed link. It was out of caution that old and new kernels
would disagree on this when faced with the same device tree, but it does
not seem to matter. I will re-target the patch to net-next.