Skip to contentSkip to conversation

fix signedness bug and memory leak in mscc driver

PATCH Superseded Gustavo A. R. Silva 2 participants 6 messages Oct 9, 2018 – Oct 10, 2018

Subsystems (2)
Network drivers Ethernet PHY

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

cover fix signedness bug and memory leak in mscc driver

This patchset aims to fix a signedness bug in function vsc85xx_downshift_get() and a memory leak in function vsc8574_config_pre_init().

Thanks

Gustavo A. R. Silva (2):
net: phy: mscc: fix signedness bug in vsc85xx_downshift_get
net: phy: mscc: fix memory leak in vsc8574_config_pre_init

 drivers/net/phy/mscc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 1/2 net: phy: mscc: fix signedness bug in vsc85xx_downshift_get

Currently, the error handling for the call to function phy_read_paged() doesn't work because *reg_val* is of type u16 (16 bits, unsigned), which makes it impossible for it to hold a value less than 0.

Fix this by changing the type of variable *reg_val* to int.

Addresses-Coverity-ID: 1473970 ("Unsigned compared against 0")

Fixes 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page functions")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/net/phy/mscc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/phy/mscc.c +1 -1
index bffe077..bff56c3 100644
@@ -522,7 +522,7 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
 
 static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
 {
-	u16 reg_val;
+	int reg_val;
 
 	reg_val = phy_read_paged(phydev, MSCC_PHY_PAGE_EXTENDED,
 				 MSCC_PHY_ACTIPHY_CNTL);
Signature · 1 line
2.7.4
Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH net-next 1/2] net: phy: mscc: fix signedness bug in vsc85xx_downshift_get

Hi Gustavo,

Quoted 5 lines · Gustavo A. R. Silva

Currently, the error handling for the call to function phy_read_paged() doesn't work because *reg_val* is of type u16 (16 bits, unsigned), which makes it impossible for it to hold a value less than 0.

Ah, rookie mistake! I should have run Coccinelle on the patches.

Quoted 4 lines

Fix this by changing the type of variable *reg_val* to int.

Addresses-Coverity-ID: 1473970 ("Unsigned compared against 0")

Fixes 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page functions")
Reviewed-by Quentin Schulz <[email protected]>

Thanks!
Quentin

author
#3 lore raw

PATCH 2/2 net: phy: mscc: fix memory leak in vsc8574_config_pre_init

In reply to Gustavo A. R. Silva

In case memory resources for *fw* were successfully allocated, release them before return.

Addresses-Coverity-ID: 1473968 ("Resource leak")

Fixes 00d70d8e0e78 ("net: phy: mscc: add support for VSC8574 PHY")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/net/phy/mscc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/phy/mscc.c +2 -2
index bff56c3..af9fb2d 100644
@@ -1292,7 +1292,7 @@ static int vsc8574_config_pre_init(struct phy_device *phydev)
 				dev_err(dev,
 					"%s: failed to assert reset of micro\n",
 					__func__);
-				return ret;
+				goto release_fw;
 			}
 		}
 	} else {

@@ -1338,7 +1338,7 @@ static int vsc8574_config_pre_init(struct phy_device *phydev)
 
 out:
 	phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD);
-
+release_fw:
 	release_firmware(fw);
 
 	return ret;
Signature · 1 line
2.7.4
#4 lore raw

Re: [PATCH net-next 2/2] net: phy: mscc: fix memory leak in vsc8574_config_pre_init

Hi Gustavo,

Quoted 24 lines · Gustavo A. R. Silva

In case memory resources for *fw* were successfully allocated, release them before return.

Addresses-Coverity-ID: 1473968 ("Resource leak")

Fixes 00d70d8e0e78 ("net: phy: mscc: add support for VSC8574 PHY")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/net/phy/mscc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/phy/mscc.c +1 -1
index bff56c3..af9fb2d 100644
@@ -1292,7 +1292,7 @@ static int vsc8574_config_pre_init(struct phy_device *phydev)
 				dev_err(dev,
 					"%s: failed to assert reset of micro\n",
 					__func__);
-				return ret;
+				goto release_fw;
 			}
 		}
 	} else {

drivers/net/phy/mscc.c static int vsc8574_config_pre_init(struct phy_device *phydev) · line 1342

Quoted 6 lines
drivers/net/phy/mscc.c +1 -1
@@ -1338,7 +1338,7 @@ static int vsc8574_config_pre_init(struct phy_device *phydev)
 
 out:
 	phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD);
-
+release_fw:
 	release_firmware(fw);

You can reuse the out goto label instead, I would like all functions in the driver to exit with the PHY set to access the standard page.

Thanks,
Quentin

author
#5 lore raw

Re: [PATCH net-next 2/2] net: phy: mscc: fix memory leak in vsc8574_config_pre_init

Hey Quentin,

Quoted 10 lines · Quentin Schulz

drivers/net/phy/mscc.c static int vsc8574_config_pre_init(struct phy_device *phydev) · line 1342

Quoted 6 lines

out:

drivers/net/phy/mscc.c +1 -1
 	phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD);
-
+release_fw:
 	release_firmware(fw);

You can reuse the out goto label instead, I would like all functions in the driver to exit with the PHY set to access the standard page.

Oh okay. I've got it.

I'll send v2 shortly.

Thanks for the feedback.
--
Gustavo