[RFC,5/7] net: phy: make phy_stop synchronous

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.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 48 ++++++++++++++++++++++++++++++++----------------
 include/linux/phy.h   | 12 +++++++++++-
 2 files changed, 43 insertions(+), 17 deletions(-)

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 <hkallweit1@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.