[PATCH net 1/4] lan78xx: Fix for crash associated with System suspend

[PATCH net 1/4] lan78xx: Fix for crash associated with System suspend

[Nisar.Sayed]

From: Nisar Sayed <Nisar.Sayed@microchip.com>

Fix for crash associated with System suspend

Since ndo_stop removes phydev which makes phydev NULL.
Whenever system suspend is initiated or after "ifconfig <interface> down",
if set_wol or get_wol is triggered phydev is NULL leads system crash.
Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues
instead of adding/removing phydevice

Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
---
 drivers/net/usb/lan78xx.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index b99a7fb..955ab3b 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 						 lan8835_fixup);
 		if (ret < 0) {
 			netdev_err(dev->net, "fail to register fixup\n");
+			phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
+						     0xfffffff0);
 			return ret;
 		}
 		/* add more external PHY fixup here if needed */
@@ -2031,8 +2033,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		phydev->is_internal = false;
 	} else {
 		netdev_err(dev->net, "unknown ID found\n");
-		ret = -EIO;
-		goto error;
+		return -EIO;
 	}
 
 	/* if phyirq is not set, use polling mode in phylib */
@@ -2051,7 +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	if (ret) {
 		netdev_err(dev->net, "can't attach PHY to %s\n",
 			   dev->mdiobus->id);
-		return -EIO;
+		ret = -EIO;
+		if (dev->chipid == ID_REV_CHIP_ID_7801_)
+			goto error;
+		return ret;
 	}
 
 	/* MAC doesn't support 1000T Half */
@@ -2067,8 +2071,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;
@@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)
 	if (ret < 0)
 		goto done;
 
-	ret = lan78xx_phy_init(dev);
-	if (ret < 0)
-		goto done;
+	if (dev->domain_data.phyirq > 0)
+		phy_start_interrupts(dev->net->phydev);
+	phy_start(dev->net->phydev);
 
 	/* for Link Check */
 	if (dev->urb_intr) {
@@ -2560,13 +2562,11 @@ 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;
+	if (net->phydev) {
+		if (dev->domain_data.phyirq > 0)
+			phy_stop_interrupts(net->phydev);
+		phy_stop(net->phydev);
+	}
 
 	clear_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_stop_queue(net);
@@ -3464,6 +3464,12 @@ static void lan78xx_disconnect(struct usb_interface *intf)
 	udev = interface_to_usbdev(intf);
 
 	net = dev->net;
+	if (dev->chipid == ID_REV_CHIP_ID_7801_) {
+		phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
+		phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
+	}
+	phy_disconnect(net->phydev);
+	net->phydev = NULL;
 	unregister_netdev(net);
 
 	cancel_delayed_work_sync(&dev->wq);
@@ -3613,6 +3619,10 @@ static int lan78xx_probe(struct usb_interface *intf,
 		goto out3;
 	}
 
+	ret = lan78xx_phy_init(dev);
+	if (ret < 0)
+		goto out3;
+
 	usb_set_intfdata(intf, dev);
 
 	ret = device_set_wakeup_enable(&udev->dev, true);
@@ -3972,7 +3982,9 @@ static int lan78xx_reset_resume(struct usb_interface *intf)
 
 	lan78xx_reset(dev);
 
-	lan78xx_phy_init(dev);
+	if (dev->domain_data.phyirq > 0)
+		phy_start_interrupts(dev->net->phydev);
+	phy_start(dev->net->phydev);
 
 	return lan78xx_resume(intf);
 }
-- 
1.9.1

Re: [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend

[Andrew Lunn]

On Wed, Sep 06, 2017 at 10:51:31AM +0000, Nisar.Sayed@microchip.com wrote:
> From: Nisar Sayed <Nisar.Sayed@microchip.com>
> 
> Fix for crash associated with System suspend
> 
> Since ndo_stop removes phydev which makes phydev NULL.
> Whenever system suspend is initiated or after "ifconfig <interface> down",
> if set_wol or get_wol is triggered phydev is NULL leads system crash.
> Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues
> instead of adding/removing phydevice

Looking at this patch, there apears to be lots of different things
going on. Please can you split it up into multiple patches.

> Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
> ---
>  drivers/net/usb/lan78xx.c | 44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index b99a7fb..955ab3b 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  						 lan8835_fixup);
>  		if (ret < 0) {
>  			netdev_err(dev->net, "fail to register fixup\n");
> +			phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> +						     0xfffffff0);

goto error; would be better. phy_unregister_fixup_for_uid() does not care if you try to unregister
something which has not been registered.

Also, this should be a separate patch. 

>  			return ret;
>  		}
>  		/* add more external PHY fixup here if needed */
> @@ -2031,8 +2033,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		phydev->is_internal = false;
>  	} else {
>  		netdev_err(dev->net, "unknown ID found\n");
> -		ret = -EIO;
> -		goto error;
> +		return -EIO;
>  	}
>  
>  	/* if phyirq is not set, use polling mode in phylib */
> @@ -2051,7 +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  	if (ret) {
>  		netdev_err(dev->net, "can't attach PHY to %s\n",
>  			   dev->mdiobus->id);
> -		return -EIO;
> +		ret = -EIO;
> +		if (dev->chipid == ID_REV_CHIP_ID_7801_)
> +			goto error;
> +		return ret;

Why not add the if (dev->chipid == ID_REV_CHIP_ID_7801_) after the
error: label?



>  	}
>  
>  	/* MAC doesn't support 1000T Half */
> @@ -2067,8 +2071,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;
> @@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)
>  	if (ret < 0)
>  		goto done;
>  
> -	ret = lan78xx_phy_init(dev);
> -	if (ret < 0)
> -		goto done;
> +	if (dev->domain_data.phyirq > 0)
> +		phy_start_interrupts(dev->net->phydev);

This is unusual. I don't see any other MAC driver starting interrupts.
This needs explaining.

     Andrew

RE: [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend

[Nisar.Sayed]

Thanks Andrew inputs.
 
> On Wed, Sep 06, 2017 at 10:51:31AM +0000, Nisar.Sayed@microchip.com
> wrote:
> > From: Nisar Sayed <Nisar.Sayed@microchip.com>
> >
> > Fix for crash associated with System suspend
> >
> > Since ndo_stop removes phydev which makes phydev NULL.
> > Whenever system suspend is initiated or after "ifconfig <interface>
> > down", if set_wol or get_wol is triggered phydev is NULL leads system
> crash.
> > Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues
> > instead of adding/removing phydevice
> 
> Looking at this patch, there apears to be lots of different things going on.
> Please can you split it up into multiple patches.

Sure will split it up.

> 
> > Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
> > ---
> >  drivers/net/usb/lan78xx.c | 44
> > ++++++++++++++++++++++++++++----------------
> >  1 file changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index b99a7fb..955ab3b 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
> > @@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
> >  						 lan8835_fixup);
> >  		if (ret < 0) {
> >  			netdev_err(dev->net, "fail to register fixup\n");
> > +			phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> > +						     0xfffffff0);
> 
> goto error; would be better. phy_unregister_fixup_for_uid() does not care if
> you try to unregister something which has not been registered.
> 
> Also, this should be a separate patch.

Ok, will make it as separate patch

> 
> >  			return ret;
> >  		}
> >  		/* add more external PHY fixup here if needed */ @@ -
> 2031,8 +2033,7
> > @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> >  		phydev->is_internal = false;
> >  	} else {
> >  		netdev_err(dev->net, "unknown ID found\n");
> > -		ret = -EIO;
> > -		goto error;
> > +		return -EIO;
> >  	}
> >
> >  	/* if phyirq is not set, use polling mode in phylib */ @@ -2051,7
> > +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> >  	if (ret) {
> >  		netdev_err(dev->net, "can't attach PHY to %s\n",
> >  			   dev->mdiobus->id);
> > -		return -EIO;
> > +		ret = -EIO;
> > +		if (dev->chipid == ID_REV_CHIP_ID_7801_)
> > +			goto error;
> > +		return ret;
> 
> Why not add the if (dev->chipid == ID_REV_CHIP_ID_7801_) after the
> error: label?
> 
> 


Yes, will correct it

> 
> >  	}
> >
> >  	/* MAC doesn't support 1000T Half */ @@ -2067,8 +2071,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;
> > @@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)
> >  	if (ret < 0)
> >  		goto done;
> >
> > -	ret = lan78xx_phy_init(dev);
> > -	if (ret < 0)
> > -		goto done;
> > +	if (dev->domain_data.phyirq > 0)
> > +		phy_start_interrupts(dev->net->phydev);
> 
> This is unusual. I don't see any other MAC driver starting interrupts.
> This needs explaining.
> 
>      Andrew

Since "lan78xx_open" calls  "lan78xx_reset" (Device reset) it is required to start/enable interrupt back
Initially when "phydev->state = PHY_READY" state  "phy_start" will not enable interrupts,
However after "lan78xx_stop" when "phy_stop" makes "phydev->state = PHY_HALTED"
Subsequent call to "phy_start" will enable interrupt.

Hence "phy_start_interrupts" used after "lan78xx_reset"

- Nisar