* [net-intel-e1000e] question about value overwrite
@ 2017-05-18 22:22 Gustavo A. R. Silva
2017-06-20 21:22 ` [PATCH] net: intel: e1000e: add check on e1e_wphy() return value Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-18 22:22 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: intel-wired-lan, netdev, linux-kernel
Hello everybody,
While looking into Coverity ID 1226905 I ran into the following piece
of code at drivers/net/ethernet/intel/e1000e/ich8lan.c:2400
2400/**
2401 * e1000_hv_phy_workarounds_ich8lan - A series of Phy workarounds to be
2402 * done after every PHY reset.
2403 **/
2404static s32 e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw)
2405{
2406 s32 ret_val = 0;
2407 u16 phy_data;
2408
2409 if (hw->mac.type != e1000_pchlan)
2410 return 0;
2411
2412 /* Set MDIO slow mode before any other MDIO access */
2413 if (hw->phy.type == e1000_phy_82577) {
2414 ret_val = e1000_set_mdio_slow_mode_hv(hw);
2415 if (ret_val)
2416 return ret_val;
2417 }
2418
2419 if (((hw->phy.type == e1000_phy_82577) &&
2420 ((hw->phy.revision == 1) || (hw->phy.revision == 2))) ||
2421 ((hw->phy.type == e1000_phy_82578) &&
(hw->phy.revision == 1))) {
2422 /* Disable generation of early preamble */
2423 ret_val = e1e_wphy(hw, PHY_REG(769, 25), 0x4431);
2424 if (ret_val)
2425 return ret_val;
2426
2427 /* Preamble tuning for SSC */
2428 ret_val = e1e_wphy(hw, HV_KMRN_FIFO_CTRLSTA, 0xA204);
2429 if (ret_val)
2430 return ret_val;
2431 }
2432
2433 if (hw->phy.type == e1000_phy_82578) {
2434 /* Return registers to default by doing a soft reset then
2435 * writing 0x3140 to the control register.
2436 */
2437 if (hw->phy.revision < 2) {
2438 e1000e_phy_sw_reset(hw);
2439 ret_val = e1e_wphy(hw, MII_BMCR, 0x3140);
2440 }
2441 }
2442
2443 /* Select page 0 */
2444 ret_val = hw->phy.ops.acquire(hw);
2445 if (ret_val)
2446 return ret_val;
2447
2448 hw->phy.addr = 1;
2449 ret_val = e1000e_write_phy_reg_mdic(hw,
IGP01E1000_PHY_PAGE_SELECT, 0);
2450 hw->phy.ops.release(hw);
2451 if (ret_val)
2452 return ret_val;
2453
2454 /* Configure the K1 Si workaround during phy reset
assuming there is
2455 * link so that it disables K1 if link is in 1Gbps.
2456 */
2457 ret_val = e1000_k1_gig_workaround_hv(hw, true);
2458 if (ret_val)
2459 return ret_val;
2460
2461 /* Workaround for link disconnects on a busy hub in half duplex */
2462 ret_val = hw->phy.ops.acquire(hw);
2463 if (ret_val)
2464 return ret_val;
2465 ret_val = e1e_rphy_locked(hw, BM_PORT_GEN_CFG, &phy_data);
2466 if (ret_val)
2467 goto release;
2468 ret_val = e1e_wphy_locked(hw, BM_PORT_GEN_CFG, phy_data & 0x00FF);
2469 if (ret_val)
2470 goto release;
2471
2472 /* set MSE higher to enable link to stay up when noise is high */
2473 ret_val = e1000_write_emi_reg_locked(hw,
I82577_MSE_THRESHOLD, 0x0034);
2474release:
2475 hw->phy.ops.release(hw);
2476
2477 return ret_val;
2478}
The issue is that the value stored in variable _ret_val_ at line 2439
is overwritten by the one stored at line 2444, before it can be used.
My question is if the original intention was to return this value
immediately after the assignment at line 2439, something like in the
following patch:
index 68ea8b4..d6d4ed7 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -2437,6 +2437,8 @@ static s32
e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw)
if (hw->phy.revision < 2) {
e1000e_phy_sw_reset(hw);
ret_val = e1e_wphy(hw, MII_BMCR, 0x3140);
+ if (ret_val)
+ return ret_val;
}
}
What do you think?
I'd really appreciate any comment on this.
Thank you!
--
Gustavo A. R. Silva
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] net: intel: e1000e: add check on e1e_wphy() return value
2017-05-18 22:22 [net-intel-e1000e] question about value overwrite Gustavo A. R. Silva
@ 2017-06-20 21:22 ` Gustavo A. R. Silva
2017-06-21 2:44 ` Ethan Zhao
2017-06-27 1:21 ` Brown, Aaron F
0 siblings, 2 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-06-20 21:22 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: intel-wired-lan, netdev, linux-kernel, Gustavo A. R. Silva
Check return value from call to e1e_wphy(). This value is being
checked during previous calls to function e1e_wphy() and it seems
a check was missing here.
Addresses-Coverity-ID: 1226905
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 68ea8b4..d6d4ed7 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -2437,6 +2437,8 @@ static s32 e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw)
if (hw->phy.revision < 2) {
e1000e_phy_sw_reset(hw);
ret_val = e1e_wphy(hw, MII_BMCR, 0x3140);
+ if (ret_val)
+ return ret_val;
}
}
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: intel: e1000e: add check on e1e_wphy() return value
2017-06-20 21:22 ` [PATCH] net: intel: e1000e: add check on e1e_wphy() return value Gustavo A. R. Silva
@ 2017-06-21 2:44 ` Ethan Zhao
2017-06-21 19:52 ` Gustavo A. R. Silva
2017-06-27 1:21 ` Brown, Aaron F
1 sibling, 1 reply; 6+ messages in thread
From: Ethan Zhao @ 2017-06-21 2:44 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Jeff Kirsher, intel-wired-lan, netdev, LKML
Gustavo,
The return value of ret_val seems used to check if the access to PHY/NVM
got its semaphore, generally speaking, it is needed for every PHY
access of this driver.
Reviewed-by: Ethan Zhao <ethan.zhao@oracle.com>
On Wed, Jun 21, 2017 at 5:22 AM, Gustavo A. R. Silva
<garsilva@embeddedor.com> wrote:
> Check return value from call to e1e_wphy(). This value is being
> checked during previous calls to function e1e_wphy() and it seems
> a check was missing here.
>
> Addresses-Coverity-ID: 1226905
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index 68ea8b4..d6d4ed7 100644
> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> @@ -2437,6 +2437,8 @@ static s32 e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw)
> if (hw->phy.revision < 2) {
> e1000e_phy_sw_reset(hw);
> ret_val = e1e_wphy(hw, MII_BMCR, 0x3140);
> + if (ret_val)
> + return ret_val;
> }
> }
>
> --
> 2.5.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: intel: e1000e: add check on e1e_wphy() return value
2017-06-21 2:44 ` Ethan Zhao
@ 2017-06-21 19:52 ` Gustavo A. R. Silva
2017-06-22 6:54 ` [Intel-wired-lan] " Neftin, Sasha
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-06-21 19:52 UTC (permalink / raw)
To: Ethan Zhao; +Cc: Jeff Kirsher, intel-wired-lan, netdev, LKML
Hi Ethan,
Quoting Ethan Zhao <ethan.kernel@gmail.com>:
> Gustavo,
>
> The return value of ret_val seems used to check if the access to PHY/NVM
> got its semaphore, generally speaking, it is needed for every PHY
> access of this driver.
>
> Reviewed-by: Ethan Zhao <ethan.zhao@oracle.com>
>
Thank you very much for the clarification.
> On Wed, Jun 21, 2017 at 5:22 AM, Gustavo A. R. Silva
> <garsilva@embeddedor.com> wrote:
>> Check return value from call to e1e_wphy(). This value is being
>> checked during previous calls to function e1e_wphy() and it seems
>> a check was missing here.
>>
>> Addresses-Coverity-ID: 1226905
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>> drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> index 68ea8b4..d6d4ed7 100644
>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> @@ -2437,6 +2437,8 @@ static s32
>> e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw)
>> if (hw->phy.revision < 2) {
>> e1000e_phy_sw_reset(hw);
>> ret_val = e1e_wphy(hw, MII_BMCR, 0x3140);
>> + if (ret_val)
>> + return ret_val;
>> }
>> }
>>
>> --
>> 2.5.0
>>
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH] net: intel: e1000e: add check on e1e_wphy() return value
2017-06-21 19:52 ` Gustavo A. R. Silva
@ 2017-06-22 6:54 ` Neftin, Sasha
0 siblings, 0 replies; 6+ messages in thread
From: Neftin, Sasha @ 2017-06-22 6:54 UTC (permalink / raw)
To: Gustavo A. R. Silva, Ethan Zhao; +Cc: netdev, intel-wired-lan, LKML
On 21/06/2017 22:52, Gustavo A. R. Silva wrote:
> Hi Ethan,
>
> Quoting Ethan Zhao <ethan.kernel@gmail.com>:
>
>> Gustavo,
>>
>> The return value of ret_val seems used to check if the access to
>> PHY/NVM
>> got its semaphore, generally speaking, it is needed for every PHY
>> access of this driver.
>>
>> Reviewed-by: Ethan Zhao <ethan.zhao@oracle.com>
>>
>
> Thank you very much for the clarification.
>
>> On Wed, Jun 21, 2017 at 5:22 AM, Gustavo A. R. Silva
>> <garsilva@embeddedor.com> wrote:
>>> Check return value from call to e1e_wphy(). This value is being
>>> checked during previous calls to function e1e_wphy() and it seems
>>> a check was missing here.
>>>
>>> Addresses-Coverity-ID: 1226905
>>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>>> ---
>>> drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> index 68ea8b4..d6d4ed7 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> @@ -2437,6 +2437,8 @@ static s32
>>> e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw)
>>> if (hw->phy.revision < 2) {
>>> e1000e_phy_sw_reset(hw);
>>> ret_val = e1e_wphy(hw, MII_BMCR, 0x3140);
>>> + if (ret_val)
>>> + return ret_val;
>>> }
>>> }
>>>
>>> --
>>> 2.5.0
>>>
>
> --
> Gustavo A. R. Silva
>
>
>
>
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
We will accept this patch.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [Intel-wired-lan] [PATCH] net: intel: e1000e: add check on e1e_wphy() return value
2017-06-20 21:22 ` [PATCH] net: intel: e1000e: add check on e1e_wphy() return value Gustavo A. R. Silva
2017-06-21 2:44 ` Ethan Zhao
@ 2017-06-27 1:21 ` Brown, Aaron F
1 sibling, 0 replies; 6+ messages in thread
From: Brown, Aaron F @ 2017-06-27 1:21 UTC (permalink / raw)
To: Gustavo A. R. Silva, Kirsher, Jeffrey T
Cc: netdev, intel-wired-lan, linux-kernel
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Gustavo A. R. Silva
> Sent: Tuesday, June 20, 2017 2:23 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org; Gustavo A. R. Silva <garsilva@embeddedor.com>
> Subject: [Intel-wired-lan] [PATCH] net: intel: e1000e: add check on
> e1e_wphy() return value
>
> Check return value from call to e1e_wphy(). This value is being
> checked during previous calls to function e1e_wphy() and it seems
> a check was missing here.
>
> Addresses-Coverity-ID: 1226905
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
> 1 file changed, 2 insertions(+)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-27 1:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-18 22:22 [net-intel-e1000e] question about value overwrite Gustavo A. R. Silva
2017-06-20 21:22 ` [PATCH] net: intel: e1000e: add check on e1e_wphy() return value Gustavo A. R. Silva
2017-06-21 2:44 ` Ethan Zhao
2017-06-21 19:52 ` Gustavo A. R. Silva
2017-06-22 6:54 ` [Intel-wired-lan] " Neftin, Sasha
2017-06-27 1:21 ` Brown, Aaron F
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).