Date:   Mon, 30 Oct 2017 09:09:10 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Marc Gonzalez <marc_gonzalez@...madesigns.com>,
        "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>, opendmb@...il.com,
        Mason <slash.tmp@...e.fr>, David Daney <david.daney@...ium.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>
Subject: Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in
 phy_stop_machine()

On 10/30/2017 06:56 AM, Geert Uytterhoeven wrote:
> Hi Florian,
> 
> On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@...il.com> 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 set the state machine to
>> PHY_HALTED but we don't get to run it to process this state past that
>> point.
>>
>> Fix this with 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.
>>
>> At the end of phy_state_machine() though, if we are going to be moving
>> from PHY_HALTED to PHY_HALTED, do not reschedule the state machine, this
>> is pointless.
>>
>> Reported-by: Marc Gonzalez <marc_gonzalez@...madesigns.com>
>> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@...madesigns.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> 
> Thanks for your patch!
> 
> Unfortunately, after applying this one, the last in your series, both
> sh73a0/kzm9g and r8a73a4/ape6evm start crashing again in the system
> suspend/resume path, due to register accesses while the device is already
> suspended:

OK, seems like there is another path, uncovered by this patch that we
can be hitting, does the following patch below help?

diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 82f9a175073f..51498699b18c 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1136,6 +1136,9 @@ static void smsc911x_tx_update_txcounters(struct
net_device *dev)
        struct smsc911x_data *pdata = netdev_priv(dev);
        unsigned int tx_stat;

+       if (!netif_running(dev))
+               return;
+
        while ((tx_stat = smsc911x_tx_get_txstatus(pdata)) != 0) {
                if (unlikely(tx_stat & 0x80000000)) {
                        /* In this driver the packet tag is used as the
packet

> 
> PM: suspend entry (deep)
> PM: Syncing filesystems ... done.
> Freezing user space processes ... (elapsed 0.000 seconds) done.
> OOM killer disabled.
> Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
> PM: suspend devices took 0.100 seconds
> Unhandled fault: imprecise external abort (0x1406) at 0x0005b950
> Disabling non-boot CPUs ...
> pgd = c0004000
> [0005b950] *pgd=00000000
> Internal error: : 1406 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 17 Comm: kworker/1:0 Not tainted
> 4.14.0-rc5-kzm9g-00454-g13bf0e7c8794d746 #1002
> Hardware name: Generic SH73A0 (Flattened Device Tree)
> Workqueue: events linkwatch_event
> task: df490480 task.stack: df492000
> PC is at __smsc911x_reg_read+0x1c/0x60
> LR is at smsc911x_tx_get_txstatus+0x64/0x7c
> pc : [<c03cc77c>]    lr : [<c03cd000>]    psr: 20030093
> sp : df493d98  ip : 00000000  fp : c09313a0
> r10: c0931330  r9 : c0909a40  r8 : 00000000
> r7 : 00030013  r6 : df705e08  r5 : df705dc0  r4 : 001c0000
> r3 : e0903000  r2 : 00000001  r1 : e0903048  r0 : 00000000
> Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 5ed1804a  DAC: 00000051
> Process kworker/1:0 (pid: 17, stack limit = 0xdf492210)
> Stack: (0xdf493d98 to 0xdf494000)
> 3d80:                                                       c03cd2dc df705800
> 3da0: df705dc0 c063b2b0 df493e18 c03cd248 c03cd2dc df705800 df705800 c03cd2e8
> 3dc0: c03cd2dc def4b0e4 df705800 c063b2b0 df493e18 c048a13c def4b0e0 dedbb300
> 3de0: df705800 c04ab388 dedbb300 df705800 def4b000 c04a5e10 0000002a 00000000
> 3e00: c04a5a8c c01ffcfc c04aabd0 dedbb300 014000c0 df493e47 00000000 00000000
> 3e20: 00000000 00000000 00000000 00000000 00000050 00000000 df493e47 014000c0
> 3e40: 00000000 dedbb300 df705800 00000010 00000000 00000000 00000000 c0931330
> 3e60: c09313a0 c04aac00 00000000 00000000 00000000 00000000 00000000 00000000
> 3e80: 014000c0 df705800 df705800 c09313a0 df493ee0 c04aac9c 014000c0 00000000
> 3ea0: 00000000 df705800 c0931330 c04aace0 014000c0 00000000 00000000 c048ef18
> 3ec0: df705800 df705800 00000000 00000000 df705800 c04abee0 df705ab0 c04ac150
> 3ee0: df493ee0 df493ee0 c109a9f8 df436d00 c0931330 dfbd7bc0 df493f30 dfbdae00
> 3f00: 00000000 00000000 00000001 c04ac1bc c04ac198 c013a6f8 00000001 00000000
> 3f20: c013a680 00000000 00000000 00000000 c0931330 00000000 00000000 c0758db6
> 3f40: c0905900 df436d00 dfbd7bc0 dfbd7bc0 df492000 dfbd7bf4 c0905900 df436d18
> 3f60: 00000008 c013aecc df490480 df436400 df42c900 00000000 df443e6c df436d00
> 3f80: c013ac14 df436438 00000000 c0140200 df42c900 c01400dc 00000000 00000000
> 3fa0: 00000000 00000000 00000000 c01071c8 00000000 00000000 00000000 00000000
> 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 7fdfffff fff7fdff
> [<c03cc77c>] (__smsc911x_reg_read) from [<c03cd000>]
> (smsc911x_tx_get_txstatus+0x64/0x7c)
> [<c03cd000>] (smsc911x_tx_get_txstatus) from [<c03cd248>]
> (smsc911x_tx_update_txcounters+0x14/0xa8)
> [<c03cd248>] (smsc911x_tx_update_txcounters) from [<c03cd2e8>]
> (smsc911x_get_stats+0xc/0x58)
> [<c03cd2e8>] (smsc911x_get_stats) from [<c048a13c>] (dev_get_stats+0x54/0xa4)
> [<c048a13c>] (dev_get_stats) from [<c04ab388>] (rtnl_fill_stats+0x38/0x118)
> [<c04ab388>] (rtnl_fill_stats) from [<c04a5e10>] (rtnl_fill_ifinfo+0x5f0/0xf74)
> [<c04a5e10>] (rtnl_fill_ifinfo) from [<c04aac00>]
> (rtmsg_ifinfo_build_skb+0x6c/0xc0)
> [<c04aac00>] (rtmsg_ifinfo_build_skb) from [<c04aac9c>]
> (rtmsg_ifinfo_event.part.5+0x1c/0x40)
> [<c04aac9c>] (rtmsg_ifinfo_event.part.5) from [<c04aace0>]
> (rtmsg_ifinfo+0x20/0x28)
> [<c04aace0>] (rtmsg_ifinfo) from [<c048ef18>] (netdev_state_change+0x48/0x54)
> [<c048ef18>] (netdev_state_change) from [<c04abee0>]
> (linkwatch_do_dev+0x50/0x74)
> [<c04abee0>] (linkwatch_do_dev) from [<c04ac150>]
> (__linkwatch_run_queue+0x124/0x16c)
> [<c04ac150>] (__linkwatch_run_queue) from [<c04ac1bc>]
> (linkwatch_event+0x24/0x34)
> [<c04ac1bc>] (linkwatch_event) from [<c013a6f8>] (process_one_work+0x240/0x3fc)
> [<c013a6f8>] (process_one_work) from [<c013aecc>] (worker_thread+0x2b8/0x3f4)
> [<c013aecc>] (worker_thread) from [<c0140200>] (kthread+0x124/0x144)
> [<c0140200>] (kthread) from [<c01071c8>] (ret_from_fork+0x14/0x2c)
> Code: e5903000 e0831001 e5910000 f57ff04f (e12fff1e)
> ---[ end trace e71c0b5246c61082 ]---
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
Florian

Powered by blists - more mailing lists