RE: [PATCH] lan78xx: Connect phy early

[Nisar.Sayed]

Hi Alexander,

Thanks for the patch.

> @@ -2575,13 +2571,7 @@ static int lan78xx_stop(struct net_device *net)
>   if (timer_pending(>stat_monitor))
>   del_timer_sync(>stat_monitor);
> 
> - phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfff0);
> - phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfff0);
> -
>   phy_stop(net->phydev);
> - phy_disconnect(net->phydev);
> -
> - net->phydev = NULL;
> 
>   clear_bit(EVENT_DEV_OPEN, >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, 0xfff0);
> + phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfff0);
> +
> + phy_disconnect(net->phydev);
> +
>   cancel_delayed_work_sync(>wq);
> 
>   usb_scuttle_anchored_urbs(>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(>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


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] lan78xx: Connect phy early

[Andrew Lunn]

> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] lan78xx: Connect phy early

[Woojung.Huh]

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:ag...@suse.de]
> Sent: Wednesday, March 14, 2018 10:55 AM
> To: Woojung Huh - C21699 
> Cc: UNGLinuxDriver ; net...@vger.kernel.org; 
> linux-
> u...@vger.kernel.org; linux-ker...@vger.kernel.org; Thomas Bogendoerfer
> ; Phil Elwell 
> 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 
> 039c
> pgd = 800035b3
> [039c] *pgd=
> Internal error: Oops: 9604 [#1] SMP
> Modules linked in: [...]
> Supported: Yes
> CPU: 3 PID: 638 Comm: wickedd Tainted: GE  
> 4.12.14-0-default #1
> Hardware name: raspberrypi rpi/rpi, BIOS 2018.03-rc2 02/21/2018
> task: 800035e74180 task.stack: 800036718000
> PC is at phy_ethtool_ksettings_get+0x20/0x98
> LR is at lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]
> pc : [] lr : [] pstate: 2005
> sp : 80003671bb20
> x29: 80003671bb20 x28: 800035e74180
> x27: 08912000 x26: 001d
> x25: 0124 x24: 08f74d00
> x23: 004000114809 x22: 
> x21: 80003671bbd0 x20: 
> x19: 80003671bbd0 x18: 040d
> x17: 0001 x16: 
> x15:  x14: 
> x13:  x12: 0020
> x11: 0101010101010101 x10: fefefefefefefeff
> x9 : 7f7f7f7f7f7f7f7f x8 : fefefeff31677364
> x7 : 80808080 x6 : 80003671bc9c
> x5 : 80003671b9f8 x4 : 80002c296190
> x3 :  x2 : 
> x1 : 80003671bbd0 x0 : 80003671bc00
> Process wickedd (pid: 638, stack limit = 0x800036718000)
> Call trace:
> Exception stack(0x80003671b9e0 to 0x80003671bb20)
> b9e0: 80003671bc00 80003671bbd0  
> ba00: 80002c296190 80003671b9f8 80003671bc9c 80808080
> ba20: fefefeff31677364 7f7f7f7f7f7f7f7f fefefefefefefeff 0101010101010101
> ba40: 0020   
> ba60:  0001 040d 80003671bbd0
> ba80:  80003671bbd0  004000114809
> baa0: 08f74d00 0124 001d 08912000
> bac0: 800035e74180 80003671bb20 00dcca84 80003671bb20
> bae0: 086f7f30 2005 80002c296000 800035223900
> bb00:   80003671bb20 086f7f30
> [] phy_ethtool_ksettings_get+0x20/0x98
> [] lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]
> [] ethtool_get_settings+0x68/0x210
> [] dev_ethtool+0x214/0x2180
> [] dev_ioctl+0x400/0x630
> [] sock_do_ioctl+0x70/0x88
> [] sock_ioctl+0x208/0x368
> [] do_vfs_ioctl+0xb0/0x848
> [] SyS_ioctl+0x8c/0xa8
> Exception stack(0x80003671bec0 to 0x80003671c000)
> bec0: 0009 8946 f4e841d0 aa0032687465
> bee0: fa2319d4 f4e841d4 32687465 32687465
> bf00: 001d 7f7fff7f7f7f7f7f 72606b622e71ff4c 7f7f7f7f7f7f7f7f
> bf20: 0101010101010101 0020  7f510c68
> bf40: 7f6a9d18 7f44ce30 040d 7f6f98f0
> bf60: f4e842c0 0001 fa2c2e00 7f6ab000
> bf80: f4e842c0 7f62a000 fa2b9f20 fa2c2e00
> bfa0: f4e84818 f4e841a0 7f5ad0cc f4e841a0
> bfc0: 7f44ce3c 8000 0009 001d
> bfe0:    
> 
> 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 
> ---
>  drivers/net/usb/lan78xx.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git