* [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend
@ 2017-09-06 10:51 Nisar.Sayed
2017-09-06 13:59 ` Andrew Lunn
0 siblings, 1 reply; 3+ messages in thread
From: Nisar.Sayed @ 2017-09-06 10:51 UTC (permalink / raw)
To: davem, UNGLinuxDriver; +Cc: netdev
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend
2017-09-06 10:51 [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend Nisar.Sayed
@ 2017-09-06 13:59 ` Andrew Lunn
2017-09-06 17:34 ` Nisar.Sayed
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Lunn @ 2017-09-06 13:59 UTC (permalink / raw)
To: Nisar.Sayed; +Cc: davem, UNGLinuxDriver, netdev
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend
2017-09-06 13:59 ` Andrew Lunn
@ 2017-09-06 17:34 ` Nisar.Sayed
0 siblings, 0 replies; 3+ messages in thread
From: Nisar.Sayed @ 2017-09-06 17:34 UTC (permalink / raw)
To: andrew; +Cc: davem, UNGLinuxDriver, netdev
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-06 17:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 10:51 [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend Nisar.Sayed
2017-09-06 13:59 ` Andrew Lunn
2017-09-06 17:34 ` Nisar.Sayed
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).