On 30.11.2018 19:45, Anssi Hannula wrote: > When a PHY_HALTED phydev is resumed by phy_start(), it is set to > PHY_RESUMING to wait for the link to come up. > > However, if the phydev was put to PHY_HALTED (by e.g. phy_stop()) before > autonegotiation was ever started by phy_state_machine(), autonegotiation > remains unconfigured, i.e. phy_config_aneg() is never called. > phy_stop() should only be called once the PHY was started. But it's right that we don't have full control over it because misbehaving drivers may call phy_stop() even if the PHY hasn't been started yet.
I think phy_error() and phy_stop() should be extended to set the state to PHY_HALTED only if the PHY is in a started state, means: don't touch the state if it's DOWN, READY, or HALTED. In case of phy_error() it's more a precaution, because it's used within phylib only and AFAICS it can be triggered from a started state only. So yes, there is a theoretical issue. But as you wrote already, I think there's a better way to deal with it. For checking whether PHY is in a started state you could use the helper which is being discussed here: https://marc.info/?l=linux-netdev&m=154360368104946&w=2 > Fix that by going to PHY_UP instead of PHY_RESUMING if autonegotiation > has never been configured. > > Signed-off-by: Anssi Hannula <anssi.hann...@bitwise.fi> > --- > > This doesn't feel as clean is I'd like to, though. Maybe there is a > better way? > > > drivers/net/phy/phy.c | 11 ++++++++++- > include/linux/phy.h | 2 ++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index d46858694db9..7a650cce7599 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -553,6 +553,8 @@ int phy_start_aneg(struct phy_device *phydev) > if (err < 0) > goto out_unlock; > > + phydev->autoneg_configured = 1; > + > if (phydev->state != PHY_HALTED) { > if (AUTONEG_ENABLE == phydev->autoneg) { > err = phy_check_link_status(phydev); > @@ -893,7 +895,14 @@ void phy_start(struct phy_device *phydev) > break; > } > > - phydev->state = PHY_RESUMING; > + /* if autoneg/forcing was never configured, go back to PHY_UP > + * to make that happen > + */ > + if (!phydev->autoneg_configured) > + phydev->state = PHY_UP; > + else > + phydev->state = PHY_RESUMING; > + > break; > default: > break; > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 8f927246acdb..b306d5ed9d09 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -389,6 +389,8 @@ struct phy_device { > unsigned loopback_enabled:1; > > unsigned autoneg:1; > + /* autoneg has been configured at least once */ > + unsigned autoneg_configured:1; > /* The most recently read link state */ > unsigned link:1; > >
