Re: [dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets

[Aaron Conole <aconole@redhat.com>]

Hi Wenzhou,

"Lu, Wenzhuo" <wenzhuo.lu@intel.com> writes:

> Hi Aaron,
>
>
>> -----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