Re: [PATCH] lan78xx: Don't reset the interface on open
From: Phil Elwell Date: Tue, 10 Apr 2018 13:18:25 +0100 > Commit 92571a1aae40 ("lan78xx: Connect phy early") moves the PHY > initialisation into lan78xx_probe, but lan78xx_open subsequently calls > lan78xx_reset. As well as forcing a second round of link negotiation, > this reset frequently prevents the phy interrupt from being generated > (even though the link is up), rendering the interface unusable. > > Fix this issue by removing the lan78xx_reset call from lan78xx_open. > > Fixes: 92571a1aae40 ("lan78xx: Connect phy early") > Signed-off-by: Phil Elwell Applied and queued up for -stable, thanks. -- 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: Don't reset the interface on open
Hi Phil, > Hi Nisar, > > On 10/04/2018 15:16, nisar.sa...@microchip.com wrote: > > Thanks Phil, for identifying the issues. > > > >> - ret = lan78xx_reset(dev); > >> - if (ret < 0) > >> - goto done; > >> - > >>phy_start(net->phydev); > >> > >>netif_dbg(dev, ifup, dev->net, "phy initialised successfully"); > >> -- > > > > You may need to start the interrupts before "phy_start" instead of > suppressing call to "lan78xx_reset". > > > > + if (dev->domain_data.phyirq > 0) > > + phy_start_interrupts(dev->net->phydev); > > Shouldn't phy_connect_direct, called from lan78xx_phy_init, already have > enabled interrupts for us? > > This patch addresses two problems - time wasted by renegotiating the link > after the reset and the missed interrupt - and I'd like both to be fixed. > Unless > you can come up with a good reason for performing the reset from the open > handler I think it should be removed. > > Phil Thanks, we have verified suspected test cases and these are passed, the changes are good to go. - Nisar
Re: [PATCH] lan78xx: Don't reset the interface on open
Hi Nisar, On 10/04/2018 15:16, nisar.sa...@microchip.com wrote: > Thanks Phil, for identifying the issues. > >> -ret = lan78xx_reset(dev); >> -if (ret < 0) >> -goto done; >> - >> phy_start(net->phydev); >> >> netif_dbg(dev, ifup, dev->net, "phy initialised successfully"); >> -- > > You may need to start the interrupts before "phy_start" instead of > suppressing call to "lan78xx_reset". > > + if (dev->domain_data.phyirq > 0) > + phy_start_interrupts(dev->net->phydev); Shouldn't phy_connect_direct, called from lan78xx_phy_init, already have enabled interrupts for us? This patch addresses two problems - time wasted by renegotiating the link after the reset and the missed interrupt - and I'd like both to be fixed. Unless you can come up with a good reason for performing the reset from the open handler I think it should be removed. Phil -- 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: Don't reset the interface on open
Thanks Phil, for identifying the issues. > - ret = lan78xx_reset(dev); > - if (ret < 0) > - goto done; > - > phy_start(net->phydev); > > netif_dbg(dev, ifup, dev->net, "phy initialised successfully"); > -- You may need to start the interrupts before "phy_start" instead of suppressing call to "lan78xx_reset". + if (dev->domain_data.phyirq > 0) + phy_start_interrupts(dev->net->phydev); - 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