netdev - RE: [PATCH] net: phylink: set the autoneg state in phylink_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]

Message-ID: <VI1PR0402MB28008D841D50843C0212FBFBE0EF0@VI1PR0402MB2800.eurprd04.prod.outlook.com>
Date:   Thu, 13 Jun 2019 14:56:01 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] net: phylink: set the autoneg state in phylink_phy_change

> Subject: Re: [PATCH] net: phylink: set the autoneg state in
> phylink_phy_change
> 
> On Thu, Jun 13, 2019 at 02:32:06PM +0000, Ioana Ciornei wrote:
> >
> > > Subject: Re: [PATCH] net: phylink: set the autoneg state in
> > > phylink_phy_change
> > >
> > > On Thu, Jun 13, 2019 at 08:55:16AM +0000, Ioana Ciornei wrote:
> > > > > Subject: Re: [PATCH] net: phylink: set the autoneg state in
> > > > > phylink_phy_change
> > > > >
> > > > > On Thu, Jun 13, 2019 at 09:37:51AM +0300, Ioana Ciornei wrote:
> > > > > > The phy_state field of phylink should carry only valid
> > > > > > information especially when this can be passed to the .mac_config
> callback.
> > > > > > Update the an_enabled field with the autoneg state in the
> > > > > > phylink_phy_change function.
> > > > >
> > > > > an_enabled is meaningless to mac_config for PHY mode.  Why do
> > > > > you think this is necessary?
> > > >
> > > > Well, it's not necessarily used in PHY mode but, from my opinion,
> > > > it should
> > > be set to the correct value nonetheless.
> > > >
> > > > Just to give you more context, I am working on adding phylink
> > > > support on
> > > NXP's DPAA2 platforms where any interaction between the PHY
> > > management layer and the Ethernet devices is made through a firmware.
> > > > When the .mac_config callback is invoked, the driver communicates
> > > > the
> > > new configuration to the firmware so that the corresponding
> > > net_device can see the correct info.
> > > > In this case, the an_enabled field is not used for other purpose
> > > > than to
> > > inform the net_device of the current configuration and nothing more.
> > >
> > > The fields that are applicable depend on the negotiation mode:
> > >
> > > - Non-inband (PHY or FIXED): set the speed, duplex and pause h/w
> > >    parameters as per the state's speed, duplex and pause settings.
> > >    Every other state setting should be ignored; they are not defined
> > >    for this mode of operation.
> > >
> > > - Inband SGMII: set for inband SGMII reporting of speed and duplex
> > >    h/w parameters.  Set pause mode h/w parameters as per the state's
> > >    pause settings.  Every other state setting should be ignored; they
> > >    are not defined for this mode of operation.
> > >
> > > - Inband 802.3z: set for 1G or 2.5G depending on the PHY interface mode.
> > >    If an_enabled is true, allow inband 802.3z to set the duplex h/w
> > >    parameter.  If an_enabled and the MLO_PAUSE_AN bit of the pause
> > >    setting are true, allow 802.3z to set the pause h/w parameter.
> > >    Advertise capabilities depending on the 'advertising' setting.
> > >
> > > There's only one case where an_enabled is used, which is 802.3z
> > > negotiation, because the MAC side is responsible for negotiating the
> > > link mode.  In all other cases, the MAC is not responsible for any
> autonegotiation.
> >
> > It's clear for me that an_enabled is of use for the MAC only when clause 37
> auto-negotiation is used.
> >
> > However,  the DPAA2 software architecture abstracts the MAC and the
> network interface into 2 separate entities that are managed by two different
> drivers.
> > These drivers communicate only through a firmware.
> >
> > This means that any ethtool issued on a DPAA2 network interface will go
> directly to the firmware for the latest link information.
> 
> So you won't be calling phylink_ethtool_ksettings_get(), which means you
> won't be returning correct information anyway.
> 
> > When the MAC driver is not capable to inform the firmware of the proper
> link configuration (eg whether the autoneg is on or not), the ethtool output
> will not be the correct one.
> 
> You don't get to know the list of supported link modes, so I don't see how
> the ethtool information can be correct.
> 
> I'd like to see the patches _before_ I consider accepting your proposed
> phylink change.

Sure. I'll send an RFC as soon as possible.

> 
> > > It is important to stick to the above, which will ensure correct
> > > functioning of your driver - going off and doing your own thing
> > > (such as reading from other
> > > fields) is not guaranteed to give good results.
> >
> > You're right, but unfortunately I am not dealing with a straight-forward
> architecture.
> 
> At this point, I think you need to explain why you want to use phylink, as you
> seem to be saying that your driver is unable to conform to what phylink
> expects due to firmware.

That's going to be in the cover letter.

--
Ioana