| Message ID | 024c1ea8-6547-b27d-948c-63e32b3229f1@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: phy: improve stopping PHY | expand |
On Wed, Jan 16, 2019 at 08:20:43PM +0100, Heiner Kallweit wrote: > phy_stop_interrupts() is called from phy_disconnect() only. Most of > what it does has been done by phy_stop() already which should have > been called before phy_disconnect(). Based on that we can do some > improvements: > - remove phy_stop_interrupts() and free interrupt in > phy_disconnect() directly > - replace condition "phydev->irq > 0" with the appropriate helper > - make sure phy state machine is stopped after calling phy_stop() > - check in phy_disconnect() that PHY is in a stopped state. Else > warn to detect misbehaving drivers and call phy_stop(). Hi Heiner When i see a list like this, it makes me think there should be a patchset, not a single patch. If something does break, we can bisect it to just one change. Please could you try to break this up into a few patches. Thanks Andrew
On 16.01.2019 20:40, Andrew Lunn wrote: > On Wed, Jan 16, 2019 at 08:20:43PM +0100, Heiner Kallweit wrote: >> phy_stop_interrupts() is called from phy_disconnect() only. Most of >> what it does has been done by phy_stop() already which should have >> been called before phy_disconnect(). Based on that we can do some >> improvements: >> - remove phy_stop_interrupts() and free interrupt in >> phy_disconnect() directly >> - replace condition "phydev->irq > 0" with the appropriate helper >> - make sure phy state machine is stopped after calling phy_stop() >> - check in phy_disconnect() that PHY is in a stopped state. Else >> warn to detect misbehaving drivers and call phy_stop(). > > Hi Heiner > > When i see a list like this, it makes me think there should be a > patchset, not a single patch. If something does break, we can bisect > it to just one change. > > Please could you try to break this up into a few patches. > I thought the changes are simple enough to combine them and avoid the overhead of a patchset. But I'll see to break this up, considering that not all changes are fully independent. > Thanks > Andrew > Heiner
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 2ffe08537..745a705a5 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -813,23 +813,6 @@ int phy_start_interrupts(struct phy_device *phydev) } EXPORT_SYMBOL(phy_start_interrupts); -/** - * phy_stop_interrupts - disable interrupts from a PHY device - * @phydev: target phy_device struct - */ -int phy_stop_interrupts(struct phy_device *phydev) -{ - int err = phy_disable_interrupts(phydev); - - if (err) - phy_error(phydev); - - free_irq(phydev->irq, phydev); - - return err; -} -EXPORT_SYMBOL(phy_stop_interrupts); - /** * phy_stop - Bring down the PHY link, and stop checking the status * @phydev: target phy_device struct @@ -853,6 +836,7 @@ void phy_stop(struct phy_device *phydev) mutex_unlock(&phydev->lock); phy_state_machine(&phydev->state_queue.work); + phy_stop_machine(phydev); /* Cannot call flush_scheduled_work() here as desired because * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index b94095d74..4bd6c9787 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -981,10 +981,13 @@ EXPORT_SYMBOL(phy_connect); */ void phy_disconnect(struct phy_device *phydev) { - if (phydev->irq > 0) - phy_stop_interrupts(phydev); + if (phy_is_started(phydev)) { + phydev_warn(phydev, "phy_stop should have been called before phy_disconnect!\n"); + phy_stop(phydev); + } - phy_stop_machine(phydev); + if (phy_interrupt_is_valid(phydev)) + free_irq(phydev->irq, phydev); phydev->adjust_link = NULL; diff --git a/include/linux/phy.h b/include/linux/phy.h index 00b0533de..1fa7c367b 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -951,7 +951,6 @@ int phy_aneg_done(struct phy_device *phydev); int phy_speed_down(struct phy_device *phydev, bool sync); int phy_speed_up(struct phy_device *phydev); -int phy_stop_interrupts(struct phy_device *phydev); int phy_restart_aneg(struct phy_device *phydev); int phy_reset_after_clk_enable(struct phy_device *phydev);
phy_stop_interrupts() is called from phy_disconnect() only. Most of what it does has been done by phy_stop() already which should have been called before phy_disconnect(). Based on that we can do some improvements: - remove phy_stop_interrupts() and free interrupt in phy_disconnect() directly - replace condition "phydev->irq > 0" with the appropriate helper - make sure phy state machine is stopped after calling phy_stop() - check in phy_disconnect() that PHY is in a stopped state. Else warn to detect misbehaving drivers and call phy_stop(). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy.c | 18 +----------------- drivers/net/phy/phy_device.c | 9 ++++++--- include/linux/phy.h | 1 - 3 files changed, 7 insertions(+), 21 deletions(-)