From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751696AbdF2CgD (ORCPT ); Wed, 28 Jun 2017 22:36:03 -0400 Received: from szxga01-in.huawei.com ([45.249.212.187]:9248 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751541AbdF2Cfz (ORCPT ); Wed, 28 Jun 2017 22:35:55 -0400 Subject: Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback To: Andrew Lunn CC: , , , , , , , , , , , , , References: <1498443039-134503-1-git-send-email-linyunsheng@huawei.com> <1498443039-134503-3-git-send-email-linyunsheng@huawei.com> <20170626134235.GC2623@lunn.ch> <17132762-9b94-bc32-fee8-e90a6db5762a@huawei.com> <20170627132958.GA9921@lunn.ch> <3d382bc6-f7b6-9df3-8bb0-fee55b72ac74@huawei.com> <20170628202819.GA22815@lunn.ch> From: Yunsheng Lin Message-ID: <1bff07ed-d423-dfa2-61e3-3f35c4536632@huawei.com> Date: Thu, 29 Jun 2017 10:35:25 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20170628202819.GA22815@lunn.ch> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.74.190.125] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090203.59546778.0025,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 919f0c9548e18d869e458315ac66c172 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Andrew On 2017/6/29 4:28, Andrew Lunn wrote: >>> >From your description, it sounds like you can call phy_resume() on a >>> device which is not suspended. >> Do you mean after calling dev_close, the device is still not suspended? > > You only call dev_close() if the device is running. What if somebody > runs the self test on an interface when it has never been opened? It > looks like you will call phy_resume(). But since it has never been > suspended, you could be in trouble. Here is what I can think of: 1. when the mac driver is first loaded, the phy has a default state. suspended? 2. If user runs the self test after using 'ifconfig ethX down', then I suppose phy is already suspended. Also I don't quite understand what do you mean by in trouble. Right now in phy core, phy_resume return ok even the phy is not suspended. Best Regards Yunsheng Lin >> >> In general, suspend is expected to >>> store away state which will be lost when powering down a >>> device. Resume writes that state back into the device after it is >>> powered up. So resuming a device which was never suspended could write >>> bad state into it. >> >> Do you mean phydev->suspended has bad state? > > phy_resume() current does not check the phydev->suspended state. > >>> Also, what about if WOL has been set before closing the device? >> >> phy_suspend will return errro. >> >> int phy_suspend(struct phy_device *phydev) >> { >> struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver); >> struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; >> int ret = 0; >> >> /* If the device has WOL enabled, we cannot suspend the PHY */ >> phy_ethtool_get_wol(phydev, &wol); >> if (wol.wolopts) >> return -EBUSY; >> >> if (phydev->drv && phydrv->suspend) >> ret = phydrv->suspend(phydev); >> >> if (ret) >> return ret; >> >> phydev->suspended = true; >> >> return ret; >> } > > Which means when you call phy_resume() in lb_setup() you are again > resuming a device which is not suspended... > > Andrew > > . >