From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:39693 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751456AbeCOAAv (ORCPT ); Wed, 14 Mar 2018 20:00:51 -0400 Received: by mail-wm0-f67.google.com with SMTP id u10so7067021wmu.4 for ; Wed, 14 Mar 2018 17:00:51 -0700 (PDT) Subject: Re: [PATCH RFC 5/7] net: phy: make phy_stop synchronous To: Heiner Kallweit , Andrew Lunn Cc: Geert Uytterhoeven , "netdev@vger.kernel.org" References: <421476ed-03c7-63a0-44a4-c6d9c7702647@gmail.com> <1ce9431b-a109-e7ac-f974-aeda8155e40e@gmail.com> From: Florian Fainelli Message-ID: <4e5ca4c8-ab7c-9eef-15b5-edef39082777@gmail.com> Date: Wed, 14 Mar 2018 17:00:40 -0700 MIME-Version: 1.0 In-Reply-To: <1ce9431b-a109-e7ac-f974-aeda8155e40e@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: netdev-owner@vger.kernel.org List-ID: 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 > --- > 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