| Message ID | cc566aa7-27cd-67fb-ea4b-00ee84d3dd7b@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: improve starting PHY | expand |
On Sun, Jan 20, 2019 at 10:02:13AM +0100, Heiner Kallweit wrote: > phy_start() should be called from states PHY_READY or PHY_HALTED only. > Check for this to detect misbehaving drivers. Also the state machine > should be started only when being called from one of the valid states. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/phy/phy.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 3df6aadc5..fd928979b 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -861,9 +861,16 @@ void phy_start(struct phy_device *phydev) > > mutex_lock(&phydev->lock); > > + if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) { > + WARN(1, "called from state %s\n", > + phy_state_to_str(phydev->state)); > + goto out; > + } Hi Heiner Warning is good. But jumping to out i'm not so sure about. Drivers which are 'broken' work well enough that users don't know they are broken. But jumping to out is going to really break them. It seems better to have the kernel only warn for one cycle so we find out about such drivers and fix them, and later add the goto out. Andrew
On 21.01.2019 17:40, Andrew Lunn wrote: > On Sun, Jan 20, 2019 at 10:02:13AM +0100, Heiner Kallweit wrote: >> phy_start() should be called from states PHY_READY or PHY_HALTED only. >> Check for this to detect misbehaving drivers. Also the state machine >> should be started only when being called from one of the valid states. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/phy/phy.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index 3df6aadc5..fd928979b 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -861,9 +861,16 @@ void phy_start(struct phy_device *phydev) >> >> mutex_lock(&phydev->lock); >> >> + if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) { >> + WARN(1, "called from state %s\n", >> + phy_state_to_str(phydev->state)); >> + goto out; >> + } > > Hi Heiner > > Warning is good. But jumping to out i'm not so sure about. Drivers > which are 'broken' work well enough that users don't know they are > broken. But jumping to out is going to really break them. It seems > better to have the kernel only warn for one cycle so we find out about > such drivers and fix them, and later add the goto out. > For all invalid states phy_start() basically was a no-op. All it did was triggering a state machine run, but for all "running" states the poll loop was active anyway. And if called from PHY_DOWN, the state machine does nothing. Therefore I see no scenario where jumping to out would break anything. > Andrew > Heiner
> For all invalid states phy_start() basically was a no-op. All it did was > triggering a state machine run, but for all "running" states the poll > loop was active anyway. And if called from PHY_DOWN, the state machine > does nothing. Therefore I see no scenario where jumping to out would > break anything. Hi Heiner It is useful to put this sort of analysis in the commit message. You then won't get people like me asking about it. Andrew
On 21.01.2019 19:29, Andrew Lunn wrote: >> For all invalid states phy_start() basically was a no-op. All it did was >> triggering a state machine run, but for all "running" states the poll >> loop was active anyway. And if called from PHY_DOWN, the state machine >> does nothing. Therefore I see no scenario where jumping to out would >> break anything. > > Hi Heiner > > It is useful to put this sort of analysis in the commit message. You > then won't get people like me asking about it. > Will add this to my "best practices" list ;) > Andrew > Heiner
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 3df6aadc5..fd928979b 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -861,9 +861,16 @@ void phy_start(struct phy_device *phydev) mutex_lock(&phydev->lock); + if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) { + WARN(1, "called from state %s\n", + phy_state_to_str(phydev->state)); + goto out; + } + switch (phydev->state) { case PHY_READY: phydev->state = PHY_UP; + phy_start_machine(phydev); break; case PHY_HALTED: /* if phy was suspended, bring the physical link up again */ @@ -877,13 +884,13 @@ void phy_start(struct phy_device *phydev) } phydev->state = PHY_RESUMING; + phy_start_machine(phydev); break; default: break; } +out: mutex_unlock(&phydev->lock); - - phy_start_machine(phydev); } EXPORT_SYMBOL(phy_start);
phy_start() should be called from states PHY_READY or PHY_HALTED only. Check for this to detect misbehaving drivers. Also the state machine should be started only when being called from one of the valid states. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)