Skip to contentSkip to conversation

question about value overwrite

net-intel-e1000e In review Gustavo A. R. Silva 4 participants 6 messages May 18, 2017 – Jun 27, 2017

Subsystems (3)
Networking Network drivers Intel Ethernet
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

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

drivers/net/ethernet/intel/e1000e/ich8lan.c +0 -0
@@ -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

author
#1 lore raw

PATCH 1/1 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 <[email protected]>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/net/ethernet/intel/e1000e/ich8lan.c +2 -0
index 68ea8b4..d6d4ed7 100644
@@ -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;
 		}
 	}
Signature · 1 line
2.5.0
Reviewed-by on v1 1/1
#2 lore raw

Re: [PATCH] net: intel: e1000e: add check on e1e_wphy() return value

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 <[email protected]>
Quoted 26 lines · 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 <[email protected]>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/net/ethernet/intel/e1000e/ich8lan.c +2 -1
index 68ea8b4..d6d4ed7 100644
@@ -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

author
#3 lore raw

Re: [PATCH] net: intel: e1000e: add check on e1e_wphy() return value

Hi Ethan,

Quoting Ethan Zhao <[email protected]>:

Quoted 8 lines

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 <[email protected]>

Thank you very much for the clarification.

Quoted 27 lines
Quoted 25 lines · 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 <[email protected]>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/net/ethernet/intel/e1000e/ich8lan.c +2 -0
index 68ea8b4..d6d4ed7 100644
@@ -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;
                }
        }
Quoted 3 lines
Quoted 3 lines

--
2.5.0

--
Gustavo A. R. Silva

#4 lore raw

Re: [Intel-wired-lan] [PATCH] net: intel: e1000e: add check on e1e_wphy() return value

Quoted 43 lines · Gustavo A. R. Silva

Hi Ethan,

Quoting Ethan Zhao <[email protected]>:

Quoted 9 lines

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 <[email protected]>

Thank you very much for the clarification.

Quoted 27 lines
Quoted 25 lines · 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 <[email protected]>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/net/ethernet/intel/e1000e/ich8lan.c +2 -0
index 68ea8b4..d6d4ed7 100644
@@ -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;
                }
        }
Quoted 15 lines
Quoted 3 lines
Quoted 3 lines
Signature · 1 line
2.5.0
Signature · 10 lines
Gustavo A. R. Silva





_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

We will accept this patch.

Tested-by on v1 1/1
#5 lore raw

RE: [Intel-wired-lan] [PATCH] net: intel: e1000e: add check on e1e_wphy() return value

In reply to Gustavo A. R. Silva
Quoted 13 lines

From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Gustavo A. R. Silva
Sent: Tuesday, June 20, 2017 2:23 PM
To: Kirsher, Jeffrey T <[email protected]>
Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; linux- kernel at vger.kernel.org; Gustavo A. R. Silva <[email protected]>
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.

Quoted 5 lines

Addresses-Coverity-ID: 1226905

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
 1 file changed, 2 insertions(+)
Tested-by Aaron Brown <[email protected]>