netdev - Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

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]

Message-ID: <20200812181333.69191baf@dellmb.labs.office.nic.cz>
Date:   Wed, 12 Aug 2020 18:13:33 +0200
From:   Marek Behún <marek.behun@....cz>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>,
        Baruch Siach <baruch@...s.co.il>,
        Chris Healy <cphealy@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change
 MACTYPE according to phydev->interface

On Wed, 12 Aug 2020 16:48:37 +0100
Russell King - ARM Linux admin <linux@...linux.org.uk> wrote:

> On Wed, Aug 12, 2020 at 05:37:16PM +0200, Marek Behún wrote:
> > On Wed, 12 Aug 2020 16:00:54 +0100
> > Russell King - ARM Linux admin <linux@...linux.org.uk> wrote:
> >   
> > > On Wed, Aug 12, 2020 at 04:44:31PM +0200, Marek Behún wrote:  
> > > > There is another problem though: I think the PHY driver, when
> > > > deciding whether to set MACTYPE from the XFI with rate matching
> > > > mode to the 10GBASE-R/5GBASE-R/2500BASE-X/SGMII with AN mode,
> > > > should check which modes the underlying MAC support.    
> > > 
> > > I'm aware of that problem.  I have some experimental patches
> > > which add PHY interface mode bitmaps to the MAC, PHY, and SFP
> > > module parsing functions.  I have stumbled on some problems
> > > though - it's going to be another API change (and people are
> > > already whinging about the phylink API changing "too quickly",
> > > were too quickly seems to be defined as once in three years), and
> > > in some cases, DSA, it's extremely hard to work out how to
> > > properly set such a bitmap due to DSA's layered approach.
> > >   
> > 
> > If by your experimental patches you mean
> >   net: mvneta: fill in phy interface mode bitmap
> >   net: mvpp2: fill in phy interface mode bitmap
> > found here
> >   http://git.arm.linux.org.uk/cgit/linux-arm.git/log/?h=clearfog
> > I am currently working on top of them.
> >   
> > > Having bitmaps means that we can take the union of what the MAC
> > > and PHY supports, and decide which MACTYPE setting would be most
> > > suitable. However, to do that we're into also changing phylib's
> > > interfaces as well.
> > >   
> > > > driver to phylink in the call to phylink_create. But there is
> > > > no way for the PHY driver to get this information from phylink
> > > > currently, and even if phylink exposed a function to return the
> > > > config member of struct phylink, the problem is that at the
> > > > time when mv3310_power_up is called, the phydev->phylink is not
> > > > yet set (this is done in phylink_bringup_phy, and
> > > > mv3310_power_up is called sometime in the phylink_attach_phy).
> > > >   
> > > 
> > > We _really_ do not want phylib calling back into phylink
> > > functions. That would tie phylink functionality into phylib and
> > > cause problems when phylink is not being used.
> > > 
> > > I would prefer phylib to be passed "the MAC can use these
> > > interface types, and would prefer to use this interface type" and
> > > have the phylib layer (along with the phylib driver) make the
> > > decision about which mode should be used.  That also means that
> > > non-phylink MACs can also use it.
> > >   
> > 
> > I may try to propose something, but in the meantime do you think the
> > current version of the patch
> >   net: phy: marvell10g: change MACTYPE according to
> > phydev->interface is acceptable?  
> 
> Well, I have other questions about it.  Why are you doing it in
> the power_up function?  Do you find that the MACTYPE field is
> lost when clearing the power down bit?  From what I read, it should
> only change on hardware reset, and we don't hardware reset when we
> come out of power down - only software reset.
> 

The MACTYPE is not being lost. But changing it requires Port Software
Reset, which resets the link, so it cannot be done for example in
read_status.
I think the MACTYPE should be set sometime during PHY initialisation,
and only once: either to XFI with rate matching, if the underlying MAC
does not support lower modes, or to 10gbase-r/2500base-x/sgmii mode, if
the underlying MAC supports only slower modes than 10G.

Marek