On 03/14/2018 01:16 PM, Heiner Kallweit wrote:
> Currently phy_stop() just sets the state to PHY_HALTED and relies on the
> state machine to do the remaining work. It can take up to 1s until the
> state machine runs again what causes issues in situations where e.g.
> driver / device is brought down directly after executing phy_stop().
>
> Fix this by executing all phy_stop() activities synchronously.
>
> Add a function phy_stop_suspending() which does basically the same as
> phy_stop() and just adopts the state adjustment logic from
> phy_stop_machine() to inform the resume callback about the status of
> the PHY before suspending.
Definitively an improvement, thanks!
>
> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
> ---
> drivers/net/phy/phy.c | 48 ++++++++++++++++++++++++++++++++----------------
> include/linux/phy.h | 12 +++++++++++-
> 2 files changed, 43 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 0ca1672a5..54144cd10 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -737,21 +737,49 @@ int phy_stop_interrupts(struct phy_device *phydev)
> }
> EXPORT_SYMBOL(phy_stop_interrupts);
>
> +static void phy_link_up(struct phy_device *phydev)
> +{
> + phydev->phy_link_change(phydev, true, true);
> + phy_led_trigger_change_speed(phydev);
> +}
> +
> +static void phy_link_down(struct phy_device *phydev, bool do_carrier)
> +{
> + phydev->phy_link_change(phydev, false, do_carrier);
> + phy_led_trigger_change_speed(phydev);
> +}
> +
> /**
> - * phy_stop - Bring down the PHY link, and stop checking the status
> + * __phy_stop - Bring down the PHY link, and stop checking the status
> * @phydev: target phy_device struct
> + * @suspending: called from a suspend callback
Can you put the same comment as what phy_stop_machine() has such that we
understand why there is a check for phydev->state > PHY_UP and what
happens when suspend is set to false and explain what happens when
@suspending is set to false.
> */
> -void phy_stop(struct phy_device *phydev)
> +void __phy_stop(struct phy_device *phydev, bool suspending)
> {
> mutex_lock(&phydev->lock);
>
> if (PHY_HALTED == phydev->state)
> goto out_unlock;
>
> + /* stop state machine */
> + cancel_delayed_work_sync(&phydev->state_queue);
> +
> if (phy_interrupt_is_valid(phydev))
> phy_disable_interrupts(phydev);
>
> - phydev->state = PHY_HALTED;
> + if (phydev->link) {
> + phydev->link = 0;
> + phy_link_down(phydev, true);
> + }
> +
> + phy_suspend(phydev);
> +
> + if (suspending) {
> + if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
> + phydev->state = PHY_UP;
> + } else {
> + phydev->state = PHY_HALTED;
> + }
>
> out_unlock:
> mutex_unlock(&phydev->lock);
> @@ -761,7 +789,7 @@ void phy_stop(struct phy_device *phydev)
> * will not reenable interrupts.
> */
> }
> -EXPORT_SYMBOL(phy_stop);
> +EXPORT_SYMBOL(__phy_stop);
>
> /**
> * phy_start - start or restart a PHY device
> @@ -804,18 +832,6 @@ void phy_start(struct phy_device *phydev)
> }
> EXPORT_SYMBOL(phy_start);
>
> -static void phy_link_up(struct phy_device *phydev)
> -{
> - phydev->phy_link_change(phydev, true, true);
> - phy_led_trigger_change_speed(phydev);
> -}
> -
> -static void phy_link_down(struct phy_device *phydev, bool do_carrier)
> -{
> - phydev->phy_link_change(phydev, false, do_carrier);
> - phy_led_trigger_change_speed(phydev);
> -}
> -
> /**
> * phy_state_machine - Handle the state machine
> * @work: work_struct that describes the work to be done
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index bc7aa93c6..be43bd270 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -940,7 +940,7 @@ struct phy_device *phy_connect(struct net_device *dev,
> const char *bus_id,
> void phy_disconnect(struct phy_device *phydev);
> void phy_detach(struct phy_device *phydev);
> void phy_start(struct phy_device *phydev);
> -void phy_stop(struct phy_device *phydev);
> +void __phy_stop(struct phy_device *phydev, bool suspending);
> int phy_start_aneg(struct phy_device *phydev);
> int phy_aneg_done(struct phy_device *phydev);
>
> @@ -964,6 +964,16 @@ static inline const char *phydev_name(const struct
> phy_device *phydev)
> return dev_name(&phydev->mdio.dev);
> }
>
> +static inline void phy_stop(struct phy_device *phydev)
> +{
> + __phy_stop(phydev, false);
> +}
> +
> +static inline void phy_stop_suspending(struct phy_device *phydev)
> +{
> + __phy_stop(phydev, true);
> +}
I am not a huge fond of these inline functions, I would just move thos
down to phy.c and actually export both of these symbols, this is just
personal preference though.
--
Florian
