[PATCH net-next] net: phy: initialise phydev speed and duplex sanely

[PATCH net-next] net: phy: initialise phydev speed and duplex sanely

[Russell King]

When a phydev is created, the speed and duplex are set to zero and
-1 respectively, rather than using the predefined SPEED_UNKNOWN and
DUPLEX_UNKNOWN constants.

There is a window at initialisation time where we may report link
down using the 0/-1 values.  Tidy this up and use the predefined
constants, so debug doesn't complain with:

"Unsupported (update phy-core.c)/Unsupported (update phy-core.c)"

when the speed and duplex settings are printed.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 232ad33b1159..8186aad4ef90 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -597,8 +597,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
 	mdiodev->device_free = phy_mdio_device_free;
 	mdiodev->device_remove = phy_mdio_device_remove;
 
-	dev->speed = 0;
-	dev->duplex = -1;
+	dev->speed = SPEED_UNKNOWN;
+	dev->duplex = DUPLEX_UNKNOWN;
 	dev->pause = 0;
 	dev->asym_pause = 0;
 	dev->link = 0;
-- 
2.20.1

Fwd: Re: [PATCH net-next] net: phy: initialise phydev speed and duplex sanely

[Russell King - ARM Linux admin]

Unfortunately, Andrew dropped the 'g' from the netdev email address,
and Zach's email address doesn't seem to work.

Forwarding this to netdev (with appropriate threading) for archival
purposes.

----- Forwarded message from Russell King - ARM Linux admin <linux@armlinux.org.uk> -----

Date: Fri, 22 Nov 2019 16:03:23 +0000
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Andrew Lunn <andrew@lunn.ch>
Cc: zach.brown@ni.com, Florian Fainelli <f.fainelli@gmail.com>, Heiner Kallweit
	<hkallweit1@gmail.com>, "David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.or
Subject: Re: [PATCH net-next] net: phy: initialise phydev speed and duplex
	sanely

On Fri, Nov 22, 2019 at 04:02:01PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Nov 22, 2019 at 04:51:24PM +0100, Andrew Lunn wrote:
> > On Fri, Nov 22, 2019 at 03:23:23PM +0000, Russell King wrote:
> > > When a phydev is created, the speed and duplex are set to zero and
> > > -1 respectively, rather than using the predefined SPEED_UNKNOWN and
> > > DUPLEX_UNKNOWN constants.
> > > 
> > > There is a window at initialisation time where we may report link
> > > down using the 0/-1 values.  Tidy this up and use the predefined
> > > constants, so debug doesn't complain with:
> > > 
> > > "Unsupported (update phy-core.c)/Unsupported (update phy-core.c)"
> > > 
> > > when the speed and duplex settings are printed.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  drivers/net/phy/phy_device.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index 232ad33b1159..8186aad4ef90 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -597,8 +597,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
> > >  	mdiodev->device_free = phy_mdio_device_free;
> > >  	mdiodev->device_remove = phy_mdio_device_remove;
> > >  
> > > -	dev->speed = 0;
> > > -	dev->duplex = -1;
> > > +	dev->speed = SPEED_UNKNOWN;
> > > +	dev->duplex = DUPLEX_UNKNOWN;
> > >  	dev->pause = 0;
> > >  	dev->asym_pause = 0;
> > >  	dev->link = 0;
> > 
> > Hi Russell, Zach
> > 
> > Does phy_led_trigger_change_speed() need to change? It has
> > 
> >         if (phy->speed == 0)
> >                 return;
> 
> I'm not sure what that's trying to achieve.
> 
> From what I can see, phy_speed_to_led_trigger() looks up the speed in
> the table of triggers, which is populated from the PHYs supported
> speeds, which will never contain zero or SPEED_UNKNOWN.
> 
> Note that genphy will return SPEED_UNKNOWN if autoneg_complete is
> false (see genphy_read_status()).  However, in that case, ->link
> will be false, just like it is at initialisation time, and hence
> we won't reach that statement (we'll go off to
> phy_led_trigger_no_link()).
> 
> So I think the test is entirely unnecessary.

... unless there's a buggy phylib driver, in which case we shouldn't
be working around it in this code (as it would affect network drivers
as well) but should be fixing the broken phylib driver.

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

----- End forwarded message -----

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

Re: [PATCH net-next] net: phy: initialise phydev speed and duplex sanely

[Jakub Kicinski]

On Fri, 22 Nov 2019 15:23:23 +0000, Russell King wrote:
> When a phydev is created, the speed and duplex are set to zero and
> -1 respectively, rather than using the predefined SPEED_UNKNOWN and
> DUPLEX_UNKNOWN constants.
> 
> There is a window at initialisation time where we may report link
> down using the 0/-1 values.  Tidy this up and use the predefined
> constants, so debug doesn't complain with:
> 
> "Unsupported (update phy-core.c)/Unsupported (update phy-core.c)"
> 
> when the speed and duplex settings are printed.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied, thank you!