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: <20190613093437.p4c6xiolrwzikmhq@shell.armlinux.org.uk>
Date:   Thu, 13 Jun 2019 10:34:37 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Ioana Ciornei <ioana.ciornei@....com>
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

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 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.

> 
> --
> Ioana
> 
> 
> > 
> > >
> > > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > > Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> > > ---
> > >  drivers/net/phy/phylink.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 5d0af041b8f9..dd1feb7b5472 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -688,6 +688,7 @@ static void phylink_phy_change(struct phy_device
> > *phydev, bool up,
> > >  		pl->phy_state.pause |= MLO_PAUSE_ASYM;
> > >  	pl->phy_state.interface = phydev->interface;
> > >  	pl->phy_state.link = up;
> > > +	pl->phy_state.an_enabled = phydev->autoneg;
> > >  	mutex_unlock(&pl->state_mutex);
> > >
> > >  	phylink_run_resolve(pl);
> > > --
> > > 1.9.1
> > >
> > >
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up