Re: [PATCH v4 net-next 5/8] net: phylink: explicitly configure in-band autoneg for on-board PHYs

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Nov 18, 2022 at 10:09:35AM +0000, Russell King (Oracle) wrote:
> On Fri, Nov 18, 2022 at 02:01:21AM +0200, Vladimir Oltean wrote:
> > +	if (pl->config->sync_an_inband && !phy_on_sfp(phy)) {
> 
> Hmm, this phy_on_sfp() is new to me, and looking at the git history, I
> really don't think this does what it claims to do. This returns the
> status of phydev->is_on_sfp_module, which is set by this code:
> 
>         phydev->phy_link_change = phy_link_change;
>         if (dev) {
>                 phydev->attached_dev = dev;
>                 dev->phydev = phydev;
> 
>                 if (phydev->sfp_bus_attached)
>                         dev->sfp_bus = phydev->sfp_bus;
>                 else if (dev->sfp_bus)
>                         phydev->is_on_sfp_module = true;
>         }
> 
> ... which is very wrong. "dev" here is the net_device, and a net_device
> will have its sfp_bus member set when there is a SFP cage present,
> which may be behind a off-SFP PHY.
> 
> This means that when a PHY is attached by the network driver in their
> ndo_open, if there is a SFP bus on the interface (such as on the
> Macchiatobin board), the above will set is_on_sfp_module true for the
> on-board PHY even though it is not in the SFP module.
> 
> Essentially, commit b834489bcecc is incorrect, and needs to be fixed
> before use is made of phy_on_sfp() outside of the broadcom driver.

IIUC, you're saying that if there is an SFP cage after an on-board PHY
X (presumably set using phy_sfp_attach()), then PHY X will be declared
as having phydev->is_on_sfp_module = true despite being on-board?

I don't have such a setup to experiment with. Looking at armada-8040-mcbin.dts,
it's these PHYs, right?

&cp0_xmdio {
	status = "okay";

	phy0: ethernet-phy@0 {
		compatible = "ethernet-phy-ieee802.3-c45";
		reg = <0>;
		sfp = <&sfp_eth0>;
	};

	phy8: ethernet-phy@8 {
		compatible = "ethernet-phy-ieee802.3-c45";
		reg = <8>;
		sfp = <&sfp_eth1>;
	};
};

&cp0_eth0 {
	status = "okay";
	/* Network PHY */
	phy = <&phy0>;
	phy-mode = "10gbase-r";
};

&cp1_eth0 {
	status = "okay";
	/* Network PHY */
	phy = <&phy8>;
	phy-mode = "10gbase-r";
};

But in this case, I believe that phy_sfp_attach() will set
phydev->sfp_bus_attached = true, and this will make the code go through
the first "if" branch and not through the "else" (IOW, the code excludes
the on-board PHYs from the logic)? Or are you describing some
timing/ordering issue which makes this not be the case (something like
the sfp_upstream_ops :: attach() of the on-board PHY being called later
than the phy_attach_direct())?

Could you help me better understand why the code will not enter through
the "if" in this case but will enter through the "else"?