* [PATCH] Fix use of uninitialized field in mv643xx_eth
@ 2007-03-23 12:30 Gabriel Paubert
2007-03-23 15:38 ` Dale Farnsworth
2007-03-23 19:03 ` [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field Dale Farnsworth
0 siblings, 2 replies; 6+ messages in thread
From: Gabriel Paubert @ 2007-03-23 12:30 UTC (permalink / raw)
To: Dale Farnsworth, Manish Lachwani; +Cc: netdev
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 <paubert@iram.es>
--
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Fix use of uninitialized field in mv643xx_eth
2007-03-23 12:30 [PATCH] Fix use of uninitialized field in mv643xx_eth Gabriel Paubert
@ 2007-03-23 15:38 ` Dale Farnsworth
2007-03-23 19:03 ` [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field Dale Farnsworth
1 sibling, 0 replies; 6+ messages in thread
From: Dale Farnsworth @ 2007-03-23 15:38 UTC (permalink / raw)
To: Gabriel Paubert; +Cc: Manish Lachwani, netdev
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 <paubert@iram.es>
>
> --
>
> 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field
2007-03-23 12:30 [PATCH] Fix use of uninitialized field in mv643xx_eth Gabriel Paubert
2007-03-23 15:38 ` Dale Farnsworth
@ 2007-03-23 19:03 ` Dale Farnsworth
2007-03-23 19:07 ` [PATCH 2/2] mv643xx_eth: make eth_port_uc_addr_{get,set}() calls symmetric Dale Farnsworth
2007-03-28 6:19 ` [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field Jeff Garzik
1 sibling, 2 replies; 6+ messages in thread
From: Dale Farnsworth @ 2007-03-23 19:03 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Gabriel Paubert, Manish Lachwani, netdev
From: Gabriel Paubert <paubert@iram.es>
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 <paubert@iram.es>
Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
---
This fixes a serious bug and should expeditiously pushed upstream.
drivers/net/mv643xx_eth.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] mv643xx_eth: make eth_port_uc_addr_{get,set}() calls symmetric
2007-03-23 19:03 ` [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field Dale Farnsworth
@ 2007-03-23 19:07 ` Dale Farnsworth
2007-03-29 12:35 ` Jeff Garzik
2007-03-28 6:19 ` [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field Jeff Garzik
1 sibling, 1 reply; 6+ messages in thread
From: Dale Farnsworth @ 2007-03-23 19:07 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Gabriel Paubert, Manish Lachwani, netdev
From: Gabriel Paubert <paubert@iram.es>
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 <paubert@iram.es>
Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
---
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(-)
diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 643ea31..f58d96e 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -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;
diff --git a/drivers/net/mv643xx_eth.h b/drivers/net/mv643xx_eth.h
index 7d4e90c..82f8c0c 100644
--- a/drivers/net/mv643xx_eth.h
+++ b/drivers/net/mv643xx_eth.h
@@ -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);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] mv643xx_eth: make eth_port_uc_addr_{get,set}() calls symmetric
2007-03-23 19:07 ` [PATCH 2/2] mv643xx_eth: make eth_port_uc_addr_{get,set}() calls symmetric Dale Farnsworth
@ 2007-03-29 12:35 ` Jeff Garzik
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Garzik @ 2007-03-29 12:35 UTC (permalink / raw)
To: Dale Farnsworth; +Cc: Gabriel Paubert, Manish Lachwani, netdev
Dale Farnsworth wrote:
> From: Gabriel Paubert <paubert@iram.es>
>
> 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 <paubert@iram.es>
> Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field
2007-03-23 19:03 ` [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field Dale Farnsworth
2007-03-23 19:07 ` [PATCH 2/2] mv643xx_eth: make eth_port_uc_addr_{get,set}() calls symmetric Dale Farnsworth
@ 2007-03-28 6:19 ` Jeff Garzik
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Garzik @ 2007-03-28 6:19 UTC (permalink / raw)
To: Dale Farnsworth; +Cc: Gabriel Paubert, Manish Lachwani, netdev
Dale Farnsworth wrote:
> From: Gabriel Paubert <paubert@iram.es>
>
> 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 <paubert@iram.es>
> Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> ---
applied #1 to upstream-fixes
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-03-29 12:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-03-23 12:30 [PATCH] Fix use of uninitialized field in mv643xx_eth Gabriel Paubert
2007-03-23 15:38 ` Dale Farnsworth
2007-03-23 19:03 ` [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field Dale Farnsworth
2007-03-23 19:07 ` [PATCH 2/2] mv643xx_eth: make eth_port_uc_addr_{get,set}() calls symmetric Dale Farnsworth
2007-03-29 12:35 ` Jeff Garzik
2007-03-28 6:19 ` [PATCH 1/2] mv643xx_eth: Fix use of uninitialized port_num field Jeff Garzik
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.