* [PATCH NET V5 0/2] Add loopback support in phy_driver and hns ethtool fix
@ 2017-06-26 2:10 Lin Yun Sheng
2017-06-26 2:10 ` [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework Lin Yun Sheng
2017-06-26 2:10 ` [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback Lin Yun Sheng
0 siblings, 2 replies; 16+ messages in thread
From: Lin Yun Sheng @ 2017-06-26 2:10 UTC (permalink / raw)
To: davem, andrew, f.fainelli
Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni,
john.garry, linuxarm, yisen.zhuang, salil.mehta, lipeng321,
tremyfr, netdev, linux-kernel
This Patch Set add set_loopback in phy_driver and use it to setup loopback
when doing ethtool phy self_test.
Patch V5:
Removing non loopback related code change.
Patch V4:
1. Remove c45 checking
2. Add -ENOTSUPP when function pointer is null,
take mutex in phy_loopback.
Patch V3:
Calling phy_loopback enable and disable in pair in hns mac driver.
Patch V2:
1. Add phy_loopback in phy_device.c.
2. Do error checking and do the read and write once in
genphy_loopback.
3. Remove gen10g_loopback in phy_device.c.
Patch V1:
Initial Submit
Lin Yun Sheng (2):
net: phy: Add phy loopback support in net phy framework
net: hns: Use phy_driver to setup Phy loopback
drivers/net/ethernet/hisilicon/hns/hnae.h | 1 +
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 92 +++++++-----------------
drivers/net/phy/marvell.c | 1 +
drivers/net/phy/phy_device.c | 51 +++++++++++++
include/linux/phy.h | 5 ++
5 files changed, 83 insertions(+), 67 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework 2017-06-26 2:10 [PATCH NET V5 0/2] Add loopback support in phy_driver and hns ethtool fix Lin Yun Sheng @ 2017-06-26 2:10 ` Lin Yun Sheng 2017-06-26 13:43 ` Andrew Lunn 2017-06-26 16:45 ` Florian Fainelli 2017-06-26 2:10 ` [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback Lin Yun Sheng 1 sibling, 2 replies; 16+ messages in thread From: Lin Yun Sheng @ 2017-06-26 2:10 UTC (permalink / raw) To: davem, andrew, f.fainelli Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, yisen.zhuang, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel This patch add set_loopback in phy_driver, which is used by Mac driver to enable or disable a phy. it also add a generic genphy_loopback function, which use BMCR loopback bit to enable or disable a phy. Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> --- drivers/net/phy/marvell.c | 1 + drivers/net/phy/phy_device.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/phy.h | 5 +++++ 3 files changed, 57 insertions(+) diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 57297ba..01a1586 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -2094,6 +2094,7 @@ static int m88e1510_probe(struct phy_device *phydev) .get_sset_count = marvell_get_sset_count, .get_strings = marvell_get_strings, .get_stats = marvell_get_stats, + .set_loopback = genphy_loopback, }, { .phy_id = MARVELL_PHY_ID_88E1540, diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 1219eea..1e08d62 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1123,6 +1123,39 @@ int phy_resume(struct phy_device *phydev) } EXPORT_SYMBOL(phy_resume); +int phy_loopback(struct phy_device *phydev, bool enable) +{ + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver); + int ret = 0; + + mutex_lock(&phydev->lock); + + if (enable && phydev->loopback_enabled) { + ret = -EBUSY; + goto out; + } + + if (!enable && !phydev->loopback_enabled) { + ret = -EINVAL; + goto out; + } + + if (phydev->drv && phydrv->set_loopback) + ret = phydrv->set_loopback(phydev, enable); + else + ret = -EOPNOTSUPP; + + if (ret) + goto out; + + phydev->loopback_enabled = enable; + +out: + mutex_unlock(&phydev->lock); + return ret; +} +EXPORT_SYMBOL(phy_loopback); + /* Generic PHY support and helper functions */ /** @@ -1628,6 +1661,23 @@ static int gen10g_resume(struct phy_device *phydev) return 0; } +int genphy_loopback(struct phy_device *phydev, bool enable) +{ + int value; + + value = phy_read(phydev, MII_BMCR); + if (value < 0) + return value; + + if (enable) + value |= BMCR_LOOPBACK; + else + value &= ~BMCR_LOOPBACK; + + return phy_write(phydev, MII_BMCR, value); +} +EXPORT_SYMBOL(genphy_loopback); + static int __set_phy_supported(struct phy_device *phydev, u32 max_speed) { /* The default values for phydev->supported are provided by the PHY @@ -1874,6 +1924,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) .read_status = genphy_read_status, .suspend = genphy_suspend, .resume = genphy_resume, + .set_loopback = genphy_loopback, }, { .phy_id = 0xffffffff, .phy_id_mask = 0xffffffff, diff --git a/include/linux/phy.h b/include/linux/phy.h index e76e4ad..49c903dc 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -364,6 +364,7 @@ struct phy_c45_device_ids { * is_pseudo_fixed_link: Set to true if this phy is an Ethernet switch, etc. * has_fixups: Set to true if this phy has fixups/quirks. * suspended: Set to true if this phy has been suspended successfully. + * loopback_enabled: Set true if this phy has been loopbacked successfully. * state: state of the PHY for management purposes * dev_flags: Device-specific flags used by the PHY driver. * link_timeout: The number of timer firings to wait before the @@ -400,6 +401,7 @@ struct phy_device { bool is_pseudo_fixed_link; bool has_fixups; bool suspended; + bool loopback_enabled; enum phy_state state; @@ -639,6 +641,7 @@ struct phy_driver { int (*set_tunable)(struct phy_device *dev, struct ethtool_tunable *tuna, const void *data); + int (*set_loopback)(struct phy_device *dev, bool enable); }; #define to_phy_driver(d) container_of(to_mdio_common_driver(d), \ struct phy_driver, mdiodrv) @@ -774,6 +777,7 @@ static inline void phy_device_free(struct phy_device *phydev) { } int phy_init_hw(struct phy_device *phydev); int phy_suspend(struct phy_device *phydev); int phy_resume(struct phy_device *phydev); +int phy_loopback(struct phy_device *phydev, bool enable); struct phy_device *phy_attach(struct net_device *dev, const char *bus_id, phy_interface_t interface); struct phy_device *phy_find_first(struct mii_bus *bus); @@ -825,6 +829,7 @@ void phy_attached_print(struct phy_device *phydev, const char *fmt, ...) int genphy_read_status(struct phy_device *phydev); int genphy_suspend(struct phy_device *phydev); int genphy_resume(struct phy_device *phydev); +int genphy_loopback(struct phy_device *phydev, bool enable); int genphy_soft_reset(struct phy_device *phydev); static inline int genphy_no_soft_reset(struct phy_device *phydev) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework 2017-06-26 2:10 ` [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework Lin Yun Sheng @ 2017-06-26 13:43 ` Andrew Lunn 2017-06-26 16:45 ` Florian Fainelli 1 sibling, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2017-06-26 13:43 UTC (permalink / raw) To: Lin Yun Sheng Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel On Mon, Jun 26, 2017 at 10:10:38AM +0800, Lin Yun Sheng wrote: > This patch add set_loopback in phy_driver, which is used by Mac > driver to enable or disable a phy. it also add a generic > genphy_loopback function, which use BMCR loopback bit to enable > or disable a phy. > > Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework 2017-06-26 2:10 ` [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework Lin Yun Sheng 2017-06-26 13:43 ` Andrew Lunn @ 2017-06-26 16:45 ` Florian Fainelli 1 sibling, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2017-06-26 16:45 UTC (permalink / raw) To: Lin Yun Sheng, davem, andrew Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel On 06/25/2017 07:10 PM, Lin Yun Sheng wrote: > This patch add set_loopback in phy_driver, which is used by Mac > driver to enable or disable a phy. it also add a generic > genphy_loopback function, which use BMCR loopback bit to enable > or disable a phy. > > Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> [snip] > +int phy_loopback(struct phy_device *phydev, bool enable) > +{ > + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver); > + int ret = 0; > + > + mutex_lock(&phydev->lock); > + > + if (enable && phydev->loopback_enabled) { > + ret = -EBUSY; > + goto out; > + } > + > + if (!enable && !phydev->loopback_enabled) { > + ret = -EINVAL; > + goto out; > + } I am not sure if it is necessary to treat that condition as an error, but I guess why not. -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-26 2:10 [PATCH NET V5 0/2] Add loopback support in phy_driver and hns ethtool fix Lin Yun Sheng 2017-06-26 2:10 ` [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework Lin Yun Sheng @ 2017-06-26 2:10 ` Lin Yun Sheng 2017-06-26 13:42 ` Andrew Lunn 1 sibling, 1 reply; 16+ messages in thread From: Lin Yun Sheng @ 2017-06-26 2:10 UTC (permalink / raw) To: davem, andrew, f.fainelli Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, yisen.zhuang, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel 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); + 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]) -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-26 2:10 ` [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback Lin Yun Sheng @ 2017-06-26 13:42 ` Andrew Lunn 2017-06-27 3:25 ` Yunsheng Lin 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-26 13:42 UTC (permalink / raw) To: Lin Yun Sheng Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-26 13:42 ` Andrew Lunn @ 2017-06-27 3:25 ` Yunsheng Lin 2017-06-27 13:29 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Yunsheng Lin @ 2017-06-27 3:25 UTC (permalink / raw) To: Andrew Lunn Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-27 3:25 ` Yunsheng Lin @ 2017-06-27 13:29 ` Andrew Lunn 2017-06-28 0:59 ` Yunsheng Lin 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-27 13:29 UTC (permalink / raw) To: Yunsheng Lin Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel > >> - 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-27 13:29 ` Andrew Lunn @ 2017-06-28 0:59 ` Yunsheng Lin 2017-06-28 20:28 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Yunsheng Lin @ 2017-06-28 0:59 UTC (permalink / raw) To: Andrew Lunn Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-28 0:59 ` Yunsheng Lin @ 2017-06-28 20:28 ` Andrew Lunn 2017-06-29 2:35 ` Yunsheng Lin 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-28 20:28 UTC (permalink / raw) To: Yunsheng Lin Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel > >>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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-28 20:28 ` Andrew Lunn @ 2017-06-29 2:35 ` Yunsheng Lin 2017-06-29 13:56 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Yunsheng Lin @ 2017-06-29 2:35 UTC (permalink / raw) To: Andrew Lunn Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel 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 > > . > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-29 2:35 ` Yunsheng Lin @ 2017-06-29 13:56 ` Andrew Lunn 2017-06-30 9:14 ` Yunsheng Lin 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-29 13:56 UTC (permalink / raw) To: Yunsheng Lin Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel > > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-29 13:56 ` Andrew Lunn @ 2017-06-30 9:14 ` Yunsheng Lin 2017-06-30 13:39 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Yunsheng Lin @ 2017-06-30 9:14 UTC (permalink / raw) To: Andrew Lunn Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-30 9:14 ` Yunsheng Lin @ 2017-06-30 13:39 ` Andrew Lunn 2017-07-01 15:17 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-30 13:39 UTC (permalink / raw) To: Yunsheng Lin Cc: davem, f.fainelli, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, tremyfr, netdev, linux-kernel > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-06-30 13:39 ` Andrew Lunn @ 2017-07-01 15:17 ` Andrew Lunn 2017-07-03 9:57 ` Yunsheng Lin 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-07-01 15:17 UTC (permalink / raw) To: linyunsheng Cc: davem, f.fainelli, huangdaode, xuwei (O), Liguozhu (Kenneth), Zhuangyuzeng (Yisen), Gabriele Paoloni, John Garry, Linuxarm, Salil Mehta, lipeng (Y), tremyfr, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback 2017-07-01 15:17 ` Andrew Lunn @ 2017-07-03 9:57 ` Yunsheng Lin 0 siblings, 0 replies; 16+ messages in thread From: Yunsheng Lin @ 2017-07-03 9:57 UTC (permalink / raw) To: Andrew Lunn Cc: davem, f.fainelli, huangdaode, xuwei (O), Liguozhu (Kenneth), Zhuangyuzeng (Yisen), Gabriele Paoloni, John Garry, Linuxarm, Salil Mehta, lipeng (Y), tremyfr, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-07-03 9:57 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-26 2:10 [PATCH NET V5 0/2] Add loopback support in phy_driver and hns ethtool fix Lin Yun Sheng 2017-06-26 2:10 ` [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework Lin Yun Sheng 2017-06-26 13:43 ` Andrew Lunn 2017-06-26 16:45 ` Florian Fainelli 2017-06-26 2:10 ` [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback Lin Yun Sheng 2017-06-26 13:42 ` Andrew Lunn 2017-06-27 3:25 ` Yunsheng Lin 2017-06-27 13:29 ` Andrew Lunn 2017-06-28 0:59 ` Yunsheng Lin 2017-06-28 20:28 ` Andrew Lunn 2017-06-29 2:35 ` Yunsheng Lin 2017-06-29 13:56 ` Andrew Lunn 2017-06-30 9:14 ` Yunsheng Lin 2017-06-30 13:39 ` Andrew Lunn 2017-07-01 15:17 ` Andrew Lunn 2017-07-03 9:57 ` Yunsheng Lin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).