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

[PATCH NET v3 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 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 | 82 +++++++-----------------
 drivers/net/phy/marvell.c                        |  1 +
 drivers/net/phy/phy_device.c                     | 41 ++++++++++++
 include/linux/phy.h                              |  5 ++
 5 files changed, 71 insertions(+), 59 deletions(-)

-- 
1.9.1

[PATCH NET v3 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 | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  5 +++++
 3 files changed, 47 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..1e8f800 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1123,6 +1123,29 @@ 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;
+
+	if (enable && phydev->loopback_enabled)
+		return -EBUSY;
+
+	if (!enable && !phydev->loopback_enabled)
+		return -EINVAL;
+
+	if (phydev->drv && phydrv->set_loopback)
+		ret = phydrv->set_loopback(phydev, enable);
+
+	if (ret)
+		return ret;
+
+	phydev->loopback_enabled = enable;
+
+	return 0;
+}
+EXPORT_SYMBOL(phy_loopback);
+
 /* Generic PHY support and helper functions */
 
 /**
@@ -1628,6 +1651,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 +1914,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 v3 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 | 82 +++++++-----------------
 2 files changed, 24 insertions(+), 59 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..1d54b14 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -259,67 +259,27 @@ 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;
+	int err;
 
 	if (phy_dev->is_c45) /* c45 branch adding for XGE PHY */
 		return -ENOTSUPP;
 
 	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,
@@ -346,10 +306,10 @@ 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_NONE:
+	case MAC_LOOP_PHY_NONE:
 		if ((phy_dev) && (!phy_dev->is_c45))
 			ret |= hns_nic_config_phy_loopback(phy_dev, 0x0);
-
+	case MAC_LOOP_NONE:
 		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 +542,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 +607,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 v3 1/2] net: phy: Add phy loopback support in net phy framework

[Andrew Lunn]

> +int phy_loopback(struct phy_device *phydev, bool enable)
> +{
> +	struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
> +	int ret = 0;
> +
> +	if (enable && phydev->loopback_enabled)
> +		return -EBUSY;
> +
> +	if (!enable && !phydev->loopback_enabled)
> +		return -EINVAL;
> +
> +	if (phydev->drv && phydrv->set_loopback)
> +		ret = phydrv->set_loopback(phydev, enable);

   	else
		ret = -EOPNOTSUPP;

> +
> +	if (ret)
> +		return ret;
> +
> +	phydev->loopback_enabled = enable;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_loopback);

One of the comments we made of the PHY code in the hns driver is that
its locking is completely broken. You have made the same error
here. The core needs to hold the mutex while calling into the PHY
driver.

	Andrew

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

[Andrew Lunn]

>  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;
> +	int err;
>  
>  	if (phy_dev->is_c45) /* c45 branch adding for XGE PHY */
>  		return -ENOTSUPP;

You should take this out as well. You want the core to tell you if
loopback is supported or not. At some point, a c45 PHY could support
loopback.

> +	case MAC_LOOP_PHY_NONE:
>  		if ((phy_dev) && (!phy_dev->is_c45))
>  			ret |= hns_nic_config_phy_loopback(phy_dev, 0x0);

same here.

     Andrew

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

[Yunsheng Lin]

Hi, Andrew

On 2017/6/24 11:12, Andrew Lunn wrote:
>> +int phy_loopback(struct phy_device *phydev, bool enable)
>> +{
>> +	struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
>> +	int ret = 0;
>> +
>> +	if (enable && phydev->loopback_enabled)
>> +		return -EBUSY;
>> +
>> +	if (!enable && !phydev->loopback_enabled)
>> +		return -EINVAL;
>> +
>> +	if (phydev->drv && phydrv->set_loopback)
>> +		ret = phydrv->set_loopback(phydev, enable);
> 
>    	else
> 		ret = -EOPNOTSUPP;
> 
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	phydev->loopback_enabled = enable;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(phy_loopback);
> 
> One of the comments we made of the PHY code in the hns driver is that
> its locking is completely broken. You have made the same error
> here. The core needs to hold the mutex while calling into the PHY
> driver.
Do you mean hns_nic_config_phy_loopback need to hold the mutex while
calling phy_loopback? and other place that calling phy_* function?

Best Regards
Yunsheng Lin

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

[Yunsheng Lin]

On 2017/6/24 11:40, Yunsheng Lin wrote:
> Hi, Andrew
> 
> On 2017/6/24 11:12, Andrew Lunn wrote:
>>> +int phy_loopback(struct phy_device *phydev, bool enable)
>>> +{
>>> +	struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
>>> +	int ret = 0;
>>> +
>>> +	if (enable && phydev->loopback_enabled)
>>> +		return -EBUSY;
>>> +
>>> +	if (!enable && !phydev->loopback_enabled)
>>> +		return -EINVAL;
>>> +
>>> +	if (phydev->drv && phydrv->set_loopback)
>>> +		ret = phydrv->set_loopback(phydev, enable);
>>
>>    	else
>> 		ret = -EOPNOTSUPP;
>>
>>> +
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	phydev->loopback_enabled = enable;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(phy_loopback);
>>
>> One of the comments we made of the PHY code in the hns driver is that
>> its locking is completely broken. You have made the same error
>> here. The core needs to hold the mutex while calling into the PHY
>> driver.
> Do you mean hns_nic_config_phy_loopback need to hold the mutex while
> calling phy_loopback? and other place that calling phy_* function?
I took some time looking into how to take mutex in phy core, here is what
I find:
phy_resume call phydrv->resume without take mutex.
if phy driver implement resume function, for example marvell_resume, then

static int marvell_resume(struct phy_device *phydev)
{
	int err;

	/* Resume the fiber mode first */
	if (!(phydev->supported & SUPPORTED_FIBRE)) {
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
		if (err < 0)
			goto error;

		/* With the page set, use the generic resume */
		err = genphy_resume(phydev);
		if (err < 0)
			goto error;

		/* Then, the copper link */
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
		if (err < 0)
			goto error;
	}

the code above executes without holding a mutex expect genphy_resume

	/* With the page set, use the generic resume */
	return genphy_resume(phydev);

error:
	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
	return err;
}

So I think the correct way to hold a lock is in phy_* function, not in genphy_*,
and current genphy_resume and phy_resume is broken in this way.
Please let me know if I misunderstand the mutex taking in phy core.

Best Regards
Yunsheng Lin

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

[Andrew Lunn]

> phy_resume call phydrv->resume without take mutex.
> if phy driver implement resume function, for example marvell_resume, then
> 
> static int marvell_resume(struct phy_device *phydev)
> {
> 	int err;
> 
> 	/* Resume the fiber mode first */
> 	if (!(phydev->supported & SUPPORTED_FIBRE)) {
> 		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> 		if (err < 0)
> 			goto error;
> 
> 		/* With the page set, use the generic resume */
> 		err = genphy_resume(phydev);
> 		if (err < 0)
> 			goto error;
> 
> 		/* Then, the copper link */
> 		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> 		if (err < 0)
> 			goto error;
> 	}
> 
> the code above executes without holding a mutex expect genphy_resume
> 
> 	/* With the page set, use the generic resume */
> 	return genphy_resume(phydev);
> 
> error:
> 	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> 	return err;
> }
> 
> So I think the correct way to hold a lock is in phy_* function, not in genphy_*,
> and current genphy_resume and phy_resume is broken in this way.

Suspend and resume are a bit odd. As you found out, everywhere else,
the core takes the mutex before calling into the driver function.

The general rule of thumb, is that many driver writers don't
understand locking. So it is better to do the locking in the core,
which is generally written by people who do understand locking. And
core code gets used more, so bugs tend to be found.

     Andrew