[dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets
2016-03-01 13:02, Panu Matilainen: > On 02/26/2016 03:13 PM, Aaron Conole wrote: > > "Lu, Wenzhuo" writes: > >> 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): [...] > > I will cook up a v2 of this patch if it makes sense. It is a real bug, > > so should be fixed. Yes, waiting for your v2. > Yes, it quite clearly is a real bug and there needs to be a documented > way of getting these things fixed. The README in the base/ directory is > not particularly helpful, since it only says "dont touch it". Yes don't touch it ;) > This is apparently fixed in FreeBSD codebase so fixing it would be > "just" a matter of pulling in a newer version. Probably. > The other alternatives are either disabling the whole driver in gcc 6 > builds, or paper over the bug with warning disablers, or have everybody > patch their packages locally to fix it, all of which just feel so stupid > they're not alternatives, really. No they are not sane alternatives. > OTOH the bug has been there for 2.5 years (since commit > 38db3f7f50bde45477f564783a06ac8fbd3348fa) and nobody has noticed... > > Thomas, Bruce, thoughts/comments? It is in the hands of Wenzhuo, the e1000 maintainer. You just need his ack.
[dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets
> -Original Message-
> From: Aaron Conole [mailto:aconole at redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev at 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
> ---
> 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);
I think the above line should be included in brackets above.
Though we should avoid modifying any code in base driver, I'd
suggest you to fix the issue here. Thank you!
> 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;
The same comments as above. Thanks!
> }
>
> --
> 2.5.0
[dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets
On 02/26/2016 03:13 PM, Aaron Conole wrote:
> Hi Wenzhou,
>
> "Lu, Wenzhuo" writes:
>
>> Hi Aaron,
>>
>>
>>> -Original Message-
>>> From: Aaron Conole [mailto:aconole at redhat.com]
>>> Sent: Friday, February 26, 2016 2:49 AM
>>> To: dev at 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
>>> ---
>>> 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.
Yes, it quite clearly is a real bug and there needs to be a documented
way of getting these things fixed. The README in the base/ directory is
not particularly helpful, since it only says "dont touch it".
This is apparently fixed in FreeBSD codebase so fixing it would be
"just" a matter of pulling in a newer version.
The other alternatives are either disabling the whole driver in gcc 6
builds, or paper over the bug with warning disablers, or have everybody
patch their packages locally to fix it, all of which just feel so stupid
they're not alternatives, really.
OTOH the bug has been there for 2.5 years (since commit
38db3f7f50bde45477f564783a06ac8fbd3348fa) and nobody has noticed...
Thomas, Bruce, thoughts/comments?
- Panu -
[dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets
Hi Wenzhou,
"Lu, Wenzhuo" writes:
> Hi Aaron,
>
>
>> -Original Message-
>> From: Aaron Conole [mailto:aconole at redhat.com]
>> Sent: Friday, February 26, 2016 2:49 AM
>> To: dev at 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
>> ---
>> 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
[dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets
Hi Aaron,
> -Original Message-
> From: Aaron Conole [mailto:aconole at redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev at 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
> ---
> 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. :)
[dpdk-dev] [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
---
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
