Skip to contentSkip to conversation

Fix use of uninitialized field in mv643xx_eth

PATCH In review Gabriel Paubert 3 participants 6 messages Mar 23, 2007 – Mar 29, 2007

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Gabriel Paubert

author
#0 lore raw

PATCH 1/1 Fix use of uninitialized field in mv643xx_eth

In this driver, the default ethernet address is first set by by calling eth_port_uc_addr_get() which reads the relevant registers of the corresponding port as initially set by firmware. However that function used the port_num field accessed through the private area of net_dev before it was set.

The result was that one board I have ended up with the unicast address set to 00:00:00:00:00:00 (only port 1 is connected on this board). The problem appeared after commit 84dd619e4dc3b0b1c40dafd98c90fd950bce7bc5.

This patch fixes the bug by making eth_port_uc_get_addr() more similar to eth_port_uc_set_addr(), i.e., by using the port number as the first parameter instead of a pointer to struct net_device.

Signed-off-by Gabriel Paubert <[email protected]>

--

The minimal patch I first tried consisted in just moving mp->port_num to before the call to eth_port_uc_get_addr(). The other question is why the driver never gets the info from the device tree on this PPC board, but that's for another list despite the fact I lost some time looking for bugs in the OF interface before stumbling on this use of a field before it was initialized.

drivers/net/mv643xx_eth.c +5 -6
index 1ee27c3..ca459e0 100644
@@ -51,7 +51,7 @@
 #include "mv643xx_eth.h"
 
 /* Static function declarations */
-static void eth_port_uc_addr_get(struct net_device *dev,
+static void eth_port_uc_addr_get(unsigned int port_num, 
 						unsigned char *MacAddr);
 static void eth_port_set_multicast_list(struct net_device *);
 static void mv643xx_eth_port_enable_tx(unsigned int port_num,

@@ -1382,7 +1382,7 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 	port_num = pd->port_number;
 
 	/* set default config values */
-	eth_port_uc_addr_get(dev, dev->dev_addr);
+	eth_port_uc_addr_get(port_num, dev->dev_addr);
 	mp->rx_ring_size = MV643XX_ETH_PORT_DEFAULT_RECEIVE_QUEUE_SIZE;
 	mp->tx_ring_size = MV643XX_ETH_PORT_DEFAULT_TRANSMIT_QUEUE_SIZE;
 

@@ -1883,14 +1883,13 @@ static void eth_port_uc_addr_set(unsigned int eth_port_num,
  *	N/A.
  *
  */
-static void eth_port_uc_addr_get(struct net_device *dev, unsigned char *p_addr)
+static void eth_port_uc_addr_get(unsigned int port_num, unsigned char *p_addr)
 {
-	struct mv643xx_private *mp = netdev_priv(dev);
 	unsigned int mac_h;
 	unsigned int mac_l;
 
-	mac_h = mv_read(MV643XX_ETH_MAC_ADDR_HIGH(mp->port_num));
-	mac_l = mv_read(MV643XX_ETH_MAC_ADDR_LOW(mp->port_num));
+	mac_h = mv_read(MV643XX_ETH_MAC_ADDR_HIGH(port_num));
+	mac_l = mv_read(MV643XX_ETH_MAC_ADDR_LOW(port_num));
 
 	p_addr[0] = (mac_h >> 24) & 0xff;
 	p_addr[1] = (mac_h >> 16) & 0xff;
#1 lore raw
Quoted 5 lines · Gabriel Paubert

In this driver, the default ethernet address is first set by by calling eth_port_uc_addr_get() which reads the relevant registers of the corresponding port as initially set by firmware. However that function used the port_num field accessed through the private area of net_dev before it was set.

Gabriel, you're right. I introduced the bug and I'm sorry for your trouble.

Quoted 8 lines

The result was that one board I have ended up with the unicast address set to 00:00:00:00:00:00 (only port 1 is connected on this board). The problem appeared after commit 84dd619e4dc3b0b1c40dafd98c90fd950bce7bc5.

This patch fixes the bug by making eth_port_uc_get_addr() more similar to eth_port_uc_set_addr(), i.e., by using the port number as the first parameter instead of a pointer to struct net_device.

Quoted 6 lines
Signed-off-by Gabriel Paubert <[email protected]>

--

The minimal patch I first tried consisted in just moving mp->port_num to before the call to eth_port_uc_get_addr().

Hmm. That should have fixed it. I reproduced the problem here and this fixed it for me:

drivers/net/mv643xx_eth.c +1 -3
index 1ee27c3..643ea31 100644
@@ -1379,7 +1379,7 @@ #endif
 
 	spin_lock_init(&mp->lock);
 
-	port_num = pd->port_number;
+	port_num = mp->port_num = pd->port_number;
 
 	/* set default config values */
 	eth_port_uc_addr_get(dev, dev->dev_addr);

@@ -1411,8 +1411,6 @@ #endif
 	duplex = pd->duplex;
 	speed = pd->speed;
 
-	mp->port_num = port_num;
-
 	/* Hook up MII support for ethtool */
 	mp->mii.dev = dev;
 	mp->mii.mdio_read = mv643xx_mdio_read;

Would you please confirm that this fixes it for you? If so, I'll submit it upstream as coming from you, since you did all the work. OK?

Quoted 5 lines

The other question is why
the driver never gets the info from the device tree on this PPC board, but that's for another list despite the fact I lost some time looking for bugs in the OF interface before stumbling on this use of a field before it was initialized.

Probably just because the mac address in the hardware was correct and it didn't seem necessary to overwrite it.

Thank you,
-Dale

#2 lore raw

PATCH 1/2 mv643xx_eth: Fix use of uninitialized port_num field

In reply to Gabriel Paubert

From: Gabriel Paubert <[email protected]>

In this driver, the default ethernet address is first set by by calling eth_port_uc_addr_get() which reads the relevant registers of the corresponding port as initially set by firmware. However that function used the port_num field accessed through the private area of net_dev before it was set.

The result was that one board I have ended up with the unicast address set to 00:00:00:00:00:00 (only port 1 is connected on this board). The problem appeared after commit 84dd619e4dc3b0b1c40dafd98c90fd950bce7bc5.

This patch fixes the bug by setting mp->port_num prior to calling eth_port_uc_get_addr().

Signed-off-by Gabriel Paubert <[email protected]>
Signed-off-by Dale Farnsworth <[email protected]>

---

This fixes a serious bug and should expeditiously pushed upstream.

 drivers/net/mv643xx_eth.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/net/mv643xx_eth.c +1 -3
index 1ee27c3..643ea31 100644
@@ -1379,7 +1379,7 @@ #endif
 
 	spin_lock_init(&mp->lock);
 
-	port_num = pd->port_number;
+	port_num = mp->port_num = pd->port_number;
 
 	/* set default config values */
 	eth_port_uc_addr_get(dev, dev->dev_addr);

@@ -1411,8 +1411,6 @@ #endif
 	duplex = pd->duplex;
 	speed = pd->speed;
 
-	mp->port_num = port_num;
-
 	/* Hook up MII support for ethtool */
 	mp->mii.dev = dev;
 	mp->mii.mdio_read = mv643xx_mdio_read;
#3 lore raw

PATCH 2/2 mv643xx_eth: make eth_port_uc_addr_{get,set}() calls symmetric

From: Gabriel Paubert <[email protected]>

There is no good reason for the asymmetry in the parameters of eth_port_uc_addr_get() and eth_port_uc_addr_set(). Make them symmetric. Remove some gratuitous block comments while we're here.

Signed-off-by Gabriel Paubert <[email protected]>
Signed-off-by Dale Farnsworth <[email protected]>

---
This is a clean-up patch that needn't be rushed upstream. -Dale

 drivers/net/mv643xx_eth.c |   59 +++++++-----------------------------
 drivers/net/mv643xx_eth.h |    4 --
 2 files changed, 13 insertions(+), 50 deletions(-)
drivers/net/mv643xx_eth.c +13 -46
index 643ea31..f58d96e 100644
@@ -51,8 +51,8 @@ #include <asm/delay.h>
 #include "mv643xx_eth.h"
 
 /* Static function declarations */
-static void eth_port_uc_addr_get(struct net_device *dev,
-						unsigned char *MacAddr);
+static void eth_port_uc_addr_get(unsigned int port_num, unsigned char *p_addr);
+static void eth_port_uc_addr_set(unsigned int port_num, unsigned char *p_addr);
 static void eth_port_set_multicast_list(struct net_device *);
 static void mv643xx_eth_port_enable_tx(unsigned int port_num,
 						unsigned int queues);

@@ -1382,7 +1382,7 @@ #endif
 	port_num = mp->port_num = pd->port_number;
 
 	/* set default config values */
-	eth_port_uc_addr_get(dev, dev->dev_addr);
+	eth_port_uc_addr_get(port_num, dev->dev_addr);
 	mp->rx_ring_size = MV643XX_ETH_PORT_DEFAULT_RECEIVE_QUEUE_SIZE;
 	mp->tx_ring_size = MV643XX_ETH_PORT_DEFAULT_TRANSMIT_QUEUE_SIZE;
 

@@ -1826,26 +1826,9 @@ static void eth_port_start(struct net_de
 }
 
 /*
- * eth_port_uc_addr_set - This function Set the port Unicast address.
- *
- * DESCRIPTION:
- *		This function Set the port Ethernet MAC address.
- *
- * INPUT:
- *	unsigned int	eth_port_num	Port number.
- *	char *		p_addr		Address to be set
- *
- * OUTPUT:
- *	Set MAC address low and high registers. also calls
- *	eth_port_set_filter_table_entry() to set the unicast
- *	table with the proper information.
- *
- * RETURN:
- *	N/A.
- *
+ * eth_port_uc_addr_set - Write a MAC address into the port's hw registers
  */
-static void eth_port_uc_addr_set(unsigned int eth_port_num,
-							unsigned char *p_addr)
+static void eth_port_uc_addr_set(unsigned int port_num, unsigned char *p_addr)
 {
 	unsigned int mac_h;
 	unsigned int mac_l;

@@ -1855,40 +1838,24 @@ static void eth_port_uc_addr_set(unsigne
 	mac_h = (p_addr[0] << 24) | (p_addr[1] << 16) | (p_addr[2] << 8) |
 							(p_addr[3] << 0);
 
-	mv_write(MV643XX_ETH_MAC_ADDR_LOW(eth_port_num), mac_l);
-	mv_write(MV643XX_ETH_MAC_ADDR_HIGH(eth_port_num), mac_h);
+	mv_write(MV643XX_ETH_MAC_ADDR_LOW(port_num), mac_l);
+	mv_write(MV643XX_ETH_MAC_ADDR_HIGH(port_num), mac_h);
 
-	/* Accept frames of this address */
-	table = MV643XX_ETH_DA_FILTER_UNICAST_TABLE_BASE(eth_port_num);
+	/* Accept frames with this address */
+	table = MV643XX_ETH_DA_FILTER_UNICAST_TABLE_BASE(port_num);
 	eth_port_set_filter_table_entry(table, p_addr[5] & 0x0f);
 }
 
 /*
- * eth_port_uc_addr_get - This function retrieves the port Unicast address
- * (MAC address) from the ethernet hw registers.
- *
- * DESCRIPTION:
- *		This function retrieves the port Ethernet MAC address.
- *
- * INPUT:
- *	unsigned int	eth_port_num	Port number.
- *	char		*MacAddr	pointer where the MAC address is stored
- *
- * OUTPUT:
- *	Copy the MAC address to the location pointed to by MacAddr
- *
- * RETURN:
- *	N/A.
- *
+ * eth_port_uc_addr_get - Read the MAC address from the port's hw registers
  */
-static void eth_port_uc_addr_get(struct net_device *dev, unsigned char *p_addr)
+static void eth_port_uc_addr_get(unsigned int port_num, unsigned char *p_addr)
 {
-	struct mv643xx_private *mp = netdev_priv(dev);
 	unsigned int mac_h;
 	unsigned int mac_l;
 
-	mac_h = mv_read(MV643XX_ETH_MAC_ADDR_HIGH(mp->port_num));
-	mac_l = mv_read(MV643XX_ETH_MAC_ADDR_LOW(mp->port_num));
+	mac_h = mv_read(MV643XX_ETH_MAC_ADDR_HIGH(port_num));
+	mac_l = mv_read(MV643XX_ETH_MAC_ADDR_LOW(port_num));
 
 	p_addr[0] = (mac_h >> 24) & 0xff;
 	p_addr[1] = (mac_h >> 16) & 0xff;
drivers/net/mv643xx_eth.h +0 -4
index 7d4e90c..82f8c0c 100644
@@ -346,10 +346,6 @@ static void eth_port_init(struct mv643xx
 static void eth_port_reset(unsigned int eth_port_num);
 static void eth_port_start(struct net_device *dev);
 
-/* Port MAC address routines */
-static void eth_port_uc_addr_set(unsigned int eth_port_num,
-				 unsigned char *p_addr);
-
 /* PHY and MIB routines */
 static void ethernet_phy_reset(unsigned int eth_port_num);
Applied
#4 lore raw

Re: [PATCH 2/2] mv643xx_eth: make eth_port_uc_addr_{get,set}() calls symmetric

Quoted 6 lines · Dale Farnsworth

From: Gabriel Paubert <[email protected]>

There is no good reason for the asymmetry in the parameters of eth_port_uc_addr_get() and eth_port_uc_addr_set(). Make them symmetric. Remove some gratuitous block comments while we're here.

Quoted 8 lines
Signed-off-by Gabriel Paubert <[email protected]>
Signed-off-by Dale Farnsworth <[email protected]>

---
This is a clean-up patch that needn't be rushed upstream. -Dale

 drivers/net/mv643xx_eth.c |   59 +++++++-----------------------------
 drivers/net/mv643xx_eth.h |    4 --
 2 files changed, 13 insertions(+), 50 deletions(-)

applied to #upstream

#5 lore raw

Re: [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field

In reply to Dale Farnsworth
Quoted 12 lines · Dale Farnsworth

From: Gabriel Paubert <[email protected]>

In this driver, the default ethernet address is first set by by calling eth_port_uc_addr_get() which reads the relevant registers of the corresponding port as initially set by firmware. However that function used the port_num field accessed through the private area of net_dev before it was set.

The result was that one board I have ended up with the unicast address set to 00:00:00:00:00:00 (only port 1 is connected on this board). The problem appeared after commit 84dd619e4dc3b0b1c40dafd98c90fd950bce7bc5.

Quoted 6 lines

This patch fixes the bug by setting mp->port_num prior to calling eth_port_uc_get_addr().

Signed-off-by Gabriel Paubert <[email protected]>
Signed-off-by Dale Farnsworth <[email protected]>

---

applied #1 to upstream-fixes