diff mbox series

lan78xx: Connect phy early

Message ID 20180314145456.69029-1-agraf@suse.de
State Accepted
Commit 92571a1aae40d291158d16e7142637908220f470
Headers show
Series lan78xx: Connect phy early | expand

Commit Message

Alexander Graf March 14, 2018, 2:54 p.m. UTC 
When using wicked with a lan78xx device attached to the system, we
end up with ethtool commands issued on the device before an ifup
got issued. That lead to the following crash:

    Unable to handle kernel NULL pointer dereference at virtual address 0000039c
    pgd = ffff800035b30000
    [0000039c] *pgd=0000000000000000
    Internal error: Oops: 96000004 [#1] SMP
    Modules linked in: [...]
    Supported: Yes
    CPU: 3 PID: 638 Comm: wickedd Tainted: G            E      4.12.14-0-default #1
    Hardware name: raspberrypi rpi/rpi, BIOS 2018.03-rc2 02/21/2018
    task: ffff800035e74180 task.stack: ffff800036718000
    PC is at phy_ethtool_ksettings_get+0x20/0x98
    LR is at lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]
    pc : [<ffff0000086f7f30>] lr : [<ffff000000dcca84>] pstate: 20000005
    sp : ffff80003671bb20
    x29: ffff80003671bb20 x28: ffff800035e74180
    x27: ffff000008912000 x26: 000000000000001d
    x25: 0000000000000124 x24: ffff000008f74d00
    x23: 0000004000114809 x22: 0000000000000000
    x21: ffff80003671bbd0 x20: 0000000000000000
    x19: ffff80003671bbd0 x18: 000000000000040d
    x17: 0000000000000001 x16: 0000000000000000
    x15: 0000000000000000 x14: ffffffffffffffff
    x13: 0000000000000000 x12: 0000000000000020
    x11: 0101010101010101 x10: fefefefefefefeff
    x9 : 7f7f7f7f7f7f7f7f x8 : fefefeff31677364
    x7 : 0000000080808080 x6 : ffff80003671bc9c
    x5 : ffff80003671b9f8 x4 : ffff80002c296190
    x3 : 0000000000000000 x2 : 0000000000000000
    x1 : ffff80003671bbd0 x0 : ffff80003671bc00
    Process wickedd (pid: 638, stack limit = 0xffff800036718000)
    Call trace:
    Exception stack(0xffff80003671b9e0 to 0xffff80003671bb20)
    b9e0: ffff80003671bc00 ffff80003671bbd0 0000000000000000 0000000000000000
    ba00: ffff80002c296190 ffff80003671b9f8 ffff80003671bc9c 0000000080808080
    ba20: fefefeff31677364 7f7f7f7f7f7f7f7f fefefefefefefeff 0101010101010101
    ba40: 0000000000000020 0000000000000000 ffffffffffffffff 0000000000000000
    ba60: 0000000000000000 0000000000000001 000000000000040d ffff80003671bbd0
    ba80: 0000000000000000 ffff80003671bbd0 0000000000000000 0000004000114809
    baa0: ffff000008f74d00 0000000000000124 000000000000001d ffff000008912000
    bac0: ffff800035e74180 ffff80003671bb20 ffff000000dcca84 ffff80003671bb20
    bae0: ffff0000086f7f30 0000000020000005 ffff80002c296000 ffff800035223900
    bb00: 0000ffffffffffff 0000000000000000 ffff80003671bb20 ffff0000086f7f30
    [<ffff0000086f7f30>] phy_ethtool_ksettings_get+0x20/0x98
    [<ffff000000dcca84>] lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]
    [<ffff0000087cbc40>] ethtool_get_settings+0x68/0x210
    [<ffff0000087cc0d4>] dev_ethtool+0x214/0x2180
    [<ffff0000087e5008>] dev_ioctl+0x400/0x630
    [<ffff00000879dd00>] sock_do_ioctl+0x70/0x88
    [<ffff00000879f5f8>] sock_ioctl+0x208/0x368
    [<ffff0000082cde10>] do_vfs_ioctl+0xb0/0x848
    [<ffff0000082ce634>] SyS_ioctl+0x8c/0xa8
    Exception stack(0xffff80003671bec0 to 0xffff80003671c000)
    bec0: 0000000000000009 0000000000008946 0000fffff4e841d0 0000aa0032687465
    bee0: 0000aaaafa2319d4 0000fffff4e841d4 0000000032687465 0000000032687465
    bf00: 000000000000001d 7f7fff7f7f7f7f7f 72606b622e71ff4c 7f7f7f7f7f7f7f7f
    bf20: 0101010101010101 0000000000000020 ffffffffffffffff 0000ffff7f510c68
    bf40: 0000ffff7f6a9d18 0000ffff7f44ce30 000000000000040d 0000ffff7f6f98f0
    bf60: 0000fffff4e842c0 0000000000000001 0000aaaafa2c2e00 0000ffff7f6ab000
    bf80: 0000fffff4e842c0 0000ffff7f62a000 0000aaaafa2b9f20 0000aaaafa2c2e00
    bfa0: 0000fffff4e84818 0000fffff4e841a0 0000ffff7f5ad0cc 0000fffff4e841a0
    bfc0: 0000ffff7f44ce3c 0000000080000000 0000000000000009 000000000000001d
    bfe0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000

The culprit is quite simple: The driver tries to access the phy left and right,
but only actually has a working reference to it when the device is up.

The fix thus is quite simple too: Get a reference to the phy on probe already
and keep it even when the device is going down.

With this patch applied, I can successfully run wicked on my system and bring
the interface up and down as many times as I want, without getting NULL pointer
dereferences in between.

Signed-off-by: Alexander Graf <agraf@suse.de>

---
 drivers/net/usb/lan78xx.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

-- 
2.12.3

Comments

woojung.huh@microchip.com March 14, 2018, 3:26 p.m. UTC | #1 
Hi Alexander,

Thanks for patch. We will look into it if there is any corner case
Such as plug in/out while operations.

Woojung

> -----Original Message-----

> From: Alexander Graf [mailto:agraf@suse.de]

> Sent: Wednesday, March 14, 2018 10:55 AM

> To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>

> Cc: UNGLinuxDriver <UNGLinuxDriver@microchip.com>; netdev@vger.kernel.org; linux-

> usb@vger.kernel.org; linux-kernel@vger.kernel.org; Thomas Bogendoerfer

> <tbogendoerfer@suse.de>; Phil Elwell <phil@raspberrypi.org>

> Subject: [PATCH] lan78xx: Connect phy early

> 

> When using wicked with a lan78xx device attached to the system, we

> end up with ethtool commands issued on the device before an ifup

> got issued. That lead to the following crash:

> 

>     Unable to handle kernel NULL pointer dereference at virtual address 0000039c

>     pgd = ffff800035b30000

>     [0000039c] *pgd=0000000000000000

>     Internal error: Oops: 96000004 [#1] SMP

>     Modules linked in: [...]

>     Supported: Yes

>     CPU: 3 PID: 638 Comm: wickedd Tainted: G            E      4.12.14-0-default #1

>     Hardware name: raspberrypi rpi/rpi, BIOS 2018.03-rc2 02/21/2018

>     task: ffff800035e74180 task.stack: ffff800036718000

>     PC is at phy_ethtool_ksettings_get+0x20/0x98

>     LR is at lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]

>     pc : [<ffff0000086f7f30>] lr : [<ffff000000dcca84>] pstate: 20000005

>     sp : ffff80003671bb20

>     x29: ffff80003671bb20 x28: ffff800035e74180

>     x27: ffff000008912000 x26: 000000000000001d

>     x25: 0000000000000124 x24: ffff000008f74d00

>     x23: 0000004000114809 x22: 0000000000000000

>     x21: ffff80003671bbd0 x20: 0000000000000000

>     x19: ffff80003671bbd0 x18: 000000000000040d

>     x17: 0000000000000001 x16: 0000000000000000

>     x15: 0000000000000000 x14: ffffffffffffffff

>     x13: 0000000000000000 x12: 0000000000000020

>     x11: 0101010101010101 x10: fefefefefefefeff

>     x9 : 7f7f7f7f7f7f7f7f x8 : fefefeff31677364

>     x7 : 0000000080808080 x6 : ffff80003671bc9c

>     x5 : ffff80003671b9f8 x4 : ffff80002c296190

>     x3 : 0000000000000000 x2 : 0000000000000000

>     x1 : ffff80003671bbd0 x0 : ffff80003671bc00

>     Process wickedd (pid: 638, stack limit = 0xffff800036718000)

>     Call trace:

>     Exception stack(0xffff80003671b9e0 to 0xffff80003671bb20)

>     b9e0: ffff80003671bc00 ffff80003671bbd0 0000000000000000 0000000000000000

>     ba00: ffff80002c296190 ffff80003671b9f8 ffff80003671bc9c 0000000080808080

>     ba20: fefefeff31677364 7f7f7f7f7f7f7f7f fefefefefefefeff 0101010101010101

>     ba40: 0000000000000020 0000000000000000 ffffffffffffffff 0000000000000000

>     ba60: 0000000000000000 0000000000000001 000000000000040d ffff80003671bbd0

>     ba80: 0000000000000000 ffff80003671bbd0 0000000000000000 0000004000114809

>     baa0: ffff000008f74d00 0000000000000124 000000000000001d ffff000008912000

>     bac0: ffff800035e74180 ffff80003671bb20 ffff000000dcca84 ffff80003671bb20

>     bae0: ffff0000086f7f30 0000000020000005 ffff80002c296000 ffff800035223900

>     bb00: 0000ffffffffffff 0000000000000000 ffff80003671bb20 ffff0000086f7f30

>     [<ffff0000086f7f30>] phy_ethtool_ksettings_get+0x20/0x98

>     [<ffff000000dcca84>] lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]

>     [<ffff0000087cbc40>] ethtool_get_settings+0x68/0x210

>     [<ffff0000087cc0d4>] dev_ethtool+0x214/0x2180

>     [<ffff0000087e5008>] dev_ioctl+0x400/0x630

>     [<ffff00000879dd00>] sock_do_ioctl+0x70/0x88

>     [<ffff00000879f5f8>] sock_ioctl+0x208/0x368

>     [<ffff0000082cde10>] do_vfs_ioctl+0xb0/0x848

>     [<ffff0000082ce634>] SyS_ioctl+0x8c/0xa8

>     Exception stack(0xffff80003671bec0 to 0xffff80003671c000)

>     bec0: 0000000000000009 0000000000008946 0000fffff4e841d0 0000aa0032687465

>     bee0: 0000aaaafa2319d4 0000fffff4e841d4 0000000032687465 0000000032687465

>     bf00: 000000000000001d 7f7fff7f7f7f7f7f 72606b622e71ff4c 7f7f7f7f7f7f7f7f

>     bf20: 0101010101010101 0000000000000020 ffffffffffffffff 0000ffff7f510c68

>     bf40: 0000ffff7f6a9d18 0000ffff7f44ce30 000000000000040d 0000ffff7f6f98f0

>     bf60: 0000fffff4e842c0 0000000000000001 0000aaaafa2c2e00 0000ffff7f6ab000

>     bf80: 0000fffff4e842c0 0000ffff7f62a000 0000aaaafa2b9f20 0000aaaafa2c2e00

>     bfa0: 0000fffff4e84818 0000fffff4e841a0 0000ffff7f5ad0cc 0000fffff4e841a0

>     bfc0: 0000ffff7f44ce3c 0000000080000000 0000000000000009 000000000000001d

>     bfe0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000

> 

> The culprit is quite simple: The driver tries to access the phy left and right,

> but only actually has a working reference to it when the device is up.

> 

> The fix thus is quite simple too: Get a reference to the phy on probe already

> and keep it even when the device is going down.

> 

> With this patch applied, I can successfully run wicked on my system and bring

> the interface up and down as many times as I want, without getting NULL pointer

> dereferences in between.

> 

> Signed-off-by: Alexander Graf <agraf@suse.de>

> ---

>  drivers/net/usb/lan78xx.c | 21 ++++++++++-----------

>  1 file changed, 10 insertions(+), 11 deletions(-)

> 

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c

> index 60a604cc7647..931cc124ab0c 100644

> --- a/drivers/net/usb/lan78xx.c

> +++ b/drivers/net/usb/lan78xx.c

> @@ -2082,8 +2082,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)

> 

>  	dev->fc_autoneg = phydev->autoneg;

> 

> -	phy_start(phydev);

> -

>  	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");

> 

>  	return 0;

> @@ -2512,9 +2510,7 @@ static int lan78xx_open(struct net_device *net)

>  	if (ret < 0)

>  		goto done;

> 

> -	ret = lan78xx_phy_init(dev);

> -	if (ret < 0)

> -		goto done;

> +	phy_start(net->phydev);

> 

>  	/* for Link Check */

>  	if (dev->urb_intr) {

> @@ -2575,13 +2571,7 @@ static int lan78xx_stop(struct net_device *net)

>  	if (timer_pending(&dev->stat_monitor))

>  		del_timer_sync(&dev->stat_monitor);

> 

> -	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);

> -	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);

> -

>  	phy_stop(net->phydev);

> -	phy_disconnect(net->phydev);

> -

> -	net->phydev = NULL;

> 

>  	clear_bit(EVENT_DEV_OPEN, &dev->flags);

>  	netif_stop_queue(net);

> @@ -3481,6 +3471,11 @@ static void lan78xx_disconnect(struct usb_interface *intf)

>  	net = dev->net;

>  	unregister_netdev(net);

> 

> +	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);

> +	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);

> +

> +	phy_disconnect(net->phydev);

> +

>  	cancel_delayed_work_sync(&dev->wq);

> 

>  	usb_scuttle_anchored_urbs(&dev->deferred);

> @@ -3634,6 +3629,10 @@ static int lan78xx_probe(struct usb_interface *intf,

>  	pm_runtime_set_autosuspend_delay(&udev->dev,

>  					 DEFAULT_AUTOSUSPEND_DELAY);

> 

> +	ret = lan78xx_phy_init(dev);

> +	if (ret < 0)

> +		return ret;

> +

>  	return 0;

> 

>  out3:

> --

> 2.12.3
Andrew Lunn March 14, 2018, 11:25 p.m. UTC | #2 
> @@ -2082,8 +2082,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)

>  

>  	dev->fc_autoneg = phydev->autoneg;

>  

> -	phy_start(phydev);

> -

>  	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");

>  

>  	return 0;

> @@ -2512,9 +2510,7 @@ static int lan78xx_open(struct net_device *net)

>  	if (ret < 0)

>  		goto done;

>  

> -	ret = lan78xx_phy_init(dev);

> -	if (ret < 0)

> -		goto done;

> +	phy_start(net->phydev);


Should the debug message be moved as well?

       Andrew
Nisar.Sayed@microchip.com March 15, 2018, 7:36 a.m. UTC | #3 
Hi Alexander,

Thanks for the patch.

> @@ -2575,13 +2571,7 @@ static int lan78xx_stop(struct net_device *net)

>  	if (timer_pending(&dev->stat_monitor))

>  		del_timer_sync(&dev->stat_monitor);

> 

> -	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);

> -	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);

> -

>  	phy_stop(net->phydev);

> -	phy_disconnect(net->phydev);

> -

> -	net->phydev = NULL;

> 

>  	clear_bit(EVENT_DEV_OPEN, &dev->flags);

>  	netif_stop_queue(net);


Please do add valid "phydev"  check before phy_stop, since "phy_disconnect" should be called before "unregister_netdev"

+ if (net->phydev)
	phy_stop(net->phydev);

> @@ -3481,6 +3471,11 @@ static void lan78xx_disconnect(struct

> usb_interface *intf)

>  	net = dev->net;

>  	unregister_netdev(net);

> 

> +	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);

> +	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);

> +

> +	phy_disconnect(net->phydev);

> +

>  	cancel_delayed_work_sync(&dev->wq);

> 

>  	usb_scuttle_anchored_urbs(&dev->deferred);


Please move "unregister_netdev" after "phy_disconnect", otherwise "phy_disconnect" will fail while we disconnect USB.

> @@ -3634,6 +3629,10 @@ static int lan78xx_probe(struct usb_interface

> *intf,

>  	pm_runtime_set_autosuspend_delay(&udev->dev,

>  					 DEFAULT_AUTOSUSPEND_DELAY);

> 

> +	ret = lan78xx_phy_init(dev);

> +	if (ret < 0)

> +		return ret;

> +

>  	return 0;

> 

>  out3:

> --

> 2.12.3


We should "goto out4" instead of "return" upon "lan78xx_phy_init" fail

+ out4:
+	unregister_netdev(netdev);

In addition to current changes, you might have to take care of the following.

In function "lan78xx_reset_resume"

-  lan78xx_phy_init(dev);
+ phy_start(dev->net->phydev);

Thanks,
Sd.Nisar
diff mbox series

Patch

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 60a604cc7647..931cc124ab0c 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2082,8 +2082,6 @@  static int lan78xx_phy_init(struct lan78xx_net *dev)
 
 	dev->fc_autoneg = phydev->autoneg;
 
-	phy_start(phydev);
-
 	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
 
 	return 0;
@@ -2512,9 +2510,7 @@  static int lan78xx_open(struct net_device *net)
 	if (ret < 0)
 		goto done;
 
-	ret = lan78xx_phy_init(dev);
-	if (ret < 0)
-		goto done;
+	phy_start(net->phydev);
 
 	/* for Link Check */
 	if (dev->urb_intr) {
@@ -2575,13 +2571,7 @@  static int lan78xx_stop(struct net_device *net)
 	if (timer_pending(&dev->stat_monitor))
 		del_timer_sync(&dev->stat_monitor);
 
-	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
-	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
-
 	phy_stop(net->phydev);
-	phy_disconnect(net->phydev);
-
-	net->phydev = NULL;
 
 	clear_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_stop_queue(net);
@@ -3481,6 +3471,11 @@  static void lan78xx_disconnect(struct usb_interface *intf)
 	net = dev->net;
 	unregister_netdev(net);
 
+	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
+	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
+
+	phy_disconnect(net->phydev);
+
 	cancel_delayed_work_sync(&dev->wq);
 
 	usb_scuttle_anchored_urbs(&dev->deferred);
@@ -3634,6 +3629,10 @@  static int lan78xx_probe(struct usb_interface *intf,
 	pm_runtime_set_autosuspend_delay(&udev->dev,
 					 DEFAULT_AUTOSUSPEND_DELAY);
 
+	ret = lan78xx_phy_init(dev);
+	if (ret < 0)
+		return ret;
+
 	return 0;
 
 out3: