Hi Wenzhou,

"Lu, Wenzhuo" [off-list ref] writes:

Hi Aaron,

quoted

-----Original Message-----
From: Aaron Conole [mailto:aconole@redhat.com]
Sent: Friday, February 26, 2016 2:49 AM
To: dev@dpdk.org
Cc: Lu, Wenzhuo; Zhang, Helin; Ananyev, Konstantin; Richardson, Bruce
Subject: [PATCH 3/8] drivers/net/e1000: Fix missing brackets

The register read/write mphy functions have misleading whitespace around the
locked check. This cleanup merely preserves the existing functionality while
improving the ready check.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/e1000/base/e1000_phy.c

b/drivers/net/e1000/base/e1000_phy.c
index d43b7ce..8642d38 100644

--- a/drivers/net/e1000/base/e1000_phy.c
+++ b/drivers/net/e1000/base/e1000_phy.c

@@ -4153,13 +4153,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw

*hw, u32 address, u32 *data)
 	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);

 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+	}

+	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
E1000_MPHY_DIS_ACCESS);
 	return E1000_SUCCESS;
 }

@@ -4218,13 +4218,13 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw

*hw, u32 address, u32 data,
 	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);

 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+	}

+	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
E1000_MPHY_DIS_ACCESS);
 	return E1000_SUCCESS;
 }

--
2.5.0

Normally we will not maintain the base code. It's just taken from kernel driver.
Agree with you that the whitespace is misleading. But as it's no real
impact. I'd like to say not a big deal, better not change it. :)

Thanks for this hint. It turns out my patch is wrong. It should actually
be this (and I've confirmed by looking at the drivers):

diff --git a/drivers/net/e1000/base/e1000_phy.c b/drivers/net/e1000/base/e1000_phy.c
index d43b7ce..ad3fd58 100644
--- a/drivers/net/e1000/base/e1000_phy.c
+++ b/drivers/net/e1000/base/e1000_phy.c

@@ -4153,12 +4153,12 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 *data)
 	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
 
 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
+	}
 
 	return E1000_SUCCESS;
 }

@@ -4218,12 +4218,12 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 data,
 	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
 
 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
+	}
 
 	return E1000_SUCCESS;
 }

I will cook up a v2 of this patch if it makes sense. It is a real bug,
so should be fixed.

-Aaron