[PATCH NET V5 0/2] Add loopback support in phy_driver and hns ethtool fix

[PATCH NET V5 0/2] Add loopback support in phy_driver and hns ethtool fix

[Lin Yun Sheng]

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

[PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework

[Lin Yun Sheng]

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

[PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Lin Yun Sheng]

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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Andrew Lunn]

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

Re: [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework

[Andrew Lunn]

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

Re: [PATCH NET V5 1/2] net: phy: Add phy loopback support in net phy framework

[Florian Fainelli]

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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Yunsheng Lin]

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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Andrew Lunn]

> >> -		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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Yunsheng Lin]

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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Andrew Lunn]

> >>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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Yunsheng Lin]

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
> 
> .
> 

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Andrew Lunn]

> > 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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Yunsheng Lin]

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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Andrew Lunn]

> 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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Andrew Lunn]

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

Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Yunsheng Lin]

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