Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

On Sun, Feb 14, 2021 at 10:35:29AM +0000, Russell King - ARM Linux admin wrote:

quoted

+	if (ret && ret != -EOPNOTSUPP) {
+		phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n",
+			     ret);

Please use %pe and ERR_PTR(ret) so we can get a symbolic errno value.

I didn't know that was possible, thanks for the hint.

As mentioned in this thread, we have at least one PHY which is unable
to provide the inband signalling in any mode (BCM84881). Currently,
phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and
adjusts not to use inband mode. This would need to be addressed if we
are creating an alterative way to discover whether the PHY supports
inband mode or not.

So I haven't studied the SFP code path too deeply, but I think part of
the issue is the order in which things are done. It's almost as if there
should be a validation phase for PHY inband abilities too.

phylink_sfp_connect_phy
-> phylink_sfp_config:
   -> first this checks if phylink_phy_no_inband
   -> then this changes pl->link_config.interface and pl->cur_link_an_mode
-> phylink_bringup_phy:
   -> this is where I'm adding the new phy_config_inband_aneg function

If we were to use only my phy_config_inband_aneg function, it would need
to be moved upwards in the code path, to be precise where phylink_phy_no_inband
currently is. Then we'd have to try MLO_AN_INBAND first, with a fallback
to MLO_AN_PHY if that fails. I think this would unnecessarily complicate
the code.

Alternatively, I could create a second PHY driver method, simply for
validation of supported inband modes. The validation can be done in
place of the current phylink_phy_noinband(), and I can still have the
phy_config_inband_aneg() where I put it now, since we shouldn't have any
surprises w.r.t. supported operating mode, and there should be no reason
to repeat the attempt as there would be with a single PHY driver method.
Thoughts?

Also, there needs to be consideration of PHYs that dynamically change
their interface type, and whether they support inband signalling.
For example, a PHY may support a mode where it dynamically selects
between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII
mode may have inband signalling enabled or disabled. This is not a
theoretical case; we have a PHY like that supported in the kernel and
boards use it. What would the semantics of your new call be for a PHY
that performs this?

Should we also have a phydev->inband tristate, taking values "unknown,
enabled, disabled" which the PHY driver is required to update in their
read_status callback if they dynamically change their interface type?
(Although then phylink will need to figure out how to deal with that.)

I don't have such PHY to test with, but I think the easiest way would be
to just call the validation method again, after we change the
phydev->interface value. The PHY driver can easily take phydev->interface
into consideration when answering the question "is inband aneg supported
or not". I don't think that making phydev->inband a stateful value is
going to be as useful as making it a function, since as you say, we will
be required to keep it up to date from generic PHY driver methods, but
only phylink will use it.