| Message ID | CE371C1263339941885964188A0225FA333370@CHN-SV-EXMX03.mchp-main.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | lan78xx: Fixes and Enhancements to lan78xx driver | expand |
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
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
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); }