Re: [PATCH] Fix use of uninitialized field in mv643xx_eth

2007-03-23 Thread Dale Farnsworth 
On Fri, Mar 23, 2007 at 01:30:02PM +0100, Gabriel Paubert wrote:
 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.

 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().

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

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 1ee27c3..643ea31 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -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?

 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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Fix use of uninitialized field in mv643xx_eth

2007-03-23 Thread 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.  

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.


diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 1ee27c3..ca459e0 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -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;
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html