Re: [PATCH v4.4] net: phy: Add driver for Motorcomm yt8521 gigabit ethernet phy

[Andrew Lunn]

On Mon, Aug 22, 2022 at 08:21:47PM -0700, Jakub Kicinski wrote:
> On Wed, 17 Aug 2022 19:25:54 +0800 Frank wrote:
> > +static int yt8521_fiber_config_aneg(struct phy_device *phydev)
> > +{
> > +	int err, changed;
> > +	u16 adv;
> > +
> > +	if (phydev->autoneg != AUTONEG_ENABLE)
> > +		return yt8521_fiber_setup_forced(phydev);
> > +
> > +	err =  ytphy_modify_ext_with_lock(phydev, YTPHY_MISC_CONFIG_REG,
> > +					  YTPHY_MCR_FIBER_SPEED_MASK,
> > +					  YTPHY_MCR_FIBER_1000BX);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* enable Fiber auto sensing */
> > +	err =  ytphy_modify_ext_with_lock(phydev, YT8521_LINK_TIMER_CFG2_REG,
> > +					  0, YT8521_LTCR_EN_AUTOSEN);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* Setup fiber advertisement */
> > +	adv = ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
> > +	      ADVERTISE_1000XPSE_ASYM;
> 
> Is it okay to ignore phydev->advertising and always set the same mask?

The user could of changed the pause settings, which are going to be
ignored here. Also, you should not assume the MAC can actually do
asymmetric pause, not all can. phydev->advertising will be set to only
include what the MAC can actually do.

The whole concept of having two line sides connected to one MAC and
seeing which gets link first is unsupported in Linux. In theory, you
want to be able to configure each line side differently. Maybe you
want autoneg on copper, but fixed on fibre, asymmetric pause with
fibre, but symmetric pause on copper, etc. Since there is only one
instance of phydev here, you don't have anywhere to store two sets of
configuration, nor any sort of kAPI to deal with two phydev structures
etc. So the user experience is not so great.

With the Marvell Switches which also have this capability, i actually
ignore it, use the phy-mode it decide which should be used, either
copper or fibre, and leave the other powered off so it can never get
link. There is at least one Marvell PHY which does however support
first up wins, so this behaviour is not new. I just don't recommend
it.

And it gets even more interesting when the SFP is actually copper. But
since the integration with phylink is missing in this driver, that is
not supported here.

	Andrew