Message ID | 20140401133844.GA11536@mwanda |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Dan Carpenter <dan.carpenter@oracle.com> Date: Tue, 1 Apr 2014 16:38:44 +0300 > @@ -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; > - } > - ... > @@ -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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Dan Carpenter <dan.carpenter@oracle.com> Date: Tue, 1 Apr 2014 16:38:44 +0300 > "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 <dan.carpenter@oracle.com> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller <davem@davemloft.net> : > From: Dan Carpenter <dan.carpenter@oracle.com> > Date: Tue, 1 Apr 2014 16:38:44 +0300 > > > @@ -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; > > - } > > - > ... > > @@ -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. 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_mdio.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_mdio.c index b0eb0a2..01af2cb 100644 --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_mdio.c +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_mdio.c @@ -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;
"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 <dan.carpenter@oracle.com> --- Static analysis. Untested. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html