From patchwork Mon Apr 18 22:17:58 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Fainelli X-Patchwork-Id: 611934 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3qpjJX2pLwz9t3c for ; Tue, 19 Apr 2016 08:20:28 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=yetZCPwT; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752224AbcDRWUL (ORCPT ); Mon, 18 Apr 2016 18:20:11 -0400 Received: from mail-pa0-f51.google.com ([209.85.220.51]:33889 "EHLO mail-pa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751542AbcDRWUJ (ORCPT ); Mon, 18 Apr 2016 18:20:09 -0400 Received: by mail-pa0-f51.google.com with SMTP id r5so26483357pag.1; Mon, 18 Apr 2016 15:20:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=/q5FoQOVa4rMj1m+ObL0bM2hLdZXCaJKd6ycGnvM2x8=; b=yetZCPwTAgXcd8C1FCPG1q6aLX1T3jYZfk2nSgDHwNsvXOkkDIViM96GRgnWGhgCdQ panGgWZks2n83hp+dYTs3L4EU6WYcIsySiOKzhA9mdS1vETZHgDmfarE2R6RAs+nZEB5 Lombtkh1sTgNivqHjF4jpuMZ9dg81ZXnOoJkUXkpPlMIU2M2qx9nN06gydr8awPJvX0S JLL0DsL9e8nSc+f8wdHL9J6mcc3G9tfnGbi5ACQXRYlRBZ2YeQvCkvKIZNsj/eLTAicO 1ucT+bIW341WiCvyB0ThDBotdXIpuCkQBMFpAMruJf8hkRU3JTo3ysKmJ7yk5xGbumFF WX9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=/q5FoQOVa4rMj1m+ObL0bM2hLdZXCaJKd6ycGnvM2x8=; b=b8Uz4RZfkRq+HAxWF4izRGbwZ64P+ImIHJD2qmdwdhNlK3F68k3cXlXRwa6gO6bCTU Iz9rTMwVBwYsKg89R7HLyhbMlwwl8eyVWXWRug9AnOKkOV8a511aC40DQhcS7sp2IFgo AgQvgteCuhOlDu/4Hup9pwObQaMn/rtMM9t/khVKAOkdYpQOlnONJYk560QFMpeRSVNt R6serHJLAVPdk5amnLmuyml0gQWoZ2wnzi5X/H11YJVgr1u8gPygVHgv1FyrAq7VlFab 3k5HvPxsz2Pt/+UdOTzbkeSJfJqWTZQTNjGgvwHzxAf/9Qw4j1BORThJJEOhOOQp5Sw5 zDzA== X-Gm-Message-State: AOPr4FVttCy7cKq/CHHT0HS4WpqljCfeZx9EbdW2cHsxX4XeEUUHaKKW1WCVlc1sNWxZbQ== X-Received: by 10.66.249.228 with SMTP id yx4mr52978162pac.29.1461018008366; Mon, 18 Apr 2016 15:20:08 -0700 (PDT) Received: from [10.12.156.244] (5520-maca-inet1-outside.broadcom.com. [216.31.211.11]) by smtp.googlemail.com with ESMTPSA id p26sm85936629pfi.84.2016.04.18.15.20.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 18 Apr 2016 15:20:07 -0700 (PDT) Subject: Re: [PATCH] net: phy: Ensure the state machine is called when phy is UP To: Alexandre Belloni References: <1460750172-7796-1-git-send-email-alexandre.belloni@free-electrons.com> <57114AA4.5080803@gmail.com> <20160415205613.GE25196@piout.net> <20160415220508.GC26665@lunn.ch> <20160415221711.GG25196@piout.net> <571169EB.4090300@gmail.com> <20160418221433.GX25196@piout.net> Cc: Andrew Lunn , "David S . Miller" , Nicolas Ferre , netdev@vger.kernel.org, linux-kernel@vger.kernel.org From: Florian Fainelli Message-ID: <57155D16.2080306@gmail.com> Date: Mon, 18 Apr 2016 15:17:58 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160418221433.GX25196@piout.net> Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 18/04/16 15:14, Alexandre Belloni wrote: > On 15/04/2016 at 15:23:39 -0700, Florian Fainelli wrote : >> On 15/04/16 15:17, Alexandre Belloni wrote: >>> On 16/04/2016 at 00:05:08 +0200, Andrew Lunn wrote : >>>>> Trace without my patch: >>>>> libphy: MACB_mii_bus: probed >>>>> macb f8020000.ethernet eth0: Cadence GEM rev 0x00020120 at 0xf8020000 irq 27 (fc:c2:3d:0c:6e:05) >>>>> Micrel KSZ8081 or KSZ8091 f8020000.etherne:01: attached PHY driver [Micrel KSZ8081 or KSZ8091] (mii_bus:phy_addr=f8020000.etherne:01, irq=171) >>>>> Micrel KSZ8081 or KSZ8091 f8020000.etherne:01: PHY state change READY -> READY >>>>> [...] >>>>> Micrel KSZ8081 or KSZ8091 f8020000.etherne:01: PHY state change READY -> READY >>>> >>>> Are there some state changes before this? How is it getting to state >>>> READY? It would expect it to start in DOWN, from when the phy device >>>> was created in phy_device_create(). >>>> >>> >>> No other changes. I forgot to mention that this is when booting with a >>> cable plugged in. Unplugging and replugging the cable makes the link >>> detection work fine even without the patch. >> >> OK, so the last hunk of the change in d5c3d84657db ("net: phy: Avoid >> polling PHY with PHY_IGNORE_INTERRUPTS"): >> >> - queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, >> - PHY_STATE_TIME * HZ); >> + /* Only re-schedule a PHY state machine change if we are polling the >> + * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving >> + * between states from phy_mac_interrupt() >> + */ >> + if (phydev->irq == PHY_POLL) >> + queue_delayed_work(system_power_efficient_wq, >> &phydev->state_queue, >> + PHY_STATE_TIME * HZ); >> >> >> is presumably what broke for you, right? >> >> Could you also give this patch a spin and see if it works better with >> it? The macb driver does something racy with how the MDIO and PHY are >> probe wrt. registering the netdev, that needs fixing too. >> >> diff --git a/drivers/net/ethernet/cadence/macb.c >> b/drivers/net/ethernet/cadence/macb.c >> index eec3200ade4a..98b99149ce0b 100644 >> --- a/drivers/net/ethernet/cadence/macb.c >> +++ b/drivers/net/ethernet/cadence/macb.c >> @@ -3005,28 +3005,36 @@ static int macb_probe(struct platform_device *pdev) >> if (err) >> goto err_out_free_netdev; >> >> + err = macb_mii_init(bp); >> + if (err) >> + goto err_out_free_netdev; >> + >> + phydev = bp->phy_dev; >> + phy_attached_info(phydev); >> + >> + netif_carrier_off(dev); >> + >> err = register_netdev(dev); >> if (err) { >> dev_err(&pdev->dev, "Cannot register net device, >> aborting.\n"); >> goto err_out_unregister_netdev; >> } >> >> - err = macb_mii_init(bp); >> - if (err) >> - goto err_out_unregister_netdev; >> - >> - netif_carrier_off(dev); >> - >> netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n", >> macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID), >> dev->base_addr, dev->irq, dev->dev_addr); >> >> - phydev = bp->phy_dev; >> - phy_attached_info(phydev); >> - >> return 0; >> >> err_out_unregister_netdev: >> + phy_disconnect(bp->phy_dev); >> + mdiobus_unregister(bp->mii_bus); >> + mdiobus_free(bp->mii_bus); >> + >> + /* Shutdown the PHY if there is a GPIO reset */ >> + if (bp->reset_gpio) >> + gpiod_set_value(bp->reset_gpio, 0); >> + >> unregister_netdev(dev); >> >> err_out_free_netdev: >> > > Well, this fails with: > > [ 2.780000] ------------[ cut here ]------------ > [ 2.780000] WARNING: CPU: 0 PID: 1 at lib/kobject.c:597 kobject_get+0x6c/0xa0 > [ 2.790000] kobject: '(null)' (df532280): is not initialized, yet kobject_get() is being called. > [ 2.800000] Modules linked in: > [ 2.800000] CPU: 0 PID: 1 Comm: swapper Not tainted 4.6.0-rc1+ #46 > [ 2.810000] Hardware name: Atmel SAMA5 > [ 2.810000] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [ 2.820000] [] (show_stack) from [] (__warn+0xe4/0xfc) > [ 2.830000] [] (__warn) from [] (warn_slowpath_fmt+0x38/0x48) > [ 2.840000] [] (warn_slowpath_fmt) from [] (kobject_get+0x6c/0xa0) > [ 2.840000] [] (kobject_get) from [] (device_add+0xac/0x56c) > [ 2.850000] [] (device_add) from [] (__mdiobus_register+0x8c/0x198) > [ 2.860000] [] (__mdiobus_register) from [] (of_mdiobus_register+0x20/0x184) > [ 2.870000] [] (of_mdiobus_register) from [] (macb_probe+0x488/0x898) > [ 2.880000] [] (macb_probe) from [] (platform_drv_probe+0x4c/0xb0) > [ 2.880000] [] (platform_drv_probe) from [] (driver_probe_device+0x214/0x2c0) > [ 2.890000] [] (driver_probe_device) from [] (__driver_attach+0xb8/0xbc) > [ 2.900000] [] (__driver_attach) from [] (bus_for_each_dev+0x68/0x9c) > [ 2.910000] [] (bus_for_each_dev) from [] (bus_add_driver+0x1a0/0x218) > [ 2.920000] [] (bus_add_driver) from [] (driver_register+0x78/0xf8) > [ 2.930000] [] (driver_register) from [] (do_one_initcall+0x90/0x1dc) > [ 2.930000] [] (do_one_initcall) from [] (kernel_init_freeable+0x134/0x1d4) > [ 2.940000] [] (kernel_init_freeable) from [] (kernel_init+0x8/0x110) > [ 2.950000] [] (kernel_init) from [] (ret_from_fork+0x14/0x3c) > [ 2.960000] ---[ end trace 81bf87ef8c18d052 ]--- > > > I'm not completely clear why but I think one of the parent is not initialized > until register_netdev() is called. Yes, seems like it, how about adding this: diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index 98b99149ce0b..21096dfb0e83 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -440,7 +440,7 @@ static int macb_mii_init(struct macb *bp) snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", bp->pdev->name, bp->pdev->id); bp->mii_bus->priv = bp; - bp->mii_bus->parent = &bp->dev->dev; + bp->mii_bus->parent = &bp->pdev->dev; pdata = dev_get_platdata(&bp->pdev->dev); dev_set_drvdata(&bp->dev->dev, bp->mii_bus);