Thanks Andrew inputs. > On Wed, Sep 06, 2017 at 10:51:31AM +0000, [email protected] > wrote: > > From: Nisar Sayed <[email protected]> > > > > 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 <[email protected]> > > --- > > 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
