[{"id":1764122,"web_url":"http://patchwork.ozlabs.org/comment/1764122/","msgid":"<20170906135908.GB11864@lunn.ch>","list_archive_url":null,"date":"2017-09-06T13:59:08","subject":"Re: [PATCH net 1/4] lan78xx: Fix for crash associated with System\n\tsuspend","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"On Wed, Sep 06, 2017 at 10:51:31AM +0000, Nisar.Sayed@microchip.com wrote:\n> From: Nisar Sayed \n> \n> Fix for crash associated with System suspend\n> \n> Since ndo_stop removes phydev which makes phydev NULL.\n> Whenever system suspend is initiated or after \"ifconfig down\",\n> if set_wol or get_wol is triggered phydev is NULL leads system crash.\n> Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues\n> instead of adding/removing phydevice\n\nLooking at this patch, there apears to be lots of different things\ngoing on. Please can you split it up into multiple patches.\n\n> Signed-off-by: Nisar Sayed \n> ---\n> drivers/net/usb/lan78xx.c | 44 ++++++++++++++++++++++++++++----------------\n> 1 file changed, 28 insertions(+), 16 deletions(-)\n> \n> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c\n> index b99a7fb..955ab3b 100644\n> --- a/drivers/net/usb/lan78xx.c\n> +++ b/drivers/net/usb/lan78xx.c\n> @@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)\n> \t\t\t\t\t\t lan8835_fixup);\n> \t\tif (ret < 0) {\n> \t\t\tnetdev_err(dev->net, \"fail to register fixup\\n\");\n> +\t\t\tphy_unregister_fixup_for_uid(PHY_KSZ9031RNX,\n> +\t\t\t\t\t\t 0xfffffff0);\n\ngoto error; would be better. phy_unregister_fixup_for_uid() does not care if you try to unregister\nsomething which has not been registered.\n\nAlso, this should be a separate patch. \n\n> \t\t\treturn ret;\n> \t\t}\n> \t\t/* add more external PHY fixup here if needed */\n> @@ -2031,8 +2033,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)\n> \t\tphydev->is_internal = false;\n> \t} else {\n> \t\tnetdev_err(dev->net, \"unknown ID found\\n\");\n> -\t\tret = -EIO;\n> -\t\tgoto error;\n> +\t\treturn -EIO;\n> \t}\n> \n> \t/* if phyirq is not set, use polling mode in phylib */\n> @@ -2051,7 +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)\n> \tif (ret) {\n> \t\tnetdev_err(dev->net, \"can't attach PHY to %s\\n\",\n> \t\t\t dev->mdiobus->id);\n> -\t\treturn -EIO;\n> +\t\tret = -EIO;\n> +\t\tif (dev->chipid == ID_REV_CHIP_ID_7801_)\n> +\t\t\tgoto error;\n> +\t\treturn ret;\n\nWhy not add the if (dev->chipid == ID_REV_CHIP_ID_7801_) after the\nerror: label?\n\n\n\n> \t}\n> \n> \t/* MAC doesn't support 1000T Half */\n> @@ -2067,8 +2071,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)\n> \n> \tdev->fc_autoneg = phydev->autoneg;\n> \n> -\tphy_start(phydev);\n> -\n> \tnetif_dbg(dev, ifup, dev->net, \"phy initialised successfully\");\n> \n> \treturn 0;\n> @@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)\n> \tif (ret < 0)\n> \t\tgoto done;\n> \n> -\tret = lan78xx_phy_init(dev);\n> -\tif (ret < 0)\n> -\t\tgoto done;\n> +\tif (dev->domain_data.phyirq > 0)\n> +\t\tphy_start_interrupts(dev->net->phydev);\n\nThis is unusual. I don't see any other MAC driver starting interrupts.\nThis needs explaining.\n\n Andrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnQDm6c7qz9s9Y\n\tfor ;\n\tWed, 6 Sep 2017 23:59:20 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932485AbdIFN7S (ORCPT );\n\tWed, 6 Sep 2017 09:59:18 -0400","from vps0.lunn.ch ([178.209.37.122]:58678 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S932209AbdIFN7Q (ORCPT );\n\tWed, 6 Sep 2017 09:59:16 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dparE-0003aX-Jm; Wed, 06 Sep 2017 15:59:08 +0200"],"Date":"Wed, 6 Sep 2017 15:59:08 +0200","From":"Andrew Lunn ","To":"Nisar.Sayed@microchip.com","Cc":"davem@davemloft.net, UNGLinuxDriver@microchip.com, netdev@vger.kernel.org","Subject":"Re: [PATCH net 1/4] lan78xx: Fix for crash associated with System\n\tsuspend","Message-ID":"<20170906135908.GB11864@lunn.ch>","References":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1764277,"web_url":"http://patchwork.ozlabs.org/comment/1764277/","msgid":"","list_archive_url":null,"date":"2017-09-06T17:34:48","subject":"RE: [PATCH net 1/4] lan78xx: Fix for crash associated with System\n\tsuspend","submitter":{"id":71648,"url":"http://patchwork.ozlabs.org/api/people/71648/","name":"Nisar Sayed","email":"Nisar.Sayed@microchip.com"},"content":"Thanks Andrew inputs.\n \n> On Wed, Sep 06, 2017 at 10:51:31AM +0000, Nisar.Sayed@microchip.com\n> wrote:\n> > From: Nisar Sayed \n> >\n> > Fix for crash associated with System suspend\n> >\n> > Since ndo_stop removes phydev which makes phydev NULL.\n> > Whenever system suspend is initiated or after \"ifconfig \n> > down\", if set_wol or get_wol is triggered phydev is NULL leads system\n> crash.\n> > Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues\n> > instead of adding/removing phydevice\n> \n> Looking at this patch, there apears to be lots of different things going on.\n> Please can you split it up into multiple patches.\n\nSure will split it up.\n\n> \n> > Signed-off-by: Nisar Sayed \n> > ---\n> > drivers/net/usb/lan78xx.c | 44\n> > ++++++++++++++++++++++++++++----------------\n> > 1 file changed, 28 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c\n> > index b99a7fb..955ab3b 100644\n> > --- a/drivers/net/usb/lan78xx.c\n> > +++ b/drivers/net/usb/lan78xx.c\n> > @@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net\n> *dev)\n> > \t\t\t\t\t\t lan8835_fixup);\n> > \t\tif (ret < 0) {\n> > \t\t\tnetdev_err(dev->net, \"fail to register fixup\\n\");\n> > +\t\t\tphy_unregister_fixup_for_uid(PHY_KSZ9031RNX,\n> > +\t\t\t\t\t\t 0xfffffff0);\n> \n> goto error; would be better. phy_unregister_fixup_for_uid() does not care if\n> you try to unregister something which has not been registered.\n> \n> Also, this should be a separate patch.\n\nOk, will make it as separate patch\n\n> \n> > \t\t\treturn ret;\n> > \t\t}\n> > \t\t/* add more external PHY fixup here if needed */ @@ -\n> 2031,8 +2033,7\n> > @@ static int lan78xx_phy_init(struct lan78xx_net *dev)\n> > \t\tphydev->is_internal = false;\n> > \t} else {\n> > \t\tnetdev_err(dev->net, \"unknown ID found\\n\");\n> > -\t\tret = -EIO;\n> > -\t\tgoto error;\n> > +\t\treturn -EIO;\n> > \t}\n> >\n> > \t/* if phyirq is not set, use polling mode in phylib */ @@ -2051,7\n> > +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)\n> > \tif (ret) {\n> > \t\tnetdev_err(dev->net, \"can't attach PHY to %s\\n\",\n> > \t\t\t dev->mdiobus->id);\n> > -\t\treturn -EIO;\n> > +\t\tret = -EIO;\n> > +\t\tif (dev->chipid == ID_REV_CHIP_ID_7801_)\n> > +\t\t\tgoto error;\n> > +\t\treturn ret;\n> \n> Why not add the if (dev->chipid == ID_REV_CHIP_ID_7801_) after the\n> error: label?\n> \n> \n\n\nYes, will correct it\n\n> \n> > \t}\n> >\n> > \t/* MAC doesn't support 1000T Half */ @@ -2067,8 +2071,6 @@ static\n> > int lan78xx_phy_init(struct lan78xx_net *dev)\n> >\n> > \tdev->fc_autoneg = phydev->autoneg;\n> >\n> > -\tphy_start(phydev);\n> > -\n> > \tnetif_dbg(dev, ifup, dev->net, \"phy initialised successfully\");\n> >\n> > \treturn 0;\n> > @@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)\n> > \tif (ret < 0)\n> > \t\tgoto done;\n> >\n> > -\tret = lan78xx_phy_init(dev);\n> > -\tif (ret < 0)\n> > -\t\tgoto done;\n> > +\tif (dev->domain_data.phyirq > 0)\n> > +\t\tphy_start_interrupts(dev->net->phydev);\n> \n> This is unusual. I don't see any other MAC driver starting interrupts.\n> This needs explaining.\n> \n> Andrew\n\nSince \"lan78xx_open\" calls \"lan78xx_reset\" (Device reset) it is required to start/enable interrupt back\nInitially when \"phydev->state = PHY_READY\" state \"phy_start\" will not enable interrupts,\nHowever after \"lan78xx_stop\" when \"phy_stop\" makes \"phydev->state = PHY_HALTED\"\nSubsequent call to \"phy_start\" will enable interrupt.\n\nHence \"phy_start_interrupts\" used after \"lan78xx_reset\"\n\n- Nisar","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnW1v540gz9t2d\n\tfor ;\n\tThu, 7 Sep 2017 03:35:15 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750978AbdIFRfH convert rfc822-to-8bit (ORCPT\n\t);\n\tWed, 6 Sep 2017 13:35:07 -0400","from esa5.microchip.iphmx.com ([216.71.150.166]:20200 \"EHLO\n\tesa5.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750789AbdIFRfG (ORCPT\n\t); Wed, 6 Sep 2017 13:35:06 -0400","from smtpout.microchip.com (HELO email.microchip.com)\n\t([198.175.253.82])\n\tby esa5.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA;\n\t06 Sep 2017 10:34:51 -0700","from CHN-SV-EXMX03.mchp-main.com ([fe80::58f5:b949:4b1:3df4]) by\n\tCHN-SV-EXCH06.mchp-main.com ([fe80::5404:4dc9:559:e436%16]) with mapi\n\tid 14.03.0352.000; Wed, 6 Sep 2017 10:34:49 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,354,1500966000\"; d=\"scan'208\";a=\"4472128\"","From":"","To":"","CC":", ,\n\t","Subject":"RE: [PATCH net 1/4] lan78xx: Fix for crash associated with System\n\tsuspend","Thread-Topic":"[PATCH net 1/4] lan78xx: Fix for crash associated with System\n\tsuspend","Thread-Index":"AdMm/FT/4xwnv4GdSm2C6fB/QqdHjgAVqZAAAAgzL1A=","Date":"Wed, 6 Sep 2017 17:34:48 +0000","Message-ID":"","References":"\n\t<20170906135908.GB11864@lunn.ch>","In-Reply-To":"<20170906135908.GB11864@lunn.ch>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.215.90]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]