| Message ID | 1498443039-134503-3-git-send-email-linyunsheng@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Jun 26, 2017 at 10:10:39AM +0800, Lin Yun Sheng wrote: > Use function set_loopback in phy_driver to setup phy loopback > when doing ethtool self test. > > Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> > --- > drivers/net/ethernet/hisilicon/hns/hnae.h | 1 + > drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 92 +++++++----------------- > 2 files changed, 26 insertions(+), 67 deletions(-) > > diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.h b/drivers/net/ethernet/hisilicon/hns/hnae.h > index 04211ac..7ba653a 100644 > --- a/drivers/net/ethernet/hisilicon/hns/hnae.h > +++ b/drivers/net/ethernet/hisilicon/hns/hnae.h > @@ -360,6 +360,7 @@ enum hnae_loop { > MAC_INTERNALLOOP_MAC = 0, > MAC_INTERNALLOOP_SERDES, > MAC_INTERNALLOOP_PHY, > + MAC_LOOP_PHY_NONE, > MAC_LOOP_NONE, > }; > > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > index e95795b..10d82df 100644 > --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > @@ -259,67 +259,24 @@ static int hns_nic_set_link_ksettings(struct net_device *net_dev, > > static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) > { > -#define COPPER_CONTROL_REG 0 > -#define PHY_POWER_DOWN BIT(11) > -#define PHY_LOOP_BACK BIT(14) > - u16 val = 0; > - > - if (phy_dev->is_c45) /* c45 branch adding for XGE PHY */ > - return -ENOTSUPP; > + int err; > > if (en) { > - /* speed : 1000M */ > - phy_write(phy_dev, HNS_PHY_PAGE_REG, 2); > - phy_write(phy_dev, 21, 0x1046); > - > - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); > - /* Force Master */ > - phy_write(phy_dev, 9, 0x1F00); > - > - /* Soft-reset */ > - phy_write(phy_dev, 0, 0x9140); > - /* If autoneg disabled,two soft-reset operations */ > - phy_write(phy_dev, 0, 0x9140); > - > - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0xFA); > - > - /* Default is 0x0400 */ > - phy_write(phy_dev, 1, 0x418); > - > - /* Force 1000M Link, Default is 0x0200 */ > - phy_write(phy_dev, 7, 0x20C); > - > - /* Powerup Fiber */ > - phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); > - val = phy_read(phy_dev, COPPER_CONTROL_REG); > - val &= ~PHY_POWER_DOWN; > - phy_write(phy_dev, COPPER_CONTROL_REG, val); > - > - /* Enable Phy Loopback */ > - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); > - val = phy_read(phy_dev, COPPER_CONTROL_REG); > - val |= PHY_LOOP_BACK; > - val &= ~PHY_POWER_DOWN; > - phy_write(phy_dev, COPPER_CONTROL_REG, val); > + err = phy_resume(phy_dev); Maybe this was discussed with an earlier version of these patches. Why are using phy_resume() and phy_suspend()? > static int __lb_setup(struct net_device *ndev, > @@ -332,10 +289,8 @@ static int __lb_setup(struct net_device *ndev, > > switch (loop) { > case MAC_INTERNALLOOP_PHY: > - if ((phy_dev) && (!phy_dev->is_c45)) { > - ret = hns_nic_config_phy_loopback(phy_dev, 0x1); > - ret |= h->dev->ops->set_loopback(h, loop, 0x1); > - } > + ret = hns_nic_config_phy_loopback(phy_dev, 0x1); > + ret |= h->dev->ops->set_loopback(h, loop, 0x1); Or'ing together two errno values does not make much sense: > + if (loop == MAC_INTERNALLOOP_PHY) > + ret = __lb_setup(ndev, MAC_LOOP_PHY_NONE); > + else > + ret = __lb_setup(ndev, MAC_LOOP_NONE); > if (ret) > netdev_err(ndev, "%s: __lb_setup return error(%d)!\n", > __func__, And it looks like you even print the OR'ed version here! Andrew
Hi, Andrew On 2017/6/26 21:42, Andrew Lunn wrote: > On Mon, Jun 26, 2017 at 10:10:39AM +0800, Lin Yun Sheng wrote: >> Use function set_loopback in phy_driver to setup phy loopback >> when doing ethtool self test. >> >> Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> >> --- >> drivers/net/ethernet/hisilicon/hns/hnae.h | 1 + >> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 92 +++++++----------------- >> 2 files changed, 26 insertions(+), 67 deletions(-) >> >> diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.h b/drivers/net/ethernet/hisilicon/hns/hnae.h >> index 04211ac..7ba653a 100644 >> --- a/drivers/net/ethernet/hisilicon/hns/hnae.h >> +++ b/drivers/net/ethernet/hisilicon/hns/hnae.h >> @@ -360,6 +360,7 @@ enum hnae_loop { >> MAC_INTERNALLOOP_MAC = 0, >> MAC_INTERNALLOOP_SERDES, >> MAC_INTERNALLOOP_PHY, >> + MAC_LOOP_PHY_NONE, >> MAC_LOOP_NONE, >> }; >> >> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >> index e95795b..10d82df 100644 >> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >> @@ -259,67 +259,24 @@ static int hns_nic_set_link_ksettings(struct net_device *net_dev, >> >> static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) >> { >> -#define COPPER_CONTROL_REG 0 >> -#define PHY_POWER_DOWN BIT(11) >> -#define PHY_LOOP_BACK BIT(14) >> - u16 val = 0; >> - >> - if (phy_dev->is_c45) /* c45 branch adding for XGE PHY */ >> - return -ENOTSUPP; >> + int err; >> >> if (en) { >> - /* speed : 1000M */ >> - phy_write(phy_dev, HNS_PHY_PAGE_REG, 2); >> - phy_write(phy_dev, 21, 0x1046); >> - >> - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); >> - /* Force Master */ >> - phy_write(phy_dev, 9, 0x1F00); >> - >> - /* Soft-reset */ >> - phy_write(phy_dev, 0, 0x9140); >> - /* If autoneg disabled,two soft-reset operations */ >> - phy_write(phy_dev, 0, 0x9140); >> - >> - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0xFA); >> - >> - /* Default is 0x0400 */ >> - phy_write(phy_dev, 1, 0x418); >> - >> - /* Force 1000M Link, Default is 0x0200 */ >> - phy_write(phy_dev, 7, 0x20C); >> - >> - /* Powerup Fiber */ >> - phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); >> - val = phy_read(phy_dev, COPPER_CONTROL_REG); >> - val &= ~PHY_POWER_DOWN; >> - phy_write(phy_dev, COPPER_CONTROL_REG, val); >> - >> - /* Enable Phy Loopback */ >> - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); >> - val = phy_read(phy_dev, COPPER_CONTROL_REG); >> - val |= PHY_LOOP_BACK; >> - val &= ~PHY_POWER_DOWN; >> - phy_write(phy_dev, COPPER_CONTROL_REG, val); >> + err = phy_resume(phy_dev); > > Maybe this was discussed with an earlier version of these patches. Why > are using phy_resume() and phy_suspend()? When self_test is invoked with ETH_TEST_FL_OFFLINE option, hns mac driver call dev_close to set net dev to offline state if net dev is online. Doing the actual phy loolback test require phy is power up, So phy_resume and phy_suspend are used. > >> static int __lb_setup(struct net_device *ndev, >> @@ -332,10 +289,8 @@ static int __lb_setup(struct net_device *ndev, >> >> switch (loop) { >> case MAC_INTERNALLOOP_PHY: >> - if ((phy_dev) && (!phy_dev->is_c45)) { >> - ret = hns_nic_config_phy_loopback(phy_dev, 0x1); >> - ret |= h->dev->ops->set_loopback(h, loop, 0x1); >> - } >> + ret = hns_nic_config_phy_loopback(phy_dev, 0x1); >> + ret |= h->dev->ops->set_loopback(h, loop, 0x1); > > Or'ing together two errno values does not make much sense: > >> + if (loop == MAC_INTERNALLOOP_PHY) >> + ret = __lb_setup(ndev, MAC_LOOP_PHY_NONE); >> + else >> + ret = __lb_setup(ndev, MAC_LOOP_NONE); >> if (ret) >> netdev_err(ndev, "%s: __lb_setup return error(%d)!\n", >> __func__, > > And it looks like you even print the OR'ed version here! > Thanks for pointing out, will modify it next version. Best Regard Yunsheng Lin
> >> - phy_write(phy_dev, COPPER_CONTROL_REG, val); > >> + err = phy_resume(phy_dev); > > > > Maybe this was discussed with an earlier version of these patches. Why > > are using phy_resume() and phy_suspend()? > When self_test is invoked with ETH_TEST_FL_OFFLINE option, hns mac driver > call dev_close to set net dev to offline state if net dev is online. > Doing the actual phy loolback test require phy is power up, So phy_resume > and phy_suspend are used. O.K, so you at least need some comments, because this is not obvious. From your description, it sounds like you can call phy_resume() on a device which is not suspended. 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. Also, what about if WOL has been set before closing the device? Andrew
Hi, Andrew On 2017/6/27 21:29, Andrew Lunn wrote: >>>> - phy_write(phy_dev, COPPER_CONTROL_REG, val); >>>> + err = phy_resume(phy_dev); >>> >>> Maybe this was discussed with an earlier version of these patches. Why >>> are using phy_resume() and phy_suspend()? >> When self_test is invoked with ETH_TEST_FL_OFFLINE option, hns mac driver >> call dev_close to set net dev to offline state if net dev is online. >> Doing the actual phy loolback test require phy is power up, So phy_resume >> and phy_suspend are used. > > O.K, so you at least need some comments, because this is not obvious. > >>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? If that is the case, is there any way I can ensure the device is suspended? 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? > > 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; } Best Regard Yunsheng Lin
> >>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. > > 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
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 > > . >
> > 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? Nope. The PHY will only be suspended when you actually call phy_suspend. > 2. If user runs the self test after using 'ifconfig ethX down', then I suppose > phy is already suspended. Assuming the phy has at some point been up, after a down, it should be suspended. The key thing here is, phy_resume() can only be called after a successful phy_suspend(). Those are the power management rules, and the expectations of the drivers. Doing a resume without first doing an explicit suspend is asking for bad things to happen. You are having trouble because you are not using the API for what it was intended. Maybe you need to take a step back and look at the bigger picture of how self tests are being performed. Why do you need the dev_close()/dev_open()? Maybe netif_device_detach()/netif_device_attach() would be better? How do other drivers do self test? Andrew
Hi, Andrew On 2017/6/29 21:56, Andrew Lunn wrote: >>> 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? > > Nope. The PHY will only be suspended when you actually call > phy_suspend. > >> 2. If user runs the self test after using 'ifconfig ethX down', then I suppose >> phy is already suspended. > > Assuming the phy has at some point been up, after a down, it should be > suspended. > > The key thing here is, phy_resume() can only be called after a > successful phy_suspend(). Those are the power management rules, and > the expectations of the drivers. Doing a resume without first doing an > explicit suspend is asking for bad things to happen. > > You are having trouble because you are not using the API for what it > was intended. Maybe you need to take a step back and look at the > bigger picture of how self tests are being performed. Why do you need > the dev_close()/dev_open()? Maybe > netif_device_detach()/netif_device_attach() would be better? > How do other drivers do self test? > Basically, when doing a loopback test, we put a skb in the tx ring, and see if we can receive it in the rx ring. And We can't find other functions that meets our requirement, except dev_close()/dev_open. netif_device_detach only stop the stack from sending packet, napi_disable is needed to stop the stack from receiving packet. And after phy loopback test, netif_device_attach does not bring back the link. Is there a way to bring back the link? We still need to resume the phy after 'ifconfig ethX down'. Also how can I tell the difference between phy being suspended after 'ifconfig ethX down' and phy being not suspended after mac driver is first loaded? It seems that ixgbe_ethtool in mainline kernel also use netif_tx_disable, napi_disable and other hardware specific method to setup self test. But newest ixgbe_ethtool code in github also use dev_close to do self test. Any idea? Best Regard Yunsheng Lin
> Any idea?
Maybe consider what the self test is good for.
My guess is, self test was added when a network interface card was a
full size VME card, and had a couple of hundred components or more.
They did break during there life, due to heat, mechanism stresses,
causing parts to pop off the PCB, or out of their sockets.
Nowadays, the Ethernet interface is part of the SoC, and just has
maybe 10 external parts for the PHY. What does a failed "MAC loopback
test" tell you? Probably that the driver has a bug, or there is a
silicon bug. What does "SERDES loopback test" tell you? Probably that
the driver has a bug, or there is a silicon bug. And since this is all
inside the silicon, if it fails for you, it is going to fail for
everybody, making the test pretty pointless.
What does a "PHY loopback test" tell you? There is a slim chance it
tells you the device has been hit by lightning, and the PHY is
fried. But more likely, that the driver has a bug, or there is a
silicon bug.
I really expect your own Q&A testing is much better at finding driver
and silicon bugs. You don't use the ethtool --selftest for this.
So i personally would just delete the whole selftest code, it is
pretty pointless.
Andrew
On Sat, Jul 01, 2017 at 11:57:32AM +0000, linyunsheng wrote: > Hi, Andrew > > I am agreed wih you on this. > But self test is also a feature of our product, and our > customer way choose to diagnose a problem using > self test, even if self test does not give a clear > reason to the problem. > we don't want to remove a feature that we don't > know when our customer will be using. Far enough. So please take a close look at the code and try to fix it. The corner cases are your problem, a down'ed interface, WOL, etc. It is issues like this which can result in phy_resume() being called without there first being a phy_suspend. Andrew
Hi, Andrew On 2017/7/1 23:17, Andrew Lunn wrote: > On Sat, Jul 01, 2017 at 11:57:32AM +0000, linyunsheng wrote: >> Hi, Andrew >> >> I am agreed wih you on this. >> But self test is also a feature of our product, and our >> customer way choose to diagnose a problem using >> self test, even if self test does not give a clear >> reason to the problem. >> we don't want to remove a feature that we don't >> know when our customer will be using. > > Far enough. So please take a close look at the code and try to fix > it. The corner cases are your problem, a down'ed interface, WOL, etc. > It is issues like this which can result in phy_resume() being called > without there first being a phy_suspend. > I looked into how the phy core deal with down'ed interface, WOL problem, here is what I found: 1.phydev->state is used to track the state of the phy. 2.phy_start/stop and phy_state_machine work together to make sure the phydev->state is consistent with phydev->suspended. And using phy_start/stop instead of phy_resume/suspend should take care of down'ed interface problem. Will using phy_start/stop cause other problems? As for WOL, phy_state_machine: if (needs_aneg) err = phy_start_aneg_priv(phydev, false); else if (do_suspend) phy_suspend(phydev); I think the phy core also have the same problem, because above code does not put the phy into suspending when it is WOL'ed, and it do not check the return value of phy_suspend. I hope I am not missing something obvious. Please let me know if you have any idea about WOL problem, thanks. Best Regards Yunsheng Lin
diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.h b/drivers/net/ethernet/hisilicon/hns/hnae.h index 04211ac..7ba653a 100644 --- a/drivers/net/ethernet/hisilicon/hns/hnae.h +++ b/drivers/net/ethernet/hisilicon/hns/hnae.h @@ -360,6 +360,7 @@ enum hnae_loop { MAC_INTERNALLOOP_MAC = 0, MAC_INTERNALLOOP_SERDES, MAC_INTERNALLOOP_PHY, + MAC_LOOP_PHY_NONE, MAC_LOOP_NONE, }; diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c index e95795b..10d82df 100644 --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c @@ -259,67 +259,24 @@ static int hns_nic_set_link_ksettings(struct net_device *net_dev, static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) { -#define COPPER_CONTROL_REG 0 -#define PHY_POWER_DOWN BIT(11) -#define PHY_LOOP_BACK BIT(14) - u16 val = 0; - - if (phy_dev->is_c45) /* c45 branch adding for XGE PHY */ - return -ENOTSUPP; + int err; if (en) { - /* speed : 1000M */ - phy_write(phy_dev, HNS_PHY_PAGE_REG, 2); - phy_write(phy_dev, 21, 0x1046); - - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); - /* Force Master */ - phy_write(phy_dev, 9, 0x1F00); - - /* Soft-reset */ - phy_write(phy_dev, 0, 0x9140); - /* If autoneg disabled,two soft-reset operations */ - phy_write(phy_dev, 0, 0x9140); - - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0xFA); - - /* Default is 0x0400 */ - phy_write(phy_dev, 1, 0x418); - - /* Force 1000M Link, Default is 0x0200 */ - phy_write(phy_dev, 7, 0x20C); - - /* Powerup Fiber */ - phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); - val = phy_read(phy_dev, COPPER_CONTROL_REG); - val &= ~PHY_POWER_DOWN; - phy_write(phy_dev, COPPER_CONTROL_REG, val); - - /* Enable Phy Loopback */ - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); - val = phy_read(phy_dev, COPPER_CONTROL_REG); - val |= PHY_LOOP_BACK; - val &= ~PHY_POWER_DOWN; - phy_write(phy_dev, COPPER_CONTROL_REG, val); + err = phy_resume(phy_dev); + if (err) + goto out; + + err = phy_loopback(phy_dev, true); } else { - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0xFA); - phy_write(phy_dev, 1, 0x400); - phy_write(phy_dev, 7, 0x200); - - phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); - val = phy_read(phy_dev, COPPER_CONTROL_REG); - val |= PHY_POWER_DOWN; - phy_write(phy_dev, COPPER_CONTROL_REG, val); - - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); - phy_write(phy_dev, 9, 0xF00); - - val = phy_read(phy_dev, COPPER_CONTROL_REG); - val &= ~PHY_LOOP_BACK; - val |= PHY_POWER_DOWN; - phy_write(phy_dev, COPPER_CONTROL_REG, val); + err = phy_loopback(phy_dev, false); + if (err) + goto out; + + err = phy_suspend(phy_dev); } - return 0; + +out: + return err; } static int __lb_setup(struct net_device *ndev, @@ -332,10 +289,8 @@ static int __lb_setup(struct net_device *ndev, switch (loop) { case MAC_INTERNALLOOP_PHY: - if ((phy_dev) && (!phy_dev->is_c45)) { - ret = hns_nic_config_phy_loopback(phy_dev, 0x1); - ret |= h->dev->ops->set_loopback(h, loop, 0x1); - } + ret = hns_nic_config_phy_loopback(phy_dev, 0x1); + ret |= h->dev->ops->set_loopback(h, loop, 0x1); break; case MAC_INTERNALLOOP_MAC: if ((h->dev->ops->set_loopback) && @@ -346,10 +301,9 @@ static int __lb_setup(struct net_device *ndev, if (h->dev->ops->set_loopback) ret = h->dev->ops->set_loopback(h, loop, 0x1); break; + case MAC_LOOP_PHY_NONE: + ret |= hns_nic_config_phy_loopback(phy_dev, 0x0); case MAC_LOOP_NONE: - if ((phy_dev) && (!phy_dev->is_c45)) - ret |= hns_nic_config_phy_loopback(phy_dev, 0x0); - if (h->dev->ops->set_loopback) { if (priv->ae_handle->phy_if != PHY_INTERFACE_MODE_XGMII) ret |= h->dev->ops->set_loopback(h, @@ -582,13 +536,16 @@ static int __lb_run_test(struct net_device *ndev, return ret_val; } -static int __lb_down(struct net_device *ndev) +static int __lb_down(struct net_device *ndev, enum hnae_loop loop) { struct hns_nic_priv *priv = netdev_priv(ndev); struct hnae_handle *h = priv->ae_handle; int ret; - ret = __lb_setup(ndev, MAC_LOOP_NONE); + if (loop == MAC_INTERNALLOOP_PHY) + ret = __lb_setup(ndev, MAC_LOOP_PHY_NONE); + else + ret = __lb_setup(ndev, MAC_LOOP_NONE); if (ret) netdev_err(ndev, "%s: __lb_setup return error(%d)!\n", __func__, @@ -644,7 +601,8 @@ static void hns_nic_self_test(struct net_device *ndev, if (!data[test_index]) { data[test_index] = __lb_run_test( ndev, (enum hnae_loop)st_param[i][0]); - (void)__lb_down(ndev); + (void)__lb_down(ndev, + (enum hnae_loop)st_param[i][0]); } if (data[test_index])
Use function set_loopback in phy_driver to setup phy loopback when doing ethtool self test. Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> --- drivers/net/ethernet/hisilicon/hns/hnae.h | 1 + drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 92 +++++++----------------- 2 files changed, 26 insertions(+), 67 deletions(-)