netdev - Re: [PATCH RFC 5/7] net: phy: make phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 14 Mar 2018 17:00:40 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     Geert Uytterhoeven <geert+renesas@...der.be>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 5/7] net: phy: make phy_stop synchronous

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