[PATCH net-next 0/2] net: phy: make phy_stop() synchronous

[PATCH net-next 0/2] net: phy: make phy_stop() synchronous

[Heiner Kallweit]

There have been few not that successful attempts in the past to make
phy_stop() a synchronous call instead of just changing the state.
Patch 1 of this series addresses an issue which prevented this change.
At least for me it works fine now. Would appreciate if Geert could
re-test as well that suspend doesn't throw an error.

Heiner Kallweit (2):
  net: linkwatch: add check for netdevice being present to linkwatch_do_dev
  net: phy: call state machine synchronously in phy_stop

 drivers/net/phy/phy.c | 2 ++
 net/core/link_watch.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
2.19.0

[PATCH net-next 1/2] net: linkwatch: add check for netdevice being present to linkwatch_do_dev

[Heiner Kallweit]

When bringing down the netdevice (incl. detaching it) and calling
netif_carrier_off directly or indirectly the latter triggers an
asynchronous linkwatch event.
This linkwatch event eventually may fail to access chip registers in
the ndo_get_stats/ndo_get_stats64 callback because the device isn't
accessible any longer, see call trace in [0].

To prevent this scenario don't check for IFF_UP only, but also make
sure that the netdevice is present.

[0] https://lists.openwall.net/netdev/2018/03/15/62

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 net/core/link_watch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index e38e641e9..7f51efb2b 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -155,7 +155,7 @@ static void linkwatch_do_dev(struct net_device *dev)
 	clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
 
 	rfc2863_policy(dev);
-	if (dev->flags & IFF_UP) {
+	if (dev->flags & IFF_UP && netif_device_present(dev)) {
 		if (netif_carrier_ok(dev))
 			dev_activate(dev);
 		else
-- 
2.19.0

Re: [PATCH net-next 1/2] net: linkwatch: add check for netdevice being present to linkwatch_do_dev

[Geert Uytterhoeven]

On Tue, Sep 18, 2018 at 9:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> When bringing down the netdevice (incl. detaching it) and calling
> netif_carrier_off directly or indirectly the latter triggers an
> asynchronous linkwatch event.
> This linkwatch event eventually may fail to access chip registers in
> the ndo_get_stats/ndo_get_stats64 callback because the device isn't
> accessible any longer, see call trace in [0].
>
> To prevent this scenario don't check for IFF_UP only, but also make
> sure that the netdevice is present.
>
> [0] https://lists.openwall.net/netdev/2018/03/15/62
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Survived 100 suspend/resume cycles on sh73a0/kzm9g and r8a73a4/ape6evm.

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

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

Re: [PATCH net-next 1/2] net: linkwatch: add check for netdevice being present to linkwatch_do_dev

[Florian Fainelli]

On 09/18/2018 12:55 PM, Heiner Kallweit wrote:
> When bringing down the netdevice (incl. detaching it) and calling
> netif_carrier_off directly or indirectly the latter triggers an
> asynchronous linkwatch event.
> This linkwatch event eventually may fail to access chip registers in
> the ndo_get_stats/ndo_get_stats64 callback because the device isn't
> accessible any longer, see call trace in [0].
> 
> To prevent this scenario don't check for IFF_UP only, but also make
> sure that the netdevice is present.
> 
> [0] https://lists.openwall.net/netdev/2018/03/15/62
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks Heiner!
-- 
Florian

[PATCH net-next 2/2] net: phy: call state machine synchronously in phy_stop

[Heiner Kallweit]

phy_stop() may be called e.g. when suspending, therefore all needed
actions should be performed synchronously. Therefore add a synchronous
call to the state machine.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c78203b25..f5bb6a7a8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -861,6 +861,8 @@ void phy_stop(struct phy_device *phydev)
 out_unlock:
 	mutex_unlock(&phydev->lock);
 
+	phy_state_machine(&phydev->state_queue.work);
+
 	/* Cannot call flush_scheduled_work() here as desired because
 	 * of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
 	 * will not reenable interrupts.
-- 
2.19.0

Re: [PATCH net-next 2/2] net: phy: call state machine synchronously in phy_stop

[Geert Uytterhoeven]

On Tue, Sep 18, 2018 at 9:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> phy_stop() may be called e.g. when suspending, therefore all needed
> actions should be performed synchronously. Therefore add a synchronous
> call to the state machine.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Survived 100 suspend/resume cycles on sh73a0/kzm9g and r8a73a4/ape6evm.

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

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

Re: [PATCH net-next 2/2] net: phy: call state machine synchronously in phy_stop

[Florian Fainelli]

On 09/18/2018 12:56 PM, Heiner Kallweit wrote:
> phy_stop() may be called e.g. when suspending, therefore all needed
> actions should be performed synchronously. Therefore add a synchronous
> call to the state machine.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Yes!
-- 
Florian

Re: [PATCH net-next 0/2] net: phy: make phy_stop() synchronous

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Tue, 18 Sep 2018 21:54:23 +0200

> There have been few not that successful attempts in the past to make
> phy_stop() a synchronous call instead of just changing the state.
> Patch 1 of this series addresses an issue which prevented this change.
> At least for me it works fine now. Would appreciate if Geert could
> re-test as well that suspend doesn't throw an error.

Series applied, thank you.