hotplug e1000 failed after 32 times

hotplug e1000 failed after 32 times

[Li Shaohua]

[-- Attachment #1: Type: text/plain, Size: 3541 bytes --]

Hi,
I'm testing a hotplug driver. In my test, I will hot add/remove an e1000
NIC frequently. The result is my hot add failed after 32 times hotadd.
After looking at the code of e1000 driver, I found
e1000_adapter->bd_number has maxium limitation of 32, and it increased
one every hot add. Looks like the remove driver routine didn't free the
'bd_number', so hot add failed after 32 times. Below patch fixes this
issue.

thanks,
Shaohua

Signed-off-by Li Shaohua <shaohua.li@intel.com>
===== drivers/net/e1000/e1000_main.c 1.134 vs edited =====
--- 1.134/drivers/net/e1000/e1000_main.c	2004-09-13 07:52:48 +08:00
+++ edited/drivers/net/e1000/e1000_main.c	2004-09-17 12:01:08 +08:00
@@ -180,6 +180,8 @@ struct notifier_block e1000_notifier_reb
 /* Exported from other modules */
 
 extern void e1000_check_options(struct e1000_adapter *adapter);
+extern int e1000_alloc_bd_number(void);
+extern void e1000_free_bd_number(int);
 
 static struct pci_driver e1000_driver = {
 	.name     = e1000_driver_name,
@@ -380,7 +382,6 @@ e1000_probe(struct pci_dev *pdev,
 {
 	struct net_device *netdev;
 	struct e1000_adapter *adapter;
-	static int cards_found = 0;
 	unsigned long mmio_start;
 	int mmio_len;
 	int pci_using_dac;
@@ -471,7 +472,7 @@ e1000_probe(struct pci_dev *pdev,
 	netdev->mem_end = mmio_start + mmio_len;
 	netdev->base_addr = adapter->hw.io_base;
 
-	adapter->bd_number = cards_found;
+	adapter->bd_number = e1000_alloc_bd_number();;
 
 	/* setup the private structure */
 
@@ -590,13 +591,13 @@ e1000_probe(struct pci_dev *pdev,
 	if((err = register_netdevice(netdev)))
 		goto err_register;
 
-	cards_found++;
 	rtnl_unlock();
 	return 0;
 
 err_register:
 err_sw_init:
 err_eeprom:
+	e1000_free_bd_number(adapter->bd_number);
 	iounmap(adapter->hw.hw_addr);
 err_ioremap:
 err_free_unlock:
@@ -638,6 +639,7 @@ e1000_remove(struct pci_dev *pdev)
 	e1000_phy_hw_reset(&adapter->hw);
 
 	iounmap(adapter->hw.hw_addr);
+	e1000_free_bd_number(adapter->bd_number);
 	pci_release_regions(pdev);
 
 	free_netdev(netdev);
===== drivers/net/e1000/e1000_param.c 1.31 vs edited =====
--- 1.31/drivers/net/e1000/e1000_param.c	2004-08-29 06:55:39 +08:00
+++ edited/drivers/net/e1000/e1000_param.c	2004-09-17 12:37:41 +08:00
@@ -56,7 +56,7 @@
  */
 
 #define E1000_PARAM(X, S) \
-static const int __devinitdata X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT;
\
+static int __devinitdata X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT; \
 MODULE_PARM(X, "1-" __MODULE_STRING(E1000_MAX_NIC) "i"); \
 MODULE_PARM_DESC(X, S);
 
@@ -703,3 +703,40 @@ e1000_check_copper_options(struct e1000_
 	}
 }
 
+#define BD_NUMBER_BITS (E1000_MAX_NIC + 1)
+#if BITS_PER_LONG > BD_NUMBER_BITS
+static unsigned long bd_number_bits;
+#else
+static unsigned long bd_number_bits[(E1000_MAX_NIC * 2)/BITS_PER_LONG];
+#endif
+
+int e1000_alloc_bd_number(void)
+{
+	int ret;
+
+	ret = find_first_zero_bit(&bd_number_bits, BD_NUMBER_BITS);
+	if (ret >= E1000_MAX_NIC)
+		ret = E1000_MAX_NIC;
+	__set_bit(ret, &bd_number_bits);
+	return ret;
+}
+
+void e1000_free_bd_number(int bd_number)
+{
+	if ((bd_number < 0) || (bd_number > E1000_MAX_NIC) ||
+		!test_bit(bd_number, &bd_number_bits))
+		return;
+	TxDescriptors[bd_number] = 
+	RxDescriptors[bd_number] = 
+	Speed[bd_number] =
+	Duplex[bd_number] =
+	AutoNeg[bd_number] =
+	FlowControl[bd_number] =
+	XsumRX[bd_number] =
+	TxIntDelay[bd_number] =
+	TxAbsIntDelay[bd_number] =
+	RxIntDelay[bd_number] =
+	RxAbsIntDelay[bd_number] =
+	InterruptThrottleRate[bd_number] = OPTION_UNSET;
+	__clear_bit(bd_number, &bd_number_bits);
+}


[-- Attachment #2: e1000.patch --]
[-- Type: text/x-patch, Size: 3055 bytes --]

===== drivers/net/e1000/e1000_main.c 1.134 vs edited =====
--- 1.134/drivers/net/e1000/e1000_main.c	2004-09-13 07:52:48 +08:00
+++ edited/drivers/net/e1000/e1000_main.c	2004-09-17 12:01:08 +08:00
@@ -180,6 +180,8 @@ struct notifier_block e1000_notifier_reb
 /* Exported from other modules */
 
 extern void e1000_check_options(struct e1000_adapter *adapter);
+extern int e1000_alloc_bd_number(void);
+extern void e1000_free_bd_number(int);
 
 static struct pci_driver e1000_driver = {
 	.name     = e1000_driver_name,
@@ -380,7 +382,6 @@ e1000_probe(struct pci_dev *pdev,
 {
 	struct net_device *netdev;
 	struct e1000_adapter *adapter;
-	static int cards_found = 0;
 	unsigned long mmio_start;
 	int mmio_len;
 	int pci_using_dac;
@@ -471,7 +472,7 @@ e1000_probe(struct pci_dev *pdev,
 	netdev->mem_end = mmio_start + mmio_len;
 	netdev->base_addr = adapter->hw.io_base;
 
-	adapter->bd_number = cards_found;
+	adapter->bd_number = e1000_alloc_bd_number();;
 
 	/* setup the private structure */
 
@@ -590,13 +591,13 @@ e1000_probe(struct pci_dev *pdev,
 	if((err = register_netdevice(netdev)))
 		goto err_register;
 
-	cards_found++;
 	rtnl_unlock();
 	return 0;
 
 err_register:
 err_sw_init:
 err_eeprom:
+	e1000_free_bd_number(adapter->bd_number);
 	iounmap(adapter->hw.hw_addr);
 err_ioremap:
 err_free_unlock:
@@ -638,6 +639,7 @@ e1000_remove(struct pci_dev *pdev)
 	e1000_phy_hw_reset(&adapter->hw);
 
 	iounmap(adapter->hw.hw_addr);
+	e1000_free_bd_number(adapter->bd_number);
 	pci_release_regions(pdev);
 
 	free_netdev(netdev);
===== drivers/net/e1000/e1000_param.c 1.31 vs edited =====
--- 1.31/drivers/net/e1000/e1000_param.c	2004-08-29 06:55:39 +08:00
+++ edited/drivers/net/e1000/e1000_param.c	2004-09-17 12:37:41 +08:00
@@ -56,7 +56,7 @@
  */
 
 #define E1000_PARAM(X, S) \
-static const int __devinitdata X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT; \
+static int __devinitdata X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT; \
 MODULE_PARM(X, "1-" __MODULE_STRING(E1000_MAX_NIC) "i"); \
 MODULE_PARM_DESC(X, S);
 
@@ -703,3 +703,40 @@ e1000_check_copper_options(struct e1000_
 	}
 }
 
+#define BD_NUMBER_BITS (E1000_MAX_NIC + 1)
+#if BITS_PER_LONG > BD_NUMBER_BITS
+static unsigned long bd_number_bits;
+#else
+static unsigned long bd_number_bits[(E1000_MAX_NIC * 2)/BITS_PER_LONG];
+#endif
+
+int e1000_alloc_bd_number(void)
+{
+	int ret;
+
+	ret = find_first_zero_bit(&bd_number_bits, BD_NUMBER_BITS);
+	if (ret >= E1000_MAX_NIC)
+		ret = E1000_MAX_NIC;
+	__set_bit(ret, &bd_number_bits);
+	return ret;
+}
+
+void e1000_free_bd_number(int bd_number)
+{
+	if ((bd_number < 0) || (bd_number > E1000_MAX_NIC) ||
+		!test_bit(bd_number, &bd_number_bits))
+		return;
+	TxDescriptors[bd_number] = 
+	RxDescriptors[bd_number] = 
+	Speed[bd_number] =
+	Duplex[bd_number] =
+	AutoNeg[bd_number] =
+	FlowControl[bd_number] =
+	XsumRX[bd_number] =
+	TxIntDelay[bd_number] =
+	TxAbsIntDelay[bd_number] =
+	RxIntDelay[bd_number] =
+	RxAbsIntDelay[bd_number] =
+	InterruptThrottleRate[bd_number] = OPTION_UNSET;
+	__clear_bit(bd_number, &bd_number_bits);
+}

Re: hotplug e1000 failed after 32 times

[Andrew Morton]

Li Shaohua <shaohua.li@intel.com> wrote:
>
> I'm testing a hotplug driver. In my test, I will hot add/remove an e1000
>  NIC frequently. The result is my hot add failed after 32 times hotadd.
>  After looking at the code of e1000 driver, I found
>  e1000_adapter->bd_number has maxium limitation of 32, and it increased
>  one every hot add. Looks like the remove driver routine didn't free the
>  'bd_number', so hot add failed after 32 times. Below patch fixes this
>  issue.

Yeah.  I think you'll find that damn near every net driver in the kernel
has this problem.  I think it would be better to create a little suite of
library functions in net/core/dev.c to handle this situation.

Maybe something like

struct net_boards {
	struct idr idr;
	int max_boards;
}

void net_boards_init(struct net_boards *net_boards, int max_boards);
int net_board_alloc(struct net_boards *net_boards);
int net_boards_free(struct net_boards *net_boards, int board_no);

(I wonder where the locking should be performed?)

This is a pretty thin wrapper around the idr code and actually is quite
generic and has nothing to do with networking so you might end up deciding
to rename things and to move the code into idr.c

> -	adapter->bd_number = cards_found;
> +	adapter->bd_number = e1000_alloc_bd_number();;

Extra semicolon.

> +	TxDescriptors[bd_number] = 
> +	RxDescriptors[bd_number] = 
> +	Speed[bd_number] =
> +	Duplex[bd_number] =
> +	AutoNeg[bd_number] =
> +	FlowControl[bd_number] =
> +	XsumRX[bd_number] =
> +	TxIntDelay[bd_number] =
> +	TxAbsIntDelay[bd_number] =
> +	RxIntDelay[bd_number] =
> +	RxAbsIntDelay[bd_number] =
> +	InterruptThrottleRate[bd_number] = OPTION_UNSET;

Unpopular coding style.  Please just do

	RxAbsIntDelay[bd_number] = OPTION_UNSET;
	InterruptThrottleRate[bd_number] = OPTION_UNSET;

etc.

Re: hotplug e1000 failed after 32 times

[Li Shaohua]

On Fri, 2004-09-17 at 13:14, Andrew Morton wrote:
> Li Shaohua <shaohua.li@intel.com> wrote:
> >
> > I'm testing a hotplug driver. In my test, I will hot add/remove an e1000
> >  NIC frequently. The result is my hot add failed after 32 times hotadd.
> >  After looking at the code of e1000 driver, I found
> >  e1000_adapter->bd_number has maxium limitation of 32, and it increased
> >  one every hot add. Looks like the remove driver routine didn't free the
> >  'bd_number', so hot add failed after 32 times. Below patch fixes this
> >  issue.
> 
> Yeah.  I think you'll find that damn near every net driver in the kernel
> has this problem.  I think it would be better to create a little suite of
> library functions in net/core/dev.c to handle this situation.
Thanks Andrew. That makes sense. I will add as you said.

Thanks,
Shaohua

Re: hotplug e1000 failed after 32 times

[Li Shaohua]

[-- Attachment #1: Type: text/plain, Size: 3793 bytes --]

On Fri, 2004-09-17 at 13:14, Andrew Morton wrote:
> Li Shaohua <shaohua.li@intel.com> wrote:
> >
> > I'm testing a hotplug driver. In my test, I will hot add/remove an e1000
> >  NIC frequently. The result is my hot add failed after 32 times hotadd.
> >  After looking at the code of e1000 driver, I found
> >  e1000_adapter->bd_number has maxium limitation of 32, and it increased
> >  one every hot add. Looks like the remove driver routine didn't free the
> >  'bd_number', so hot add failed after 32 times. Below patch fixes this
> >  issue.
> 
> Yeah.  I think you'll find that damn near every net driver in the kernel
> has this problem.  I think it would be better to create a little suite of
> library functions in net/core/dev.c to handle this situation.
<snip>
Here is the patch adding the API as you suggested.

Thanks,
Shaohua


Signed-off-by: Li Shaohua <shaohua.li@intel.com>
===== lib/idr.c 1.10 vs edited =====
--- 1.10/lib/idr.c	2004-08-23 16:14:51 +08:00
+++ edited/lib/idr.c	2004-09-17 15:53:47 +08:00
@@ -39,8 +39,10 @@ static struct idr_layer *alloc_layer(str
 	struct idr_layer *p;
 
 	spin_lock(&idp->lock);
-	if (!(p = idp->id_free))
+	if (!(p = idp->id_free)) {
+		spin_unlock(&idp->lock);
 		return NULL;
+	}
 	idp->id_free = p->ary[0];
 	idp->id_free_cnt--;
 	p->ary[0] = NULL;
===== include/linux/netdevice.h 1.89 vs edited =====
--- 1.89/include/linux/netdevice.h	2004-09-13 07:52:49 +08:00
+++ edited/include/linux/netdevice.h	2004-09-17 15:54:41 +08:00
@@ -154,6 +154,7 @@ enum {
 
 #include <linux/cache.h>
 #include <linux/skbuff.h>
+#include <linux/idr.h>
 
 struct neighbour;
 struct neigh_parms;
@@ -503,6 +504,16 @@ static inline void *netdev_priv(struct n
  * if set prior to registration will cause a symlink during
initialization.
  */
 #define SET_NETDEV_DEV(net, pdev)	((net)->class_dev.dev = (pdev))
+
+struct net_boards {
+	struct idr idr;
+	int max_boards;
+	spinlock_t lock;
+};
+
+void net_boards_init(struct net_boards *board, int max_boards);
+int net_boards_alloc(struct net_boards *board);
+int net_boards_free(struct net_boards *board, int board_no);
 
 struct packet_type {
 	unsigned short		type;	/* This is really htons(ether_type).	*/
===== net/core/dev.c 1.165 vs edited =====
--- 1.165/net/core/dev.c	2004-09-13 07:23:46 +08:00
+++ edited/net/core/dev.c	2004-09-17 15:52:43 +08:00
@@ -3220,6 +3220,53 @@ static int dev_cpu_callback(struct notif
 }
 #endif /* CONFIG_HOTPLUG_CPU */
 
+void net_boards_init(struct net_boards *board, int max_boards)
+{
+	if (!board || (max_boards < 0))
+		return;
+	idr_init(&board->idr);
+	board->max_boards = max_boards;
+	spin_lock_init(&board->lock);
+}
+
+/*
+ * return -1 on error, >= 0 on success
+ */
+int net_boards_alloc(struct net_boards *board)
+{
+	int ret, error;
+
+	if (!board)
+		return -1;
+retry:
+	if (idr_pre_get(&board->idr, GFP_KERNEL) == 0)
+		return -1;
+
+	spin_lock(&board->lock);
+	error = idr_get_new(&board->idr, NULL, &ret);
+	spin_unlock(&board->lock);
+	if (error == -EAGAIN)
+		goto retry;
+	else if (error)
+		return -1;
+	if (ret > board->max_boards) {
+		spin_lock(&board->lock);
+		idr_remove(&board->idr, ret);
+		spin_unlock(&board->lock);
+		return -1;
+	}
+	return ret;
+}
+
+int net_boards_free(struct net_boards *board, int board_no)
+{
+	if ((board_no < 0) || (board_no > board->max_boards))
+		return -1;
+	spin_lock(&board->lock);
+	idr_remove(&board->idr, board_no);
+	spin_unlock(&board->lock);
+	return 0;
+}
 
 /*
  *	Initialize the DEV module. At boot time this walks the device list
and
@@ -3296,6 +3343,9 @@ out:
 
 subsys_initcall(net_dev_init);
 
+EXPORT_SYMBOL(net_boards_init);
+EXPORT_SYMBOL(net_boards_alloc);
+EXPORT_SYMBOL(net_boards_free);
 EXPORT_SYMBOL(__dev_get);
 EXPORT_SYMBOL(__dev_get_by_flags);
 EXPORT_SYMBOL(__dev_get_by_index);



[-- Attachment #2: nic.patch --]
[-- Type: text/x-patch, Size: 2901 bytes --]

===== lib/idr.c 1.10 vs edited =====
--- 1.10/lib/idr.c	2004-08-23 16:14:51 +08:00
+++ edited/lib/idr.c	2004-09-17 15:53:47 +08:00
@@ -39,8 +39,10 @@ static struct idr_layer *alloc_layer(str
 	struct idr_layer *p;
 
 	spin_lock(&idp->lock);
-	if (!(p = idp->id_free))
+	if (!(p = idp->id_free)) {
+		spin_unlock(&idp->lock);
 		return NULL;
+	}
 	idp->id_free = p->ary[0];
 	idp->id_free_cnt--;
 	p->ary[0] = NULL;
===== include/linux/netdevice.h 1.89 vs edited =====
--- 1.89/include/linux/netdevice.h	2004-09-13 07:52:49 +08:00
+++ edited/include/linux/netdevice.h	2004-09-17 15:54:41 +08:00
@@ -154,6 +154,7 @@ enum {
 
 #include <linux/cache.h>
 #include <linux/skbuff.h>
+#include <linux/idr.h>
 
 struct neighbour;
 struct neigh_parms;
@@ -503,6 +504,16 @@ static inline void *netdev_priv(struct n
  * if set prior to registration will cause a symlink during initialization.
  */
 #define SET_NETDEV_DEV(net, pdev)	((net)->class_dev.dev = (pdev))
+
+struct net_boards {
+	struct idr idr;
+	int max_boards;
+	spinlock_t lock;
+};
+
+void net_boards_init(struct net_boards *board, int max_boards);
+int net_boards_alloc(struct net_boards *board);
+int net_boards_free(struct net_boards *board, int board_no);
 
 struct packet_type {
 	unsigned short		type;	/* This is really htons(ether_type).	*/
===== net/core/dev.c 1.165 vs edited =====
--- 1.165/net/core/dev.c	2004-09-13 07:23:46 +08:00
+++ edited/net/core/dev.c	2004-09-17 15:52:43 +08:00
@@ -3220,6 +3220,53 @@ static int dev_cpu_callback(struct notif
 }
 #endif /* CONFIG_HOTPLUG_CPU */
 
+void net_boards_init(struct net_boards *board, int max_boards)
+{
+	if (!board || (max_boards < 0))
+		return;
+	idr_init(&board->idr);
+	board->max_boards = max_boards;
+	spin_lock_init(&board->lock);
+}
+
+/*
+ * return -1 on error, >= 0 on success
+ */
+int net_boards_alloc(struct net_boards *board)
+{
+	int ret, error;
+
+	if (!board)
+		return -1;
+retry:
+	if (idr_pre_get(&board->idr, GFP_KERNEL) == 0)
+		return -1;
+
+	spin_lock(&board->lock);
+	error = idr_get_new(&board->idr, NULL, &ret);
+	spin_unlock(&board->lock);
+	if (error == -EAGAIN)
+		goto retry;
+	else if (error)
+		return -1;
+	if (ret > board->max_boards) {
+		spin_lock(&board->lock);
+		idr_remove(&board->idr, ret);
+		spin_unlock(&board->lock);
+		return -1;
+	}
+	return ret;
+}
+
+int net_boards_free(struct net_boards *board, int board_no)
+{
+	if ((board_no < 0) || (board_no > board->max_boards))
+		return -1;
+	spin_lock(&board->lock);
+	idr_remove(&board->idr, board_no);
+	spin_unlock(&board->lock);
+	return 0;
+}
 
 /*
  *	Initialize the DEV module. At boot time this walks the device list and
@@ -3296,6 +3343,9 @@ out:
 
 subsys_initcall(net_dev_init);
 
+EXPORT_SYMBOL(net_boards_init);
+EXPORT_SYMBOL(net_boards_alloc);
+EXPORT_SYMBOL(net_boards_free);
 EXPORT_SYMBOL(__dev_get);
 EXPORT_SYMBOL(__dev_get_by_flags);
 EXPORT_SYMBOL(__dev_get_by_index);

Re: hotplug e1000 failed after 32 times

[Andrew Morton]

Li Shaohua <shaohua.li@intel.com> wrote:
>
> On Fri, 2004-09-17 at 13:14, Andrew Morton wrote:
> > Li Shaohua <shaohua.li@intel.com> wrote:
> > >
> > > I'm testing a hotplug driver. In my test, I will hot add/remove an e1000
> > >  NIC frequently. The result is my hot add failed after 32 times hotadd.
> > >  After looking at the code of e1000 driver, I found
> > >  e1000_adapter->bd_number has maxium limitation of 32, and it increased
> > >  one every hot add. Looks like the remove driver routine didn't free the
> > >  'bd_number', so hot add failed after 32 times. Below patch fixes this
> > >  issue.
> > 
> > Yeah.  I think you'll find that damn near every net driver in the kernel
> > has this problem.  I think it would be better to create a little suite of
> > library functions in net/core/dev.c to handle this situation.
> <snip>
> Here is the patch adding the API as you suggested.

Looks pretty sane here.  Your email client did wordwrap the patch, so
please fix that up henceforth.

> ===== lib/idr.c 1.10 vs edited =====
> --- 1.10/lib/idr.c	2004-08-23 16:14:51 +08:00
> +++ edited/lib/idr.c	2004-09-17 15:53:47 +08:00
> @@ -39,8 +39,10 @@ static struct idr_layer *alloc_layer(str
>  	struct idr_layer *p;
>  
>  	spin_lock(&idp->lock);
> -	if (!(p = idp->id_free))
> +	if (!(p = idp->id_free)) {
> +		spin_unlock(&idp->lock);
>  		return NULL;
> +	}
>  	idp->id_free = p->ary[0];
>  	idp->id_free_cnt--;
>  	p->ary[0] = NULL;

ow.  I've split this fix into a separate patch.  Please leave this hunk out
of future patches.


> +struct net_boards {
> +	struct idr idr;
> +	int max_boards;
> +	spinlock_t lock;
> +};

It would be more convenient if we had a compile-time constructor for this
structure.  So device drivers can do:

static struct net_boards my_net_boards = NET_BOARDS_INIT(my_net_boards);

or

static DEFINE_NET_BOARDS(my_net_boards);

We may not need to retain net_boards_init() if we have NET_BOARDS_INIT().

> +void net_boards_init(struct net_boards *board, int max_boards)
> +{
> +	if (!board || (max_boards < 0))
> +		return;
> +	idr_init(&board->idr);
> +	board->max_boards = max_boards;
> +	spin_lock_init(&board->lock);
> +}

Even though nothing here can actually fail, it's probably best to make this
return an integer error code, in case we add something new in the future.

You can remove that initial check - if someone passes in a null pointer or
a negative max_boards then they'll work out that something went wrong soon
enough.

I'd suggest that max_boards be made unsigned throughout the whole patch.

> +/*
> + * return -1 on error, >= 0 on success
> + */
> +int net_boards_alloc(struct net_boards *board)
> +{
> +	int ret, error;
> +
> +	if (!board)
> +		return -1;
> +retry:
> +	if (idr_pre_get(&board->idr, GFP_KERNEL) == 0)
> +		return -1;

-ENOMEM

> +	spin_lock(&board->lock);
> +	error = idr_get_new(&board->idr, NULL, &ret);
> +	spin_unlock(&board->lock);
> +	if (error == -EAGAIN)
> +		goto retry;
> +	else if (error)
> +		return -1;

propagate `error' back to the caller

> +	if (ret > board->max_boards) {
> +		spin_lock(&board->lock);
> +		idr_remove(&board->idr, ret);
> +		spin_unlock(&board->lock);
> +		return -1;

-ENOSPC, maybe?

> +int net_boards_free(struct net_boards *board, int board_no)
> +{
> +	if ((board_no < 0) || (board_no > board->max_boards))
> +		return -1;

remove this check.

Re: hotplug e1000 failed after 32 times

[Jeff Garzik]

All this work and code for such an uncommon case??

First off, any settings which are _indexed_ are almost guaranteed 
broken.  For reasons as we see here, and others.  Any 
"setting_foo[board_number]" should be found and eliminated instead.

Even if you allocate and free board numbers as described, how precisely 
do you propose to predict which settings belong to which hotplugged 
board?  Look at the problem, and you realize that the board<->setting 
association becomes effectively _random_ for any adapter not present at 
modprobe time.

The best model is to set these settings via netlink/ethtool after 
registering the interface, but before bringing it up.

	Jeff

Re: hotplug e1000 failed after 32 times

[Jonathan Lundell]

At 7:34 PM -0400 9/17/04, Jeff Garzik wrote:
>Even if you allocate and free board numbers as described, how 
>precisely do you propose to predict which settings belong to which 
>hotplugged board?  Look at the problem, and you realize that the 
>board<->setting association becomes effectively _random_ for any 
>adapter not present at modprobe time.
>
>The best model is to set these settings via netlink/ethtool after 
>registering the interface, but before bringing it up.

It's certainly true that the driver-local index isn't usable for the 
association (not even if one remembers, say, the slot association, 
since a card can presumably be moved).

Out of curiosity, though, isn't there a residual related problem, in 
that a reinserted card gets a new eth# as well? Not insurmountable, I 
suppose, but a bitch to automate.
-- 
/Jonathan Lundell.

Re: hotplug e1000 failed after 32 times

[Dr. David Alan Gilbert]

* Jonathan Lundell (linux@lundell-bros.com) wrote:

> Out of curiosity, though, isn't there a residual related problem, in 
> that a reinserted card gets a new eth# as well? Not insurmountable, I 
> suppose, but a bitch to automate.

I do wonder why the eth# still gets exported to users - its a royal
pain when you have multiple cards.  I guess naming by mac address
isn't ideal either when you want to hot swap one!  Naming by
pci slot would be kind of nice.

(I sometimes wish ether cards would have a good old fashioned dil
switch on so you could set an ID).

Dave
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    | Running GNU/Linux on Alpha,68K| Happy  \ 
\ gro.gilbert @ treblig.org | MIPS,x86,ARM,SPARC,PPC & HPPA | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: hotplug e1000 failed after 32 times

[Jonathan Lundell]

At 5:50 PM +0100 9/19/04, Dr. David Alan Gilbert wrote:
>* Jonathan Lundell (linux@lundell-bros.com) wrote:
>
>>  Out of curiosity, though, isn't there a residual related problem, in
>>  that a reinserted card gets a new eth# as well? Not insurmountable, I
>>  suppose, but a bitch to automate.
>
>I do wonder why the eth# still gets exported to users - its a royal
>pain when you have multiple cards.  I guess naming by mac address
>isn't ideal either when you want to hot swap one!  Naming by
>pci slot would be kind of nice.

It's a little tricky doing that, since PCI buses get moved around 
dynamically as well. A typical quad Ethernet board (Intel's quad gig, 
for example) has a bridge onboard that creates an internal PCI bus. 
PCI numbers by bus:device:function. It happens that device tends to 
correspond to slot, but that's not at all necessary. And of course we 
can't assume that all systems use PCI buses.

In our systems, where we have lots of ports, we rename our eth's 
along these lines: eth1a, eth1b, eth2a, eth3a, where 1,2,3 are slots 
(we use 0 for motherboard ports) and a,b,c,d are ports on the card.

But doing that requires a per-system-type configuration file that 
describes how to determine which device is in which slot, and that's 
a little tricky because of the way PCI buses get renumbered.

MAC address makes a lousy port name, IMO, since the user typically 
has no way of knowing which port has which address. (And I'm reminded 
of the Solaris convention where by default all ports in the system 
have the same MAC address...).
-- 
/Jonathan Lundell.

Re: hotplug e1000 failed after 32 times

[Li Shaohua]

On Sat, 2004-09-18 at 07:34, Jeff Garzik wrote:
> All this work and code for such an uncommon case??
> 
> First off, any settings which are _indexed_ are almost guaranteed 
> broken.  For reasons as we see here, and others.  Any 
> "setting_foo[board_number]" should be found and eliminated instead.
> 
> Even if you allocate and free board numbers as described, how precisely 
> do you propose to predict which settings belong to which hotplugged 
> board?  Look at the problem, and you realize that the board<->setting 
> association becomes effectively _random_ for any adapter not present at 
> modprobe time.
> 
> The best model is to set these settings via netlink/ethtool after 
> registering the interface, but before bringing it up.
I'm not familiar with the NIC driver, but this problem really is
annoying. The gurus, please consider a solution. It's not an uncommon
case. I believe it's common in a big system with hotplug support. I can
understand why the driver doesn't support more than 32 a card, but one
card with 32 times hotplug failed is a little ugly.

Thanks,
Shaohua

Re: hotplug e1000 failed after 32 times

[Jeff Garzik]

Li Shaohua wrote:
> I'm not familiar with the NIC driver, but this problem really is
> annoying. The gurus, please consider a solution. It's not an uncommon
> case. I believe it's common in a big system with hotplug support. I can
> understand why the driver doesn't support more than 32 a card, but one
> card with 32 times hotplug failed is a little ugly.


There should be no problem at all with the driver supporting 32 NICs... 
  in fact if it cannot support at least 99 NICs, I would consider that a 
bug.

	Jeff

Re: hotplug e1000 failed after 32 times

[Chris Leech]

On Sun, 19 Sep 2004 22:55:51 -0400, Jeff Garzik <jgarzik@pobox.com> wrote:
> Li Shaohua wrote:
> > I'm not familiar with the NIC driver, but this problem really is
> > annoying. The gurus, please consider a solution. It's not an uncommon
> > case. I believe it's common in a big system with hotplug support. I can
> > understand why the driver doesn't support more than 32 a card, but one
> > card with 32 times hotplug failed is a little ugly.
> 
> There should be no problem at all with the driver supporting 32 NICs...
>   in fact if it cannot support at least 99 NICs, I would consider that a
> bug.

I'd like to hear more about what the failure condition actually is,
see any system messages logged.

The 32 NIC limitation is for configuration via the module parameters,
but e1000 itself doesn't impose any other limitations on the number of
NICs.  I know of testing situations where 360 e1000 ports were in use
on a large system.  I've run hotplug tests, powering slots on and off
while passing traffic in a fail-over teaming configuration, for weeks
without problem.  I'll run a hotplug test myself as soon as I get a
chance, but I don't see any reason why it should be failing.

As for the module parameter limitations, yes only 32 ports can be
configured that way and the counter doesn't roll back with
hot-removes.  I think we can live with those limitations.  Indexed
parameters make no sense in a hotplug environment, use ethtool.

The module parameters in e1000 work for a large number or users who
can live with those limitations.  That's why they're still in the
driver.  As long as they work, I think they should stay.  But I'm not
in favor of adding more code, or making the existing code more
complex, to try and remove those limitations.  That's why everything
can be configured through ethtool.

- Chris

Re: hotplug e1000 failed after 32 times

[Li Shaohua]

On Mon, 2004-09-20 at 12:15, Chris Leech wrote:
> On Sun, 19 Sep 2004 22:55:51 -0400, Jeff Garzik <jgarzik@pobox.com> wrote:
> > Li Shaohua wrote:
> > > I'm not familiar with the NIC driver, but this problem really is
> > > annoying. The gurus, please consider a solution. It's not an uncommon
> > > case. I believe it's common in a big system with hotplug support. I can
> > > understand why the driver doesn't support more than 32 a card, but one
> > > card with 32 times hotplug failed is a little ugly.
> > 
> > There should be no problem at all with the driver supporting 32 NICs...
> >   in fact if it cannot support at least 99 NICs, I would consider that a
> > bug.
> 
> I'd like to hear more about what the failure condition actually is,
> see any system messages logged.
> 
> The 32 NIC limitation is for configuration via the module parameters,
> but e1000 itself doesn't impose any other limitations on the number of
> NICs.  I know of testing situations where 360 e1000 ports were in use
> on a large system.  I've run hotplug tests, powering slots on and off
> while passing traffic in a fail-over teaming configuration, for weeks
> without problem.  I'll run a hotplug test myself as soon as I get a
> chance, but I don't see any reason why it should be failing.
> 
> As for the module parameter limitations, yes only 32 ports can be
> configured that way and the counter doesn't roll back with
> hot-removes.  I think we can live with those limitations.  Indexed
> parameters make no sense in a hotplug environment, use ethtool.
> 
> The module parameters in e1000 work for a large number or users who
> can live with those limitations.  That's why they're still in the
> driver.  As long as they work, I think they should stay.  But I'm not
> in favor of adding more code, or making the existing code more
> complex, to try and remove those limitations.  That's why everything
> can be configured through ethtool.
Below is the dmesg I got.
>ACPI: PCI interrupt 0000:11:01.0[A] -> GSI 120 (level, low) -> IRQ 58
>e1000: eth1: e1000_probe: Intel(R) PRO/1000 Network Connection
>e1000: eth1: e1000_check_options: Warning: no configuration for board
#36
>e1000: eth1: e1000_check_options: Using defaults for all values
but the default value isn't correct, such as the 'speed' is 65535.
I think you are right, I can change the parameter using ethtool. This
way I don't need the driver changes. Thanks.

Thanks,
Shaohua

Re: hotplug e1000 failed after 32 times

[Li Shaohua]

[-- Attachment #1: Type: text/plain, Size: 4127 bytes --]

On Fri, 2004-09-17 at 13:14, Andrew Morton wrote:
> Li Shaohua <shaohua.li@intel.com> wrote:
> >
> > I'm testing a hotplug driver. In my test, I will hot add/remove an e1000
> >  NIC frequently. The result is my hot add failed after 32 times hotadd.
> >  After looking at the code of e1000 driver, I found
> >  e1000_adapter->bd_number has maxium limitation of 32, and it increased
> >  one every hot add. Looks like the remove driver routine didn't free the
> >  'bd_number', so hot add failed after 32 times. Below patch fixes this
> >  issue.
> 
> Yeah.  I think you'll find that damn near every net driver in the kernel
> has this problem.  I think it would be better to create a little suite of
> library functions in net/core/dev.c to handle this situation.
<snip>
Here is the patch for e1000 driver using APIs in my previous email.

Thanks,
Shaohua


Signed-off-by: Li Shaohua <shaohua.li@intel.com>
===== drivers/net/e1000/e1000_main.c 1.134 vs edited =====
--- 1.134/drivers/net/e1000/e1000_main.c	2004-09-13 07:52:48 +08:00
+++ edited/drivers/net/e1000/e1000_main.c	2004-09-17 16:39:34 +08:00
@@ -180,6 +180,9 @@ struct notifier_block e1000_notifier_reb
 /* Exported from other modules */
 
 extern void e1000_check_options(struct e1000_adapter *adapter);
+extern void e1000_init_boards(void);
+extern int e1000_alloc_bd_number(void);
+extern void e1000_free_bd_number(int);
 
 static struct pci_driver e1000_driver = {
 	.name     = e1000_driver_name,
@@ -220,6 +223,7 @@ e1000_init_module(void)
 	ret = pci_module_init(&e1000_driver);
 	if(ret >= 0) {
 		register_reboot_notifier(&e1000_notifier_reboot);
+		e1000_init_boards();
 	}
 	return ret;
 }
@@ -380,7 +384,6 @@ e1000_probe(struct pci_dev *pdev,
 {
 	struct net_device *netdev;
 	struct e1000_adapter *adapter;
-	static int cards_found = 0;
 	unsigned long mmio_start;
 	int mmio_len;
 	int pci_using_dac;
@@ -471,7 +474,7 @@ e1000_probe(struct pci_dev *pdev,
 	netdev->mem_end = mmio_start + mmio_len;
 	netdev->base_addr = adapter->hw.io_base;
 
-	adapter->bd_number = cards_found;
+	adapter->bd_number = e1000_alloc_bd_number();
 
 	/* setup the private structure */
 
@@ -590,13 +593,13 @@ e1000_probe(struct pci_dev *pdev,
 	if((err = register_netdevice(netdev)))
 		goto err_register;
 
-	cards_found++;
 	rtnl_unlock();
 	return 0;
 
 err_register:
 err_sw_init:
 err_eeprom:
+	e1000_free_bd_number(adapter->bd_number);
 	iounmap(adapter->hw.hw_addr);
 err_ioremap:
 err_free_unlock:
@@ -638,6 +641,7 @@ e1000_remove(struct pci_dev *pdev)
 	e1000_phy_hw_reset(&adapter->hw);
 
 	iounmap(adapter->hw.hw_addr);
+	e1000_free_bd_number(adapter->bd_number);
 	pci_release_regions(pdev);
 
 	free_netdev(netdev);
===== drivers/net/e1000/e1000_param.c 1.31 vs edited =====
--- 1.31/drivers/net/e1000/e1000_param.c	2004-08-29 06:55:39 +08:00
+++ edited/drivers/net/e1000/e1000_param.c	2004-09-17 16:24:35 +08:00
@@ -56,7 +56,7 @@
  */
 
 #define E1000_PARAM(X, S) \
-static const int __devinitdata X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT;
\
+static int __devinitdata X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT; \
 MODULE_PARM(X, "1-" __MODULE_STRING(E1000_MAX_NIC) "i"); \
 MODULE_PARM_DESC(X, S);
 
@@ -703,3 +703,36 @@ e1000_check_copper_options(struct e1000_
 	}
 }
 
+static struct net_boards board;
+void e1000_init_boards(void)
+{
+	net_boards_init(&board, E1000_MAX_NIC);
+}
+
+int e1000_alloc_bd_number(void)
+{
+	int ret;
+
+	if ((ret = net_boards_alloc(&board)) < 0)
+		return E1000_MAX_NIC;
+	return ret;
+}
+
+void e1000_free_bd_number(int bd_number)
+{
+	if ((bd_number < 0) || (bd_number > E1000_MAX_NIC))
+		return;
+	TxDescriptors[bd_number] = OPTION_UNSET;
+	RxDescriptors[bd_number] = OPTION_UNSET;
+	Speed[bd_number] = OPTION_UNSET;
+	Duplex[bd_number] = OPTION_UNSET;
+	AutoNeg[bd_number] = OPTION_UNSET;
+	FlowControl[bd_number] = OPTION_UNSET;
+	XsumRX[bd_number] = OPTION_UNSET;
+	TxIntDelay[bd_number] = OPTION_UNSET;
+	TxAbsIntDelay[bd_number] = OPTION_UNSET;
+	RxIntDelay[bd_number] = OPTION_UNSET;
+	RxAbsIntDelay[bd_number] = OPTION_UNSET;
+	InterruptThrottleRate[bd_number] = OPTION_UNSET;
+	net_boards_free(&board, bd_number);
+}


[-- Attachment #2: nic-e1000.patch --]
[-- Type: text/x-patch, Size: 3219 bytes --]

===== drivers/net/e1000/e1000_main.c 1.134 vs edited =====
--- 1.134/drivers/net/e1000/e1000_main.c	2004-09-13 07:52:48 +08:00
+++ edited/drivers/net/e1000/e1000_main.c	2004-09-17 16:39:34 +08:00
@@ -180,6 +180,9 @@ struct notifier_block e1000_notifier_reb
 /* Exported from other modules */
 
 extern void e1000_check_options(struct e1000_adapter *adapter);
+extern void e1000_init_boards(void);
+extern int e1000_alloc_bd_number(void);
+extern void e1000_free_bd_number(int);
 
 static struct pci_driver e1000_driver = {
 	.name     = e1000_driver_name,
@@ -220,6 +223,7 @@ e1000_init_module(void)
 	ret = pci_module_init(&e1000_driver);
 	if(ret >= 0) {
 		register_reboot_notifier(&e1000_notifier_reboot);
+		e1000_init_boards();
 	}
 	return ret;
 }
@@ -380,7 +384,6 @@ e1000_probe(struct pci_dev *pdev,
 {
 	struct net_device *netdev;
 	struct e1000_adapter *adapter;
-	static int cards_found = 0;
 	unsigned long mmio_start;
 	int mmio_len;
 	int pci_using_dac;
@@ -471,7 +474,7 @@ e1000_probe(struct pci_dev *pdev,
 	netdev->mem_end = mmio_start + mmio_len;
 	netdev->base_addr = adapter->hw.io_base;
 
-	adapter->bd_number = cards_found;
+	adapter->bd_number = e1000_alloc_bd_number();
 
 	/* setup the private structure */
 
@@ -590,13 +593,13 @@ e1000_probe(struct pci_dev *pdev,
 	if((err = register_netdevice(netdev)))
 		goto err_register;
 
-	cards_found++;
 	rtnl_unlock();
 	return 0;
 
 err_register:
 err_sw_init:
 err_eeprom:
+	e1000_free_bd_number(adapter->bd_number);
 	iounmap(adapter->hw.hw_addr);
 err_ioremap:
 err_free_unlock:
@@ -638,6 +641,7 @@ e1000_remove(struct pci_dev *pdev)
 	e1000_phy_hw_reset(&adapter->hw);
 
 	iounmap(adapter->hw.hw_addr);
+	e1000_free_bd_number(adapter->bd_number);
 	pci_release_regions(pdev);
 
 	free_netdev(netdev);
===== drivers/net/e1000/e1000_param.c 1.31 vs edited =====
--- 1.31/drivers/net/e1000/e1000_param.c	2004-08-29 06:55:39 +08:00
+++ edited/drivers/net/e1000/e1000_param.c	2004-09-17 16:24:35 +08:00
@@ -56,7 +56,7 @@
  */
 
 #define E1000_PARAM(X, S) \
-static const int __devinitdata X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT; \
+static int __devinitdata X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT; \
 MODULE_PARM(X, "1-" __MODULE_STRING(E1000_MAX_NIC) "i"); \
 MODULE_PARM_DESC(X, S);
 
@@ -703,3 +703,36 @@ e1000_check_copper_options(struct e1000_
 	}
 }
 
+static struct net_boards board;
+void e1000_init_boards(void)
+{
+	net_boards_init(&board, E1000_MAX_NIC);
+}
+
+int e1000_alloc_bd_number(void)
+{
+	int ret;
+
+	if ((ret = net_boards_alloc(&board)) < 0)
+		return E1000_MAX_NIC;
+	return ret;
+}
+
+void e1000_free_bd_number(int bd_number)
+{
+	if ((bd_number < 0) || (bd_number > E1000_MAX_NIC))
+		return;
+	TxDescriptors[bd_number] = OPTION_UNSET;
+	RxDescriptors[bd_number] = OPTION_UNSET;
+	Speed[bd_number] = OPTION_UNSET;
+	Duplex[bd_number] = OPTION_UNSET;
+	AutoNeg[bd_number] = OPTION_UNSET;
+	FlowControl[bd_number] = OPTION_UNSET;
+	XsumRX[bd_number] = OPTION_UNSET;
+	TxIntDelay[bd_number] = OPTION_UNSET;
+	TxAbsIntDelay[bd_number] = OPTION_UNSET;
+	RxIntDelay[bd_number] = OPTION_UNSET;
+	RxAbsIntDelay[bd_number] = OPTION_UNSET;
+	InterruptThrottleRate[bd_number] = OPTION_UNSET;
+	net_boards_free(&board, bd_number);
+}