Skip to contentSkip to conversation

net: sxgbe: sxgbe_mdio_register() frees the bus

PATCH Applied Dan Carpenter 3 participants 4 messages Apr 1, 2014 – Apr 2, 2014

Subsystems (4)
Networking Network drivers Kernel janitors Samsung sxgbe drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/3 net: sxgbe: sxgbe_mdio_register() frees the bus

"err" is always zero at this point so we always unregister and free the mdio_bus before returning success. This seems like left over code and I have deleted it.

Fixes 1edb9ca69e8a ('net: sxgbe: add basic framework for Samsung 10Gb ethernet driver')
Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Untested.

drivers/net/ethernet/samsung/sxgbe/sxgbe_mdio.c +0 -7
index b0eb0a2..01af2cb 100644
@@ -219,13 +219,6 @@ int sxgbe_mdio_register(struct net_device *ndev)
 		}
 	}
 
-	if (!err) {
-		netdev_err(ndev, "PHY not found\n");
-		mdiobus_unregister(mdio_bus);
-		mdiobus_free(mdio_bus);
-		goto mdiobus_err;
-	}
-
 	priv->mii = mdio_bus;
 
 	return 0;
maintainer
#1 lore raw

From: Dan Carpenter <[email protected]>
Date: Tue, 1 Apr 2014 16:38:44 +0300

Quoted 11 lines
drivers/net/ethernet/samsung/sxgbe/sxgbe_mdio.c +0 -7
@@ -219,13 +219,6 @@ int sxgbe_mdio_register(struct net_device *ndev)
 		}
 	}
 
-	if (!err) {
-		netdev_err(ndev, "PHY not found\n");
-		mdiobus_unregister(mdio_bus);
-		mdiobus_free(mdio_bus);
-		goto mdiobus_err;
-	}
-

...

Review static void sxgbe_core_set_umac_addr(void __iomem *ioaddr, unsigned char *addr, · line 101

Quoted 13 lines
(quoted diff) +3 -3
@@ -93,9 +93,9 @@ static void sxgbe_core_set_umac_addr(void __iomem *ioaddr, unsigned char *addr,
 {
 	u32 high_word, low_word;
 
-	high_word = (addr[5] << 8) || (addr[4]);
-	low_word = ((addr[3] << 24) || (addr[2] << 16) ||
-		    (addr[1] << 8) || (addr[0]));
+	high_word = (addr[5] << 8) | (addr[4]);
+	low_word = (addr[3] << 24) | (addr[2] << 16) |
+		   (addr[1] << 8) | (addr[0]);
 	writel(high_word, ioaddr + SXGBE_CORE_ADD_HIGHOFFSET(reg_n));
 	writel(low_word, ioaddr + SXGBE_CORE_ADD_LOWOFFSET(reg_n));
 }

Nothing says "DRIVER NOT TESTED" like these two bugs.

The MDIO bus is always freed, and the MAC address is corrupted into a boolean value before being programmed into the hardware.

Frankly, this kind of stuff is unacceptable.

I cannot see how this driver can function successfully with these two errors, it looks simply impossible.

I'm not going to hide my feelings, this was a truly terrible driver submission. The amount of reviewing resources consumed during all of these iterations was huge, and it still went in with bugs like this.

It probably should have gone into staging.

maintainer
#2 lore raw

David Miller <[email protected]> :

Quoted 30 lines

From: Dan Carpenter <[email protected]>
Date: Tue, 1 Apr 2014 16:38:44 +0300

Quoted 11 lines
drivers/net/ethernet/samsung/sxgbe/sxgbe_mdio.c +0 -7
@@ -219,13 +219,6 @@ int sxgbe_mdio_register(struct net_device *ndev)
 		}
 	}
 
-	if (!err) {
-		netdev_err(ndev, "PHY not found\n");
-		mdiobus_unregister(mdio_bus);
-		mdiobus_free(mdio_bus);
-		goto mdiobus_err;
-	}
-

...

Review static void sxgbe_core_set_umac_addr(void __iomem · line 100

Quoted 12 lines
(quoted diff) +3 -3
@@ -93,9 +93,9 @@ static void sxgbe_core_set_umac_addr(void __iomem
 *ioaddr, unsigned char *addr,  {
 	u32 high_word, low_word;
 
-	high_word = (addr[5] << 8) || (addr[4]);
-	low_word = ((addr[3] << 24) || (addr[2] << 16) ||
-		    (addr[1] << 8) || (addr[0]));
+	high_word = (addr[5] << 8) | (addr[4]);
+	low_word = (addr[3] << 24) | (addr[2] << 16) |
+		   (addr[1] << 8) | (addr[0]);
 	writel(high_word, ioaddr + SXGBE_CORE_ADD_HIGHOFFSET(reg_n));
 	writel(low_word, ioaddr + SXGBE_CORE_ADD_LOWOFFSET(reg_n));  }

Nothing says "DRIVER NOT TESTED" like these two bugs.

Quoted 6 lines

The MDIO bus is always freed, and the MAC address is corrupted into a boolean value before being programmed into the hardware.

Frankly, this kind of stuff is unacceptable.

I cannot see how this driver can function successfully with these two

errors, it

Quoted 3 lines

looks simply impossible.

I'm not going to hide my feelings, this was a truly terrible driver

submission.

Quoted 1 line

The amount of reviewing resources consumed during all of these iterations

was

Quoted 3 lines

huge, and it still went in with bugs like this.

It probably should have gone into staging.

Oops, my apologies. mdio err path and logical and bitwise for mac address are my mistake I didn't tested carefully (maybe tested with previous version) because I couldn't much time to test it due to facing merge window. sorry about that...

For logical and bitwise OR for mac address, actually I couldn't catch since "eth_hw_addr_random" is used.. however it is also mistake.

I apology again.