[RFC net-next 0/4] net: phy: PHY_HALTED, the return of the state

[RFC net-next 0/4] net: phy: PHY_HALTED, the return of the state

[Florian Fainelli]

Hi all,

This patch series tries to address the shortcomings of the previously and then
quickly reverted commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
Correctly process PHY_HALTED in phy_stop_machine()")

This time, the empire returns and strikes back with a few additional changes:

- catch phy_disconnect() calls without prior phy_stop() and warn when that
  happens since that means a driver is not behaving properly. This is AFAIR
  the case in which David Daney ran into

- what David also was running into is that when the PHY state machine was
  already in PHY_HALTED, its synchronous call in phy_disconnect() would make
  us re-schedule ourselves at the end. This is unnecessary, and we now take
  care of that

- finally, Geert experienced bus errors on smsc911x for a number of reasons,
  but the primary one is that the driver does not do any management of the
  PHY state machine during suspend/resume. The last patch corrects that, and
  also suggests that the driver should be fixed to properly support Wake-on-LAN
  configuration to possibly suspend the PHY.

David, Marc and Geert, I would appreciate if you could give this patch series
a spin on your respective HW and confirm that the desired functionality is
achieved.

Florian Fainelli (4):
  net: phy: Export phy_stop_machine()
  net: smsc911x: Properly manage PHY during suspend/resume
  net: phy: Force PHY_HALTED during phy_disconnect()
  net: phy: Correctly process PHY_HALTED in phy_stop_machine()

 drivers/net/ethernet/smsc/smsc911x.c |  7 +++++++
 drivers/net/phy/phy.c                | 13 +++++++++++--
 drivers/net/phy/phy_device.c         |  5 +++++
 3 files changed, 23 insertions(+), 2 deletions(-)

-- 
2.9.3

[RFC net-next 1/4] net: phy: Export phy_stop_machine()

[Florian Fainelli]

phy_stop_machine() is publicly exported in include/linux/phy.y, and is
not made static because it's also used by phy_device.c. Since
phy_start_machine() is already exported, do this here too. This is a
function that provides hard guarantees that the state machine is
properly stopped past that synchronization point.

Fixes: 00db8189d984 ("This patch adds a PHY Abstraction Layer to the Linux Kernel, enabling ethernet drivers to remain as ignorant as is reasonable of the connected PHY's design and operation details.")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2b1e67bc1e73..0ddeb97217ce 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -595,6 +595,7 @@ void phy_stop_machine(struct phy_device *phydev)
 		phydev->state = PHY_UP;
 	mutex_unlock(&phydev->lock);
 }
+EXPORT_SYMBOL_GPL(phy_stop_machine);
 
 /**
  * phy_error - enter HALTED state for this PHY device
-- 
2.9.3

[RFC net-next 2/4] net: smsc911x: Properly manage PHY during suspend/resume

[Florian Fainelli]

Commit 2aa70f864955 ("net: smsc911x: Quieten netif during suspend")
addressed the network device parts of the suspend/resume process, but
this is not enough, we can also need to manage the PHY state machine
during suspend.

Because Geert indicated that we are going to cut the power to the block,
we need the hard synchronization that phy_stop_machine() guarantees,
conversely use phy_start_machine() to resume properly.

There appears to be a desire to enable Wake-on-LAN, which is obviously
dependent on the PHY not being suspend, so we don't suspend it. A future
patch should probably add proper support for turning on/off Wake-on-LAN
on a PHY activity/password etc. basis.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 012fb66eed8d..82f9a175073f 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2598,6 +2598,11 @@ static int smsc911x_suspend(struct device *dev)
 	if (netif_running(ndev)) {
 		netif_stop_queue(ndev);
 		netif_device_detach(ndev);
+		phy_stop(ndev->phydev);
+		/* Use a hard synchronization here because we will cut the
+		 * power fo the block next
+		 */
+		phy_stop_machine(ndev->phydev);
 	}
 
 	/* enable wake on LAN, energy detection and the external PME
@@ -2638,6 +2643,8 @@ static int smsc911x_resume(struct device *dev)
 
 	if (netif_running(ndev)) {
 		netif_device_attach(ndev);
+		phy_start_machine(ndev->phydev);
+		phy_start(ndev->phydev);
 		netif_start_queue(ndev);
 	}
 
-- 
2.9.3

[RFC net-next 3/4] net: phy: Force PHY_HALTED during phy_disconnect()

[Florian Fainelli]

While debugging a crash reported by David Daney, we discovered that the
offending driver was calling phy_disconnect() without a prior call to
phy_stop() although it should have.

Add a WARN_ON() to catch such drivers, in order to invite their
maintainers to fix them, and also force the PHY state machine to
PHY_HALTED since we are about to disconnect anyway, there is nothing we
will be doing.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 67f25ac29025..69eb985c26fe 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -797,6 +797,11 @@ void phy_disconnect(struct phy_device *phydev)
 	if (phydev->irq > 0)
 		phy_stop_interrupts(phydev);
 
+	mutex_lock(&phydev->lock);
+	WARN_ON(phydev->state != PHY_HALTED);
+	phydev->state = PHY_HALTED;
+	mutex_unlock(&phydev->lock);
+
 	phy_stop_machine(phydev);
 
 	phydev->adjust_link = NULL;
-- 
2.9.3

[RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Florian Fainelli]

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@sigmadesigns.com>
Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0ddeb97217ce..20b84ea013c0 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -594,6 +594,9 @@ void phy_stop_machine(struct phy_device *phydev)
 	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);
 }
 EXPORT_SYMBOL_GPL(phy_stop_machine);
 
@@ -1077,9 +1080,14 @@ void phy_state_machine(struct work_struct *work)
 
 	/* 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()
+	 * between states from phy_mac_interrupt().
+	 *
+	 * If do_suspend is set to true from PHY_HALTED, in that case, do not
+	 * reschedule the state machine since that would be pointless and
+	 * possibly error prone when called from phy_disconnect()
+	 * synchronously.
 	 */
-	if (phydev->irq == PHY_POLL)
+	if (phydev->irq == PHY_POLL && !do_suspend)
 		queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
 				   PHY_STATE_TIME * HZ);
 }
-- 
2.9.3

Re: [RFC net-next 0/4] net: phy: PHY_HALTED, the return of the state

[Andrew Lunn]

On Wed, Oct 25, 2017 at 04:21:20PM -0700, Florian Fainelli wrote:
> Hi all,
> 
> This patch series tries to address the shortcomings of the previously and then
> quickly reverted commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
> Correctly process PHY_HALTED in phy_stop_machine()")
> 
> This time, the empire returns and strikes back with a few additional changes:
> 
> - catch phy_disconnect() calls without prior phy_stop() and warn when that
>   happens since that means a driver is not behaving properly. This is AFAIR
>   the case in which David Daney ran into
> 
> - what David also was running into is that when the PHY state machine was
>   already in PHY_HALTED, its synchronous call in phy_disconnect() would make
>   us re-schedule ourselves at the end. This is unnecessary, and we now take
>   care of that
> 
> - finally, Geert experienced bus errors on smsc911x for a number of reasons,
>   but the primary one is that the driver does not do any management of the
>   PHY state machine during suspend/resume. The last patch corrects that, and
>   also suggests that the driver should be fixed to properly support Wake-on-LAN
>   configuration to possibly suspend the PHY.
> 
> David, Marc and Geert, I would appreciate if you could give this patch series
> a spin on your respective HW and confirm that the desired functionality is
> achieved.

Hi Florian

I quickly look through these patches and they all seem
sensible. Feedback from the listed people would however be good.

	  Andrew

Re: [RFC net-next 1/4] net: phy: Export phy_stop_machine()

[Geert Uytterhoeven]

On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> phy_stop_machine() is publicly exported in include/linux/phy.y, and is
> not made static because it's also used by phy_device.c. Since
> phy_start_machine() is already exported, do this here too. This is a
> function that provides hard guarantees that the state machine is
> properly stopped past that synchronization point.
>
> Fixes: 00db8189d984 ("This patch adds a PHY Abstraction Layer to the Linux Kernel, enabling ethernet drivers to remain as ignorant as is reasonable of the connected PHY's design and operation details.")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

Re: [RFC net-next 2/4] net: smsc911x: Properly manage PHY during suspend/resume

[Geert Uytterhoeven]

On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> Commit 2aa70f864955 ("net: smsc911x: Quieten netif during suspend")
> addressed the network device parts of the suspend/resume process, but
> this is not enough, we can also need to manage the PHY state machine
> during suspend.
>
> Because Geert indicated that we are going to cut the power to the block,
> we need the hard synchronization that phy_stop_machine() guarantees,
> conversely use phy_start_machine() to resume properly.
>
> There appears to be a desire to enable Wake-on-LAN, which is obviously
> dependent on the PHY not being suspend, so we don't suspend it. A future
> patch should probably add proper support for turning on/off Wake-on-LAN
> on a PHY activity/password etc. basis.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-and-tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Geert Uytterhoeven]

Hi Florian,

On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@gmail.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@sigmadesigns.com>
> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.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:

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

Re: [RFC net-next 0/4] net: phy: PHY_HALTED, the return of the state

[Marc Gonzalez]

On 26/10/2017 01:21, Florian Fainelli wrote:

> This patch series tries to address the shortcomings of the previously and then
> quickly reverted commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
> Correctly process PHY_HALTED in phy_stop_machine()")
> 
> This time, the empire returns and strikes back with a few additional changes:
> 
> - catch phy_disconnect() calls without prior phy_stop() and warn when that
>   happens since that means a driver is not behaving properly. This is AFAIR
>   the case in which David Daney ran into
> 
> - what David also was running into is that when the PHY state machine was
>   already in PHY_HALTED, its synchronous call in phy_disconnect() would make
>   us re-schedule ourselves at the end. This is unnecessary, and we now take
>   care of that
> 
> - finally, Geert experienced bus errors on smsc911x for a number of reasons,
>   but the primary one is that the driver does not do any management of the
>   PHY state machine during suspend/resume. The last patch corrects that, and
>   also suggests that the driver should be fixed to properly support Wake-on-LAN
>   configuration to possibly suspend the PHY.
> 
> David, Marc and Geert, I would appreciate if you could give this patch series
> a spin on your respective HW and confirm that the desired functionality is
> achieved.

Hello Florian,

Thanks for taking a fresh look at this viper's nest :-)

I applied the patch series on top of v4.14-rc7, and my board had the expected
Ethernet functionality.

The "link down" notification does appear on the console shortly after issuing
the "ip link set eth0 down" command.

A one-minute iperf3 default run shows no particular issue.


I've had something in the back of my mind for a few weeks now: it seems like most
drivers try to disable the Ethernet hardware, and reclaim the packet descriptors,
when the ndo_stop() callback is invoked.

What happens when the HW does not support such a feature?

Case in point, the HW engineer responsible for integrating the Aurora IP told me
that it does not support disabling RX DMA. His exact words are:

> Once a descriptor chain is set by the software, the hardware will use
> them whenever a packet is received. There is no way to reclaim the
> buffers unless you reset the hardware.

So basically, the nb8800_dma_stop() function is a big hack, trying to disable
RX DMA, despite this being unsupported.

If I understand correctly, this is what we should do instead:

* When ndo_stop() is invoked, turn the Ethernet block off.
* Wait for the dust to settle.
* At this point, it should be safe to free the packet descriptors.
* As Mans pointed out, we would need to save the state of the HW block,
since turning it off discards context.

Regards.

Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Florian Fainelli]

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@gmail.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@sigmadesigns.com>
>> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.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@linux-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

Re: [RFC net-next 0/4] net: phy: PHY_HALTED, the return of the state

[David Daney]

On 10/25/2017 04:21 PM, Florian Fainelli wrote:
> Hi all,
> 
> This patch series tries to address the shortcomings of the previously and then
> quickly reverted commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
> Correctly process PHY_HALTED in phy_stop_machine()")
> 
> This time, the empire returns and strikes back with a few additional changes:
> 
> - catch phy_disconnect() calls without prior phy_stop() and warn when that
>    happens since that means a driver is not behaving properly. This is AFAIR
>    the case in which David Daney ran into
>

Light testing demonstrates that the links are usable and the systems no 
longer crash.  The WARNING you added is being activated, which indicates 
we need to fix the netdev drivers.  We are working on some patches for 
this now.

Thanks,
David



> - what David also was running into is that when the PHY state machine was
>    already in PHY_HALTED, its synchronous call in phy_disconnect() would make
>    us re-schedule ourselves at the end. This is unnecessary, and we now take
>    care of that
> 
> - finally, Geert experienced bus errors on smsc911x for a number of reasons,
>    but the primary one is that the driver does not do any management of the
>    PHY state machine during suspend/resume. The last patch corrects that, and
>    also suggests that the driver should be fixed to properly support Wake-on-LAN
>    configuration to possibly suspend the PHY.
> 
> David, Marc and Geert, I would appreciate if you could give this patch series
> a spin on your respective HW and confirm that the desired functionality is
> achieved.
> 
> Florian Fainelli (4):
>    net: phy: Export phy_stop_machine()
>    net: smsc911x: Properly manage PHY during suspend/resume
>    net: phy: Force PHY_HALTED during phy_disconnect()
>    net: phy: Correctly process PHY_HALTED in phy_stop_machine()
> 
>   drivers/net/ethernet/smsc/smsc911x.c |  7 +++++++
>   drivers/net/phy/phy.c                | 13 +++++++++++--
>   drivers/net/phy/phy_device.c         |  5 +++++
>   3 files changed, 23 insertions(+), 2 deletions(-)
> 

Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Geert Uytterhoeven]

Hi Florian,

On Mon, Oct 30, 2017 at 5:09 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/30/2017 06:56 AM, Geert Uytterhoeven wrote:
>> On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@gmail.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@sigmadesigns.com>
>>> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.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?

Unfortunately it doesn't help.

>> Unhandled fault: imprecise external abort (0x1406) at 0x0005b950

Note that this is an imprecise external abort, i.e. it's reporting may
be delayed,
and the backtrace may be inaccurate.

Gr{oetje,eeting}s,

                        Geert

Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Florian Fainelli]

On 10/31/2017 08:26 AM, Geert Uytterhoeven wrote:
> Hi Florian,
> 
> On Mon, Oct 30, 2017 at 5:09 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 10/30/2017 06:56 AM, Geert Uytterhoeven wrote:
>>> On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@gmail.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@sigmadesigns.com>
>>>> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.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?
> 
> Unfortunately it doesn't help.

OK :/

> 
>>> Unhandled fault: imprecise external abort (0x1406) at 0x0005b950
> 
> Note that this is an imprecise external abort, i.e. it's reporting may
> be delayed,
> and the backtrace may be inaccurate.

True, can you help narrow it down with me? Can you confirm that
adjust_link() (assuming that is the problem) does not get called past
phy_stop_machine() as it should?
-- 
Florian

Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Geert Uytterhoeven]

Hi Florian,

On Tue, Oct 31, 2017 at 5:33 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/31/2017 08:26 AM, Geert Uytterhoeven wrote:
>> On Mon, Oct 30, 2017 at 5:09 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 10/30/2017 06:56 AM, Geert Uytterhoeven wrote:
>>>> On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@gmail.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@sigmadesigns.com>
>>>>> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.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?
>>
>> Unfortunately it doesn't help.
>
> OK :/
>
>>
>>>> Unhandled fault: imprecise external abort (0x1406) at 0x0005b950
>>
>> Note that this is an imprecise external abort, i.e. it's reporting may
>> be delayed,
>> and the backtrace may be inaccurate.
>
> True, can you help narrow it down with me? Can you confirm that
> adjust_link() (assuming that is the problem) does not get called past
> phy_stop_machine() as it should?

I've added some additional debug checks (keep track of both phy and
smsc state, and refuse the access registers if smsc is disabled).

Apparently phy_stop_machine() is called twice:
  - Once from mdio_bus_phy_suspend(), cfr. the first backtrace,
  - A second time from smsc911x_suspend(), cfr. the second backtrace.

The second call causes a call to smsc911x_phy_adjust_link() while the smsc is
already disabled, cfr. the third backtrace. This would trigger the imprecise
external abort if I let it access the registers.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1083 at drivers/net/phy/phy.c:597
phy_stop_machine+0x44/0xcc
phy_stop_machine: phy running, good
CPU: 0 PID: 1083 Comm: bash Not tainted
4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
Hardware name: Generic R8A73A4 (Flattened Device Tree)
[<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
[<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
[<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
[<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
[<c002ac8c>] (warn_slowpath_fmt) from [<c0314680>] (phy_stop_machine+0x44/0xcc)
[<c0314680>] (phy_stop_machine) from [<c03162ec>]
(mdio_bus_phy_suspend+0x24/0x40)
[<c03162ec>] (mdio_bus_phy_suspend) from [<c0307090>]
(dpm_run_callback+0x17c/0x3ec)
[<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
[<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
[<c030a974>] (dpm_suspend) from [<c00880bc>]
(suspend_devices_and_enter+0x78/0xe98)
[<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
(pm_suspend+0xa40/0xbec)
[<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
[<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
[<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
[<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
[<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
[<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
---[ end trace 8fc4c71351438007 ]---
libphy: phy_stop_machine: Kicking state machine synchronously
libphy: phy_stop_machine: Kicking state machine done
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1083 at drivers/net/phy/phy.c:598
phy_stop_machine+0x64/0xcc
phy_stop_machine: phy already stopped
CPU: 0 PID: 1083 Comm: bash Tainted: G        W
4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
Hardware name: Generic R8A73A4 (Flattened Device Tree)
[<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
[<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
[<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
[<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
[<c002ac8c>] (warn_slowpath_fmt) from [<c03146a0>] (phy_stop_machine+0x64/0xcc)
[<c03146a0>] (phy_stop_machine) from [<c031a448>] (smsc911x_suspend+0x44/0xa4)
[<c031a448>] (smsc911x_suspend) from [<c0307090>] (dpm_run_callback+0x17c/0x3ec)
[<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
[<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
[<c030a974>] (dpm_suspend) from [<c00880bc>]
(suspend_devices_and_enter+0x78/0xe98)
[<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
(pm_suspend+0xa40/0xbec)
[<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
[<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
[<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
[<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
[<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
[<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
---[ end trace 8fc4c71351438008 ]---
libphy: phy_stop_machine: Kicking state machine synchronously
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1083 at drivers/net/ethernet/smsc/smsc911x.c:988
smsc911x_phy_adjust_link+0x2c/0x2e0
PHY stopped
CPU: 0 PID: 1083 Comm: bash Tainted: G        W
4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
Hardware name: Generic R8A73A4 (Flattened Device Tree)
[<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
[<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
[<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
[<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
[<c002ac8c>] (warn_slowpath_fmt) from [<c031b1e8>]
(smsc911x_phy_adjust_link+0x2c/0x2e0)
[<c031b1e8>] (smsc911x_phy_adjust_link) from [<c031378c>]
(phy_link_down+0x18/0x24)
[<c031378c>] (phy_link_down) from [<c0314518>] (phy_state_machine+0x2d0/0x3f4)
[<c0314518>] (phy_state_machine) from [<c03146d8>] (phy_stop_machine+0x9c/0xcc)
[<c03146d8>] (phy_stop_machine) from [<c031a448>] (smsc911x_suspend+0x44/0xa4)
[<c031a448>] (smsc911x_suspend) from [<c0307090>] (dpm_run_callback+0x17c/0x3ec)
[<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
[<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
[<c030a974>] (dpm_suspend) from [<c00880bc>]
(suspend_devices_and_enter+0x78/0xe98)
[<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
(pm_suspend+0xa40/0xbec)
[<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
[<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
[<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
[<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
[<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
[<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
---[ end trace 8fc4c71351438009 ]---
libphy: phy_stop_machine: Kicking state machine done

If I revert your "net: smsc911x: Properly manage PHY during suspend/resume",
phy_stop_machine() is no longer called twice, and system suspend works.

However, during resume, smsc911x_mii_read() is called before the
smsc is enabled, cfr. the fourth backtrace:

     WARNING: CPU: 1 PID: 17 at
drivers/net/ethernet/smsc/smsc911x.c:165 __smsc911x_reg_read+0x1c/0x88
    Modules linked in:
    CPU: 1 PID: 17 Comm: kworker/1:0 Tainted: G        W
4.14.0-rc7-kzm9g-00443-gcdfc0e18a47e0bb3-dirty #1013
    Hardware name: Generic SH73A0 (Flattened Device Tree)

smsc hangs of [fec10000.bus, which is started only here --->

    Workqueue: events_power_efficient phy_state_machine
    [<c010f304>] (unwind_backtrace) from [<c010b7ec>] (show_stack+0x10/0x14)
    [<c010b7ec>] (show_stack) from [<c0551ab0>] (dump_stack+0xa4/0xdc)
    [<c0551ab0>] (dump_stack) from [<c0120480>] (__warn+0xcc/0xfc)
    [<c0120480>] (__warn) from [<c0120554>] (warn_slowpath_null+0x1c/0x24)
    [<c0120554>] (warn_slowpath_null) from [<c03ce264>]
(__smsc911x_reg_read+0x1c/0x88)
    [<c03ce264>] (__smsc911x_reg_read) from [<c03cefc8>]
(smsc911x_mac_read+0x4c/0x118)
    [<c03cefc8>] (smsc911x_mac_read) from [<c03cf320>]
(smsc911x_mii_read+0x2c/0xa4)
    [<c03cf320>] (smsc911x_mii_read) from [<c03cd490>] (mdiobus_read+0x58/0x70)
    [<c03cd490>] (mdiobus_read) from [<c03cc4a0>] (genphy_update_link+0x18/0x50)
    [<c03cc4a0>] (genphy_update_link) from [<c03cc4e4>]
(genphy_read_status+0xc/0x1cc)
    [<c03cc4e4>] (genphy_read_status) from [<c03ca3e4>]
(phy_state_machine+0xa8/0x3f4)
    [<c03ca3e4>] (phy_state_machine) from [<c013a740>]
(process_one_work+0x240/0x3fc)
    [<c013a740>] (process_one_work) from [<c013af28>]
(worker_thread+0x2cc/0x40c)
    [<c013af28>] (worker_thread) from [<c014024c>] (kthread+0x124/0x144)
    [<c014024c>] (kthread) from [<c01071c8>] (ret_from_fork+0x14/0x2c)
    ---[ end trace 21b7024e273f9f21 ]---
    ------------[ cut here ]------------

(yes, the fourth backtrace is from another machine, but I can trigger
all of this
 on both r8a73a4/ape6evm and sh73a0/kzm9g anyway).

Gr{oetje,eeting}s,

                        Geert

Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Florian Fainelli]

Hi Geert,

On 11/06/2017 07:50 AM, Geert Uytterhoeven wrote:
> Hi Florian,
> 
> On Tue, Oct 31, 2017 at 5:33 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 10/31/2017 08:26 AM, Geert Uytterhoeven wrote:
>>> On Mon, Oct 30, 2017 at 5:09 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 10/30/2017 06:56 AM, Geert Uytterhoeven wrote:
>>>>> On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@gmail.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@sigmadesigns.com>
>>>>>> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
>>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.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?
>>>
>>> Unfortunately it doesn't help.
>>
>> OK :/
>>
>>>
>>>>> Unhandled fault: imprecise external abort (0x1406) at 0x0005b950
>>>
>>> Note that this is an imprecise external abort, i.e. it's reporting may
>>> be delayed,
>>> and the backtrace may be inaccurate.
>>
>> True, can you help narrow it down with me? Can you confirm that
>> adjust_link() (assuming that is the problem) does not get called past
>> phy_stop_machine() as it should?
> 
> I've added some additional debug checks (keep track of both phy and
> smsc state, and refuse the access registers if smsc is disabled).

Thanks for doing that, and sorry for responding that late.

> 
> Apparently phy_stop_machine() is called twice:
>   - Once from mdio_bus_phy_suspend(), cfr. the first backtrace,
>   - A second time from smsc911x_suspend(), cfr. the second backtrace.
> 
> The second call causes a call to smsc911x_phy_adjust_link() while the smsc is
> already disabled, cfr. the third backtrace. This would trigger the imprecise
> external abort if I let it access the registers.
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1083 at drivers/net/phy/phy.c:597
> phy_stop_machine+0x44/0xcc
> phy_stop_machine: phy running, good
> CPU: 0 PID: 1083 Comm: bash Not tainted
> 4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
> Hardware name: Generic R8A73A4 (Flattened Device Tree)
> [<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
> [<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
> [<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
> [<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
> [<c002ac8c>] (warn_slowpath_fmt) from [<c0314680>] (phy_stop_machine+0x44/0xcc)
> [<c0314680>] (phy_stop_machine) from [<c03162ec>]
> (mdio_bus_phy_suspend+0x24/0x40)
> [<c03162ec>] (mdio_bus_phy_suspend) from [<c0307090>]
> (dpm_run_callback+0x17c/0x3ec)
> [<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
> [<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
> [<c030a974>] (dpm_suspend) from [<c00880bc>]
> (suspend_devices_and_enter+0x78/0xe98)
> [<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
> (pm_suspend+0xa40/0xbec)
> [<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
> [<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
> [<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
> [<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
> [<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
> [<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
> ---[ end trace 8fc4c71351438007 ]---
> libphy: phy_stop_machine: Kicking state machine synchronously
> libphy: phy_stop_machine: Kicking state machine done
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1083 at drivers/net/phy/phy.c:598
> phy_stop_machine+0x64/0xcc
> phy_stop_machine: phy already stopped
> CPU: 0 PID: 1083 Comm: bash Tainted: G        W
> 4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
> Hardware name: Generic R8A73A4 (Flattened Device Tree)
> [<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
> [<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
> [<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
> [<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
> [<c002ac8c>] (warn_slowpath_fmt) from [<c03146a0>] (phy_stop_machine+0x64/0xcc)
> [<c03146a0>] (phy_stop_machine) from [<c031a448>] (smsc911x_suspend+0x44/0xa4)
> [<c031a448>] (smsc911x_suspend) from [<c0307090>] (dpm_run_callback+0x17c/0x3ec)
> [<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
> [<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
> [<c030a974>] (dpm_suspend) from [<c00880bc>]
> (suspend_devices_and_enter+0x78/0xe98)
> [<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
> (pm_suspend+0xa40/0xbec)
> [<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
> [<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
> [<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
> [<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
> [<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
> [<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
> ---[ end trace 8fc4c71351438008 ]---
> libphy: phy_stop_machine: Kicking state machine synchronously
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1083 at drivers/net/ethernet/smsc/smsc911x.c:988
> smsc911x_phy_adjust_link+0x2c/0x2e0
> PHY stopped
> CPU: 0 PID: 1083 Comm: bash Tainted: G        W
> 4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
> Hardware name: Generic R8A73A4 (Flattened Device Tree)
> [<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
> [<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
> [<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
> [<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
> [<c002ac8c>] (warn_slowpath_fmt) from [<c031b1e8>]
> (smsc911x_phy_adjust_link+0x2c/0x2e0)
> [<c031b1e8>] (smsc911x_phy_adjust_link) from [<c031378c>]
> (phy_link_down+0x18/0x24)
> [<c031378c>] (phy_link_down) from [<c0314518>] (phy_state_machine+0x2d0/0x3f4)
> [<c0314518>] (phy_state_machine) from [<c03146d8>] (phy_stop_machine+0x9c/0xcc)
> [<c03146d8>] (phy_stop_machine) from [<c031a448>] (smsc911x_suspend+0x44/0xa4)
> [<c031a448>] (smsc911x_suspend) from [<c0307090>] (dpm_run_callback+0x17c/0x3ec)
> [<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
> [<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
> [<c030a974>] (dpm_suspend) from [<c00880bc>]
> (suspend_devices_and_enter+0x78/0xe98)
> [<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
> (pm_suspend+0xa40/0xbec)
> [<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
> [<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
> [<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
> [<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
> [<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
> [<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
> ---[ end trace 8fc4c71351438009 ]---
> libphy: phy_stop_machine: Kicking state machine done
> 
> If I revert your "net: smsc911x: Properly manage PHY during suspend/resume",
> phy_stop_machine() is no longer called twice, and system suspend works.

OK, there does appear to be a problem in how the network device vs. mdio
bus are suspended/resumed in this particular driver, and I have no idea
why, see below.

> 
> However, during resume, smsc911x_mii_read() is called before the
> smsc is enabled, cfr. the fourth backtrace:

Humm, how is that possible? smsc911x_mii_bus properly sets its parent
device to be the platform device of the network device (which is
correct) so by virtue of child -> parent relationships, we should have
the network device resume first, and then have the MDIO bus resume
second (unless I am completely off here).

> 
>      WARNING: CPU: 1 PID: 17 at
> drivers/net/ethernet/smsc/smsc911x.c:165 __smsc911x_reg_read+0x1c/0x88
>     Modules linked in:
>     CPU: 1 PID: 17 Comm: kworker/1:0 Tainted: G        W
> 4.14.0-rc7-kzm9g-00443-gcdfc0e18a47e0bb3-dirty #1013
>     Hardware name: Generic SH73A0 (Flattened Device Tree)
> 
> smsc hangs of [fec10000.bus, which is started only here --->
> 
>     Workqueue: events_power_efficient phy_state_machine
>     [<c010f304>] (unwind_backtrace) from [<c010b7ec>] (show_stack+0x10/0x14)
>     [<c010b7ec>] (show_stack) from [<c0551ab0>] (dump_stack+0xa4/0xdc)
>     [<c0551ab0>] (dump_stack) from [<c0120480>] (__warn+0xcc/0xfc)
>     [<c0120480>] (__warn) from [<c0120554>] (warn_slowpath_null+0x1c/0x24)
>     [<c0120554>] (warn_slowpath_null) from [<c03ce264>]
> (__smsc911x_reg_read+0x1c/0x88)
>     [<c03ce264>] (__smsc911x_reg_read) from [<c03cefc8>]
> (smsc911x_mac_read+0x4c/0x118)
>     [<c03cefc8>] (smsc911x_mac_read) from [<c03cf320>]
> (smsc911x_mii_read+0x2c/0xa4)
>     [<c03cf320>] (smsc911x_mii_read) from [<c03cd490>] (mdiobus_read+0x58/0x70)
>     [<c03cd490>] (mdiobus_read) from [<c03cc4a0>] (genphy_update_link+0x18/0x50)
>     [<c03cc4a0>] (genphy_update_link) from [<c03cc4e4>]
> (genphy_read_status+0xc/0x1cc)
>     [<c03cc4e4>] (genphy_read_status) from [<c03ca3e4>]
> (phy_state_machine+0xa8/0x3f4)
>     [<c03ca3e4>] (phy_state_machine) from [<c013a740>]
> (process_one_work+0x240/0x3fc)
>     [<c013a740>] (process_one_work) from [<c013af28>]
> (worker_thread+0x2cc/0x40c)
>     [<c013af28>] (worker_thread) from [<c014024c>] (kthread+0x124/0x144)
>     [<c014024c>] (kthread) from [<c01071c8>] (ret_from_fork+0x14/0x2c)
>     ---[ end trace 21b7024e273f9f21 ]---
>     ------------[ cut here ]------------
> 
> (yes, the fourth backtrace is from another machine, but I can trigger
> all of this
>  on both r8a73a4/ape6evm and sh73a0/kzm9g anyway).
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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

Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Geert Uytterhoeven]

Hi Florian,

On Mon, Nov 27, 2017 at 5:05 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/06/2017 07:50 AM, Geert Uytterhoeven wrote:
>> On Tue, Oct 31, 2017 at 5:33 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 10/31/2017 08:26 AM, Geert Uytterhoeven wrote:
>>>> On Mon, Oct 30, 2017 at 5:09 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> On 10/30/2017 06:56 AM, Geert Uytterhoeven wrote:
>>>>>> On Thu, Oct 26, 2017 at 1:21 AM, Florian Fainelli <f.fainelli@gmail.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@sigmadesigns.com>
>>>>>>> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
>>>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.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?
>>>>
>>>> Unfortunately it doesn't help.
>>>
>>> OK :/
>>>
>>>>
>>>>>> Unhandled fault: imprecise external abort (0x1406) at 0x0005b950
>>>>
>>>> Note that this is an imprecise external abort, i.e. it's reporting may
>>>> be delayed,
>>>> and the backtrace may be inaccurate.
>>>
>>> True, can you help narrow it down with me? Can you confirm that
>>> adjust_link() (assuming that is the problem) does not get called past
>>> phy_stop_machine() as it should?
>>
>> I've added some additional debug checks (keep track of both phy and
>> smsc state, and refuse the access registers if smsc is disabled).
>
> Thanks for doing that, and sorry for responding that late.
>
>>
>> Apparently phy_stop_machine() is called twice:
>>   - Once from mdio_bus_phy_suspend(), cfr. the first backtrace,
>>   - A second time from smsc911x_suspend(), cfr. the second backtrace.
>>
>> The second call causes a call to smsc911x_phy_adjust_link() while the smsc is
>> already disabled, cfr. the third backtrace. This would trigger the imprecise
>> external abort if I let it access the registers.
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1083 at drivers/net/phy/phy.c:597
>> phy_stop_machine+0x44/0xcc
>> phy_stop_machine: phy running, good
>> CPU: 0 PID: 1083 Comm: bash Not tainted
>> 4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
>> Hardware name: Generic R8A73A4 (Flattened Device Tree)
>> [<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
>> [<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
>> [<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
>> [<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
>> [<c002ac8c>] (warn_slowpath_fmt) from [<c0314680>] (phy_stop_machine+0x44/0xcc)
>> [<c0314680>] (phy_stop_machine) from [<c03162ec>]
>> (mdio_bus_phy_suspend+0x24/0x40)
>> [<c03162ec>] (mdio_bus_phy_suspend) from [<c0307090>]
>> (dpm_run_callback+0x17c/0x3ec)
>> [<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
>> [<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
>> [<c030a974>] (dpm_suspend) from [<c00880bc>]
>> (suspend_devices_and_enter+0x78/0xe98)
>> [<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
>> (pm_suspend+0xa40/0xbec)
>> [<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
>> [<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
>> [<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
>> [<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
>> [<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
>> [<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
>> ---[ end trace 8fc4c71351438007 ]---
>> libphy: phy_stop_machine: Kicking state machine synchronously
>> libphy: phy_stop_machine: Kicking state machine done
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1083 at drivers/net/phy/phy.c:598
>> phy_stop_machine+0x64/0xcc
>> phy_stop_machine: phy already stopped
>> CPU: 0 PID: 1083 Comm: bash Tainted: G        W
>> 4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
>> Hardware name: Generic R8A73A4 (Flattened Device Tree)
>> [<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
>> [<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
>> [<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
>> [<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
>> [<c002ac8c>] (warn_slowpath_fmt) from [<c03146a0>] (phy_stop_machine+0x64/0xcc)
>> [<c03146a0>] (phy_stop_machine) from [<c031a448>] (smsc911x_suspend+0x44/0xa4)
>> [<c031a448>] (smsc911x_suspend) from [<c0307090>] (dpm_run_callback+0x17c/0x3ec)
>> [<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
>> [<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
>> [<c030a974>] (dpm_suspend) from [<c00880bc>]
>> (suspend_devices_and_enter+0x78/0xe98)
>> [<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
>> (pm_suspend+0xa40/0xbec)
>> [<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
>> [<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
>> [<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
>> [<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
>> [<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
>> [<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
>> ---[ end trace 8fc4c71351438008 ]---
>> libphy: phy_stop_machine: Kicking state machine synchronously
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1083 at drivers/net/ethernet/smsc/smsc911x.c:988
>> smsc911x_phy_adjust_link+0x2c/0x2e0
>> PHY stopped
>> CPU: 0 PID: 1083 Comm: bash Tainted: G        W
>> 4.14.0-rc7-ape6evm-00443-gcdfc0e18a47e0bb3-dirty #637
>> Hardware name: Generic R8A73A4 (Flattened Device Tree)
>> [<c0018f3c>] (unwind_backtrace) from [<c0014f40>] (show_stack+0x10/0x14)
>> [<c0014f40>] (show_stack) from [<c04bd928>] (dump_stack+0xa4/0xdc)
>> [<c04bd928>] (dump_stack) from [<c002ac28>] (__warn+0xcc/0xfc)
>> [<c002ac28>] (__warn) from [<c002ac8c>] (warn_slowpath_fmt+0x34/0x44)
>> [<c002ac8c>] (warn_slowpath_fmt) from [<c031b1e8>]
>> (smsc911x_phy_adjust_link+0x2c/0x2e0)
>> [<c031b1e8>] (smsc911x_phy_adjust_link) from [<c031378c>]
>> (phy_link_down+0x18/0x24)
>> [<c031378c>] (phy_link_down) from [<c0314518>] (phy_state_machine+0x2d0/0x3f4)
>> [<c0314518>] (phy_state_machine) from [<c03146d8>] (phy_stop_machine+0x9c/0xcc)
>> [<c03146d8>] (phy_stop_machine) from [<c031a448>] (smsc911x_suspend+0x44/0xa4)
>> [<c031a448>] (smsc911x_suspend) from [<c0307090>] (dpm_run_callback+0x17c/0x3ec)
>> [<c0307090>] (dpm_run_callback) from [<c030a584>] (__device_suspend+0x498/0x6b0)
>> [<c030a584>] (__device_suspend) from [<c030a974>] (dpm_suspend+0x1d8/0x568)
>> [<c030a974>] (dpm_suspend) from [<c00880bc>]
>> (suspend_devices_and_enter+0x78/0xe98)
>> [<c00880bc>] (suspend_devices_and_enter) from [<c008991c>]
>> (pm_suspend+0xa40/0xbec)
>> [<c008991c>] (pm_suspend) from [<c0086e68>] (state_store+0xac/0xcc)
>> [<c0086e68>] (state_store) from [<c01c7780>] (kernfs_fop_write+0x190/0x1d0)
>> [<c01c7780>] (kernfs_fop_write) from [<c0153418>] (__vfs_write+0x20/0x11c)
>> [<c0153418>] (__vfs_write) from [<c0153688>] (vfs_write+0xb8/0x144)
>> [<c0153688>] (vfs_write) from [<c0153808>] (SyS_write+0x40/0x80)
>> [<c0153808>] (SyS_write) from [<c0010200>] (ret_fast_syscall+0x0/0x28)
>> ---[ end trace 8fc4c71351438009 ]---
>> libphy: phy_stop_machine: Kicking state machine done
>>
>> If I revert your "net: smsc911x: Properly manage PHY during suspend/resume",
>> phy_stop_machine() is no longer called twice, and system suspend works.
>
> OK, there does appear to be a problem in how the network device vs. mdio
> bus are suspended/resumed in this particular driver, and I have no idea
> why, see below.
>
>>
>> However, during resume, smsc911x_mii_read() is called before the
>> smsc is enabled, cfr. the fourth backtrace:
>
> Humm, how is that possible? smsc911x_mii_bus properly sets its parent
> device to be the platform device of the network device (which is
> correct) so by virtue of child -> parent relationships, we should have
> the network device resume first, and then have the MDIO bus resume
> second (unless I am completely off here).

The MDIO bus callback is not called from the network device ...

>>      WARNING: CPU: 1 PID: 17 at
>> drivers/net/ethernet/smsc/smsc911x.c:165 __smsc911x_reg_read+0x1c/0x88
>>     Modules linked in:
>>     CPU: 1 PID: 17 Comm: kworker/1:0 Tainted: G        W
>> 4.14.0-rc7-kzm9g-00443-gcdfc0e18a47e0bb3-dirty #1013
>>     Hardware name: Generic SH73A0 (Flattened Device Tree)
>>
>> smsc hangs of [fec10000.bus, which is started only here --->
>>
>>     Workqueue: events_power_efficient phy_state_machine
>>     [<c010f304>] (unwind_backtrace) from [<c010b7ec>] (show_stack+0x10/0x14)
>>     [<c010b7ec>] (show_stack) from [<c0551ab0>] (dump_stack+0xa4/0xdc)
>>     [<c0551ab0>] (dump_stack) from [<c0120480>] (__warn+0xcc/0xfc)
>>     [<c0120480>] (__warn) from [<c0120554>] (warn_slowpath_null+0x1c/0x24)
>>     [<c0120554>] (warn_slowpath_null) from [<c03ce264>]
>> (__smsc911x_reg_read+0x1c/0x88)
>>     [<c03ce264>] (__smsc911x_reg_read) from [<c03cefc8>]
>> (smsc911x_mac_read+0x4c/0x118)
>>     [<c03cefc8>] (smsc911x_mac_read) from [<c03cf320>]
>> (smsc911x_mii_read+0x2c/0xa4)
>>     [<c03cf320>] (smsc911x_mii_read) from [<c03cd490>] (mdiobus_read+0x58/0x70)
>>     [<c03cd490>] (mdiobus_read) from [<c03cc4a0>] (genphy_update_link+0x18/0x50)
>>     [<c03cc4a0>] (genphy_update_link) from [<c03cc4e4>]
>> (genphy_read_status+0xc/0x1cc)
>>     [<c03cc4e4>] (genphy_read_status) from [<c03ca3e4>]
>> (phy_state_machine+0xa8/0x3f4)
>>     [<c03ca3e4>] (phy_state_machine) from [<c013a740>]
>> (process_one_work+0x240/0x3fc)
>>     [<c013a740>] (process_one_work) from [<c013af28>]
>> (worker_thread+0x2cc/0x40c)
>>     [<c013af28>] (worker_thread) from [<c014024c>] (kthread+0x124/0x144)

... but from the worker thread, which is unaware of PM states, unless it is
a freezable workqueue.

Cfr. my patch "[1/2] net: phy: Freeze PHY polling before suspending devices"
(https://patchwork.kernel.org/patch/9915901/), which made it a freezable
workqueue, like was done on PCI to solve similar issues with PME scanning.

>>     [<c014024c>] (kthread) from [<c01071c8>] (ret_from_fork+0x14/0x2c)
>>     ---[ end trace 21b7024e273f9f21 ]---
>>     ------------[ cut here ]------------
>>
>> (yes, the fourth backtrace is from another machine, but I can trigger
>> all of this
>>  on both r8a73a4/ape6evm and sh73a0/kzm9g anyway).

Gr{oetje,eeting}s,

                        Geert

Re: [RFC net-next 4/4] net: phy: Correctly process PHY_HALTED in phy_stop_machine()

[Marc Gonzalez]

On 27/11/2017 08:48, Geert Uytterhoeven wrote:

> The MDIO bus callback is not called from the network device ...
> 
>>> smsc hangs of [fec10000.bus, which is started only here --->
>>>
>>>     Workqueue: events_power_efficient phy_state_machine
>>>     [<c010f304>] (unwind_backtrace) from [<c010b7ec>] (show_stack+0x10/0x14)
>>>     [<c010b7ec>] (show_stack) from [<c0551ab0>] (dump_stack+0xa4/0xdc)
>>>     [<c0551ab0>] (dump_stack) from [<c0120480>] (__warn+0xcc/0xfc)
>>>     [<c0120480>] (__warn) from [<c0120554>] (warn_slowpath_null+0x1c/0x24)
>>>     [<c0120554>] (warn_slowpath_null) from [<c03ce264>] (__smsc911x_reg_read+0x1c/0x88)
>>>     [<c03ce264>] (__smsc911x_reg_read) from [<c03cefc8>] (smsc911x_mac_read+0x4c/0x118)
>>>     [<c03cefc8>] (smsc911x_mac_read) from [<c03cf320>] (smsc911x_mii_read+0x2c/0xa4)
>>>     [<c03cf320>] (smsc911x_mii_read) from [<c03cd490>] (mdiobus_read+0x58/0x70)
>>>     [<c03cd490>] (mdiobus_read) from [<c03cc4a0>] (genphy_update_link+0x18/0x50)
>>>     [<c03cc4a0>] (genphy_update_link) from [<c03cc4e4>] (genphy_read_status+0xc/0x1cc)
>>>     [<c03cc4e4>] (genphy_read_status) from [<c03ca3e4>] (phy_state_machine+0xa8/0x3f4)
>>>     [<c03ca3e4>] (phy_state_machine) from [<c013a740>] (process_one_work+0x240/0x3fc)
>>>     [<c013a740>] (process_one_work) from [<c013af28>] (worker_thread+0x2cc/0x40c)
>>>     [<c013af28>] (worker_thread) from [<c014024c>] (kthread+0x124/0x144)
>
> ... but from the worker thread, which is unaware of PM states, unless it is
> a freezable workqueue.
> 
> Cfr. my patch "[1/2] net: phy: Freeze PHY polling before suspending devices"
> (https://patchwork.kernel.org/patch/9915901/), which made it a freezable
> workqueue, like was done on PCI to solve similar issues with PME scanning.

Hello Geert, Florian,

Is there something I can do to help this patch move forward?

Should we look for a different solution than a synchronous call
to phy_state_machine() ? (Or should I call it from the device
driver, instead of expecting the phy framework to do it?)

Regards.