* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).