Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751928AbdG1K5x convert rfc822-to-8bit (ORCPT ); Fri, 28 Jul 2017 06:57:53 -0400 Received: from us-smtp-delivery-107.mimecast.com ([216.205.24.107]:21925 "EHLO us-smtp-delivery-107.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751740AbdG1K5v (ORCPT ); Fri, 28 Jul 2017 06:57:51 -0400 Subject: Re: [PATCH net] net: phy: Run state machine to completion To: Florian Fainelli , "David S. Miller" , netdev CC: Russell King , Zach Brown , Nathan Sullivan , Andrew Lunn , LKML , Mason , Mans Rullgard References: <20170726192446.32045-1-f.fainelli@gmail.com> From: Marc Gonzalez Message-ID: <69ecff09-3822-31b3-99d2-ebd5ca888e7f@sigmadesigns.com> Date: Fri, 28 Jul 2017 12:57:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49.1 MIME-Version: 1.0 In-Reply-To: <20170726192446.32045-1-f.fainelli@gmail.com> X-Originating-IP: [172.27.0.114] X-MC-Unique: 60WicKLVP0i0Y3qG0orqWA-1 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7136 Lines: 137 On 26/07/2017 21:24, Florian Fainelli wrote: > Marc reported that he was not getting the PHY library adjust_link() > callback function to run when calling phy_stop() + phy_disconnect() > which does not indeed happen because we don't make sure we flush the > PHYLIB delayed work and let it run to completion. We also need to have > a synchronous call to phy_state_machine() in order to have the state > machine actually act on PHY_HALTED, set the PHY device's link down, turn > the network device's carrier off and finally call the adjust_link() > function. > > Reported-by: Marc Gonzalez > Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work") > Signed-off-by: Florian Fainelli > --- > David, this dates back from before the commit mentioned in Fixes but it would > be hard to backport to earlier kernels if we flagged the original design flaw > that used timers. Also, I am not clear on the timer API whether there was a way > to ensure timers would run to completion before they would be cancelled. > > Marc, please add your Signed-off-by tag since you contributed the second line. > > Thanks! > > drivers/net/phy/phy.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index d0626bf5c540..30e7c43e0d87 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -743,12 +743,17 @@ void phy_trigger_machine(struct phy_device *phydev, bool sync) > */ > void phy_stop_machine(struct phy_device *phydev) > { > + /* Run the state machine to completion */ > + flush_delayed_work(&phydev->state_queue); > cancel_delayed_work_sync(&phydev->state_queue); > > mutex_lock(&phydev->lock); > if (phydev->state > PHY_UP && phydev->state != PHY_HALTED) > phydev->state = PHY_UP; > mutex_unlock(&phydev->lock); > + > + /* Now we can run the state machine synchronously */ > + phy_state_machine(&phydev->state_queue.work); Hello Florian, Sorry, I was AFK for a couple of days. Thanks for putting the patch together! I added dump_stack() at the top of phy_state_machine() to better understand what's going on. Action: set link down on the command-line Results: 1) PHY in interrupt mode [ 136.238889] [] (phy_state_machine) from [] (phy_stop_machine+0x4c/0x50) [ 136.247285] [] (phy_stop_machine) from [] (phy_disconnect+0x20/0x34) [ 136.255420] [] (phy_disconnect) from [] (nb8800_stop+0x84/0xa0) [ 136.263118] [] (nb8800_stop) from [] (__dev_close_many+0x88/0xd0) [ 136.270989] [] (__dev_close_many) from [] (__dev_close+0x24/0x38) [ 136.278862] [] (__dev_close) from [] (__dev_change_flags+0x94/0x144) [ 136.286997] [] (__dev_change_flags) from [] (dev_change_flags+0x18/0x48) [ 136.295481] [] (dev_change_flags) from [] (devinet_ioctl+0x6e0/0x7a0) [ 136.303702] [] (devinet_ioctl) from [] (inet_ioctl+0x194/0x1c0) [ 136.311400] [] (inet_ioctl) from [] (sock_ioctl+0x148/0x2fc) [ 136.318837] [] (sock_ioctl) from [] (do_vfs_ioctl+0x9c/0x8e8) [ 136.326361] [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c) [ 136.333710] [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x3c) [ 136.341357] nb8800 26000.ethernet eth0: Link is Down Above, phy_state_machine() is called synchronously from phy_stop_machine(). 2) PHY in polling mode [ 54.045752] CPU: 3 PID: 892 Comm: kworker/3:1 Not tainted 4.13.0-rc1 #7 [ 54.056435] Workqueue: events_power_efficient phy_state_machine [ 54.085399] [] (phy_state_machine) from [] (process_one_work+0x1d4/0x3ec) [ 54.093969] [] (process_one_work) from [] (worker_thread+0x268/0x554) [ 54.102189] [] (worker_thread) from [] (kthread+0x108/0x138) [ 54.109623] [] (kthread) from [] (ret_from_fork+0x14/0x3c) [ 54.116918] nb8800 26000.ethernet eth0: Link is Down [ 54.122100] CPU: 2 PID: 973 Comm: ip Not tainted 4.13.0-rc1 #7 [ 54.155001] [] (phy_state_machine) from [] (phy_stop_machine+0x4c/0x50) [ 54.163397] [] (phy_stop_machine) from [] (phy_disconnect+0x20/0x34) [ 54.171532] [] (phy_disconnect) from [] (nb8800_stop+0x84/0xa0) [ 54.179231] [] (nb8800_stop) from [] (__dev_close_many+0x88/0xd0) [ 54.187103] [] (__dev_close_many) from [] (__dev_close+0x24/0x38) [ 54.194975] [] (__dev_close) from [] (__dev_change_flags+0x94/0x144) [ 54.203109] [] (__dev_change_flags) from [] (dev_change_flags+0x18/0x48) [ 54.211594] [] (dev_change_flags) from [] (devinet_ioctl+0x6e0/0x7a0) [ 54.219816] [] (devinet_ioctl) from [] (inet_ioctl+0x194/0x1c0) [ 54.227513] [] (inet_ioctl) from [] (sock_ioctl+0x148/0x2fc) [ 54.234950] [] (sock_ioctl) from [] (do_vfs_ioctl+0x9c/0x8e8) [ 54.242473] [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c) [ 54.249821] [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x3c) Above() phy_state_machine() is called twice - first supposedly after flush_delayed_work() => calls adjust_link() - a second time by the explicit call => probably no-op Therefore, my reasoning is that flush_delayed_work() is unnecessary. The explicit call to phy_state_machine() at the end of phy_stop_machine() will handle all cases. What do you think? Testing without flush_delayed_work() in polling mode: [ 58.819100] CPU: 0 PID: 968 Comm: ip Not tainted 4.13.0-rc1 #10 [ 58.852098] [] (phy_state_machine) from [] (phy_stop_machine+0x44/0x48) [ 58.860494] [] (phy_stop_machine) from [] (phy_disconnect+0x20/0x34) [ 58.868629] [] (phy_disconnect) from [] (nb8800_stop+0x84/0xa0) [ 58.876328] [] (nb8800_stop) from [] (__dev_close_many+0x88/0xd0) [ 58.884198] [] (__dev_close_many) from [] (__dev_close+0x24/0x38) [ 58.892071] [] (__dev_close) from [] (__dev_change_flags+0x94/0x144) [ 58.900206] [] (__dev_change_flags) from [] (dev_change_flags+0x18/0x48) [ 58.908690] [] (dev_change_flags) from [] (devinet_ioctl+0x6e0/0x7a0) [ 58.916911] [] (devinet_ioctl) from [] (inet_ioctl+0x194/0x1c0) [ 58.924608] [] (inet_ioctl) from [] (sock_ioctl+0x148/0x2fc) [ 58.932046] [] (sock_ioctl) from [] (do_vfs_ioctl+0x9c/0x8e8) [ 58.939569] [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c) [ 58.946917] [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x3c) [ 58.954609] nb8800 26000.ethernet eth0: Link is Down Above, the call to adjust_link() occurs from the explicit call to phy_state_machine() Regards.