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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 Aug 2022 11:37:12 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Frank <Frank.Sae@...or-comm.com>, Peter Geis <pgwipeout@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, yinghong.zhang@...or-comm.com,
        fei.zhang@...or-comm.com, hua.sun@...or-comm.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4.4] net: phy: Add driver for Motorcomm yt8521 gigabit
 ethernet phy

On Tue, 23 Aug 2022 16:14:16 +0200 Andrew Lunn wrote:
> 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.

Interesting. Just to confirm - regardless of the two-sided design..
-edness.. IIUC my question has merit and we need v5?