Re: airo

Re: airo

[Matteo Croce]

after booting i have this situation:

root@raver:~# lsmod |fgrep airo
airo                   80016  0
root@raver:~# dmesg |fgrep airo
airo(): Probing for PCI adapters
airo(eth1): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
airo(eth1): Doing fast bap_reads
airo(eth1): WPA is supported.
airo(eth1): Couldn't register_netdev
airo(): Finished probing for PCI adapters
root@raver:~#

I have to do this to connect:

root@raver:~# rmmod airo
root@raver:~# modprobe airo
root@raver:~# dmesg |fgrep airo
airo(): Probing for PCI adapters
airo(eth1): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
airo(eth1): Doing fast bap_reads
airo(eth1): WPA is supported.
airo(eth1): Couldn't register_netdev
airo(): Finished probing for PCI adapters
airo(): Probing for PCI adapters
airo(eth0): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
airo(eth0): Doing fast bap_reads
airo(eth0): WPA is supported.
airo(eth0): MAC enabled 0:d:29:4f:c:be
airo(): Finished probing for PCI adapters
root@raver:~#

Re: airo

[Michal Schmidt]

Matteo Croce wrote:
> after booting i have this situation:
>
> root@raver:~# lsmod |fgrep airo
> airo                   80016  0
> root@raver:~# dmesg |fgrep airo
> airo(): Probing for PCI adapters
> airo(eth1): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
> airo(eth1): Doing fast bap_reads
> airo(eth1): WPA is supported.
> airo(eth1): Couldn't register_netdev
> airo(): Finished probing for PCI adapters
> root@raver:~#
>
> I have to do this to connect:
>
> root@raver:~# rmmod airo
> root@raver:~# modprobe airo
> root@raver:~# dmesg |fgrep airo
> airo(): Probing for PCI adapters
> airo(eth1): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
> airo(eth1): Doing fast bap_reads
> airo(eth1): WPA is supported.
> airo(eth1): Couldn't register_netdev
> airo(): Finished probing for PCI adapters
> airo(): Probing for PCI adapters
> airo(eth0): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
> airo(eth0): Doing fast bap_reads
> airo(eth0): WPA is supported.
> airo(eth0): MAC enabled 0:d:29:4f:c:be
> airo(): Finished probing for PCI adapters
> root@raver:~#
>   

Hi Matteo,

It looks like some other network interface gets renamed from "eth0" to
"eth1" at the same time the airo driver is initializing the card. Does
it happen always after booting? Do you have other network interfaces?
This patch should fix it. Can you test it?

Michal

diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 2d3a180..1a350b3 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -41,6 +41,7 @@
 
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/rtnetlink.h>
 #include <linux/skbuff.h>
 #include <linux/if_arp.h>
 #include <linux/ioport.h>
@@ -2802,10 +2803,6 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 		airo_print_err("", "Couldn't alloc_etherdev");
 		return NULL;
         }
-	if (dev_alloc_name(dev, dev->name) < 0) {
-		airo_print_err("", "Couldn't get name!");
-		goto err_out_free;
-	}
 
 	ai = dev->priv;
 	ai->wifidev = NULL;
@@ -2813,7 +2810,7 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 	ai->jobs = 0;
 	ai->dev = dev;
 	if (pci && (pci->device == 0x5000 || pci->device == 0xa504)) {
-		airo_print_dbg(dev->name, "Found an MPI350 card");
+		airo_print_dbg("", "Found an MPI350 card");
 		set_bit(FLAG_MPI, &ai->flags);
 	}
 	spin_lock_init(&ai->aux_lock);
@@ -2821,14 +2818,11 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 	ai->config.len = 0;
 	ai->pci = pci;
 	init_waitqueue_head (&ai->thr_wait);
-	ai->airo_thread_task = kthread_run(airo_thread, dev, dev->name);
-	if (IS_ERR(ai->airo_thread_task))
-		goto err_out_free;
 	ai->tfm = NULL;
 	add_airo_dev(ai);
 
 	if (airo_networks_allocate (ai))
-		goto err_out_thr;
+		goto err_out_free;
 	airo_networks_initialize (ai);
 
 	/* The Airo-specific entries in the device structure. */
@@ -2855,16 +2849,31 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 	reset_card (dev, 1);
 	msleep(400);
 
+	rtnl_lock();  /* Makes sure no device registers our name. */
+	if (dev_alloc_name(dev, dev->name) < 0) {
+		airo_print_err("", "Couldn't get name!");
+		rtnl_unlock();
+		goto err_out_nets;
+	}
+
+	ai->airo_thread_task = kthread_run(airo_thread, dev, dev->name);
+	if (IS_ERR(ai->airo_thread_task)) {
+		rtnl_unlock();
+		goto err_out_nets;
+	}
+
 	rc = request_irq( dev->irq, airo_interrupt, IRQF_SHARED, dev->name, dev );
 	if (rc) {
 		airo_print_err(dev->name, "register interrupt %d failed, rc %d",
 				irq, rc);
-		goto err_out_nets;
+		rtnl_unlock();
+		goto err_out_thr;
 	}
 	if (!is_pcmcia) {
 		if (!request_region( dev->base_addr, 64, dev->name )) {
 			rc = -EBUSY;
 			airo_print_err(dev->name, "Couldn't request region");
+			rtnl_unlock();
 			goto err_out_irq;
 		}
 	}
@@ -2872,6 +2881,7 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 	if (test_bit(FLAG_MPI,&ai->flags)) {
 		if (mpi_map_card(ai, pci, dev->name)) {
 			airo_print_err(dev->name, "Could not map memory");
+			rtnl_unlock();
 			goto err_out_res;
 		}
 	}
@@ -2880,6 +2890,7 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 		if ( setup_card( ai, dev->dev_addr, 1 ) != SUCCESS ) {
 			airo_print_err(dev->name, "MAC could not be enabled" );
 			rc = -EIO;
+			rtnl_unlock();
 			goto err_out_map;
 		}
 	} else if (!test_bit(FLAG_MPI,&ai->flags)) {
@@ -2899,9 +2910,10 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 		ai->bssListRidLen = sizeof(BSSListRid) - sizeof(BSSListRidExtra);
 	}
 
-	rc = register_netdev(dev);
+	rc = register_netdevice(dev);
+	rtnl_unlock();
 	if (rc) {
-		airo_print_err(dev->name, "Couldn't register_netdev");
+		airo_print_err(dev->name, "Couldn't register_netdevice");
 		goto err_out_map;
 	}
 	ai->wifidev = init_wifidev(ai, dev);
@@ -2942,13 +2954,13 @@ err_out_res:
 	        release_region( dev->base_addr, 64 );
 err_out_irq:
 	free_irq(dev->irq, dev);
-err_out_nets:
-	airo_networks_free(ai);
 err_out_thr:
-	del_airo_dev(ai);
 	set_bit(JOB_DIE, &ai->jobs);
 	kthread_stop(ai->airo_thread_task);
+err_out_nets:
+	airo_networks_free(ai);
 err_out_free:
+	del_airo_dev(ai);
 	free_netdev(dev);
 	return NULL;
 }

Re: airo

[Dan Williams]

On Mon, 2007-06-25 at 16:38 +0200, Michal Schmidt wrote:
> Matteo Croce wrote:
> > after booting i have this situation:
> >
> > root@raver:~# lsmod |fgrep airo
> > airo                   80016  0
> > root@raver:~# dmesg |fgrep airo
> > airo(): Probing for PCI adapters
> > airo(eth1): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
> > airo(eth1): Doing fast bap_reads
> > airo(eth1): WPA is supported.
> > airo(eth1): Couldn't register_netdev
> > airo(): Finished probing for PCI adapters
> > root@raver:~#
> >
> > I have to do this to connect:
> >
> > root@raver:~# rmmod airo
> > root@raver:~# modprobe airo
> > root@raver:~# dmesg |fgrep airo
> > airo(): Probing for PCI adapters
> > airo(eth1): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
> > airo(eth1): Doing fast bap_reads
> > airo(eth1): WPA is supported.
> > airo(eth1): Couldn't register_netdev
> > airo(): Finished probing for PCI adapters
> > airo(): Probing for PCI adapters
> > airo(eth0): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
> > airo(eth0): Doing fast bap_reads
> > airo(eth0): WPA is supported.
> > airo(eth0): MAC enabled 0:d:29:4f:c:be
> > airo(): Finished probing for PCI adapters
> > root@raver:~#
> >   
> 
> Hi Matteo,
> 
> It looks like some other network interface gets renamed from "eth0" to
> "eth1" at the same time the airo driver is initializing the card. Does
> it happen always after booting? Do you have other network interfaces?
> This patch should fix it. Can you test it?

Wow, that's somewhat ugly.  Do other drivers just not have this problem,
or have they worked around it in similar or different ways?  Just trying
to understand if the scope of the issue is wider than just airo.

Dan

> Michal
> 
> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
> index 2d3a180..1a350b3 100644
> --- a/drivers/net/wireless/airo.c
> +++ b/drivers/net/wireless/airo.c
> @@ -41,6 +41,7 @@
>  
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
> +#include <linux/rtnetlink.h>
>  #include <linux/skbuff.h>
>  #include <linux/if_arp.h>
>  #include <linux/ioport.h>
> @@ -2802,10 +2803,6 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
>  		airo_print_err("", "Couldn't alloc_etherdev");
>  		return NULL;
>          }
> -	if (dev_alloc_name(dev, dev->name) < 0) {
> -		airo_print_err("", "Couldn't get name!");
> -		goto err_out_free;
> -	}
>  
>  	ai = dev->priv;
>  	ai->wifidev = NULL;
> @@ -2813,7 +2810,7 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
>  	ai->jobs = 0;
>  	ai->dev = dev;
>  	if (pci && (pci->device == 0x5000 || pci->device == 0xa504)) {
> -		airo_print_dbg(dev->name, "Found an MPI350 card");
> +		airo_print_dbg("", "Found an MPI350 card");
>  		set_bit(FLAG_MPI, &ai->flags);
>  	}
>  	spin_lock_init(&ai->aux_lock);
> @@ -2821,14 +2818,11 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
>  	ai->config.len = 0;
>  	ai->pci = pci;
>  	init_waitqueue_head (&ai->thr_wait);
> -	ai->airo_thread_task = kthread_run(airo_thread, dev, dev->name);
> -	if (IS_ERR(ai->airo_thread_task))
> -		goto err_out_free;
>  	ai->tfm = NULL;
>  	add_airo_dev(ai);
>  
>  	if (airo_networks_allocate (ai))
> -		goto err_out_thr;
> +		goto err_out_free;
>  	airo_networks_initialize (ai);
>  
>  	/* The Airo-specific entries in the device structure. */
> @@ -2855,16 +2849,31 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
>  	reset_card (dev, 1);
>  	msleep(400);
>  
> +	rtnl_lock();  /* Makes sure no device registers our name. */
> +	if (dev_alloc_name(dev, dev->name) < 0) {
> +		airo_print_err("", "Couldn't get name!");
> +		rtnl_unlock();
> +		goto err_out_nets;
> +	}
> +
> +	ai->airo_thread_task = kthread_run(airo_thread, dev, dev->name);
> +	if (IS_ERR(ai->airo_thread_task)) {
> +		rtnl_unlock();
> +		goto err_out_nets;
> +	}
> +
>  	rc = request_irq( dev->irq, airo_interrupt, IRQF_SHARED, dev->name, dev );
>  	if (rc) {
>  		airo_print_err(dev->name, "register interrupt %d failed, rc %d",
>  				irq, rc);
> -		goto err_out_nets;
> +		rtnl_unlock();
> +		goto err_out_thr;
>  	}
>  	if (!is_pcmcia) {
>  		if (!request_region( dev->base_addr, 64, dev->name )) {
>  			rc = -EBUSY;
>  			airo_print_err(dev->name, "Couldn't request region");
> +			rtnl_unlock();
>  			goto err_out_irq;
>  		}
>  	}
> @@ -2872,6 +2881,7 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
>  	if (test_bit(FLAG_MPI,&ai->flags)) {
>  		if (mpi_map_card(ai, pci, dev->name)) {
>  			airo_print_err(dev->name, "Could not map memory");
> +			rtnl_unlock();
>  			goto err_out_res;
>  		}
>  	}
> @@ -2880,6 +2890,7 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
>  		if ( setup_card( ai, dev->dev_addr, 1 ) != SUCCESS ) {
>  			airo_print_err(dev->name, "MAC could not be enabled" );
>  			rc = -EIO;
> +			rtnl_unlock();
>  			goto err_out_map;
>  		}
>  	} else if (!test_bit(FLAG_MPI,&ai->flags)) {
> @@ -2899,9 +2910,10 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
>  		ai->bssListRidLen = sizeof(BSSListRid) - sizeof(BSSListRidExtra);
>  	}
>  
> -	rc = register_netdev(dev);
> +	rc = register_netdevice(dev);
> +	rtnl_unlock();
>  	if (rc) {
> -		airo_print_err(dev->name, "Couldn't register_netdev");
> +		airo_print_err(dev->name, "Couldn't register_netdevice");
>  		goto err_out_map;
>  	}
>  	ai->wifidev = init_wifidev(ai, dev);
> @@ -2942,13 +2954,13 @@ err_out_res:
>  	        release_region( dev->base_addr, 64 );
>  err_out_irq:
>  	free_irq(dev->irq, dev);
> -err_out_nets:
> -	airo_networks_free(ai);
>  err_out_thr:
> -	del_airo_dev(ai);
>  	set_bit(JOB_DIE, &ai->jobs);
>  	kthread_stop(ai->airo_thread_task);
> +err_out_nets:
> +	airo_networks_free(ai);
>  err_out_free:
> +	del_airo_dev(ai);
>  	free_netdev(dev);
>  	return NULL;
>  }
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: airo

[Michal Schmidt]

Dan Williams wrote:
> On Mon, 2007-06-25 at 16:38 +0200, Michal Schmidt wrote:
>> Matteo Croce wrote:
>>> after booting i have this situation:
>>>
>>> root@raver:~# lsmod |fgrep airo
>>> airo                   80016  0
>>> root@raver:~# dmesg |fgrep airo
>>> airo(): Probing for PCI adapters
>>> airo(eth1): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
>>> airo(eth1): Doing fast bap_reads
>>> airo(eth1): WPA is supported.
>>> airo(eth1): Couldn't register_netdev
>>> airo(): Finished probing for PCI adapters
>>> root@raver:~#
>>>
>>> I have to do this to connect:
>>>
>>> root@raver:~# rmmod airo
>>> root@raver:~# modprobe airo
>>> root@raver:~# dmesg |fgrep airo
>>> airo(): Probing for PCI adapters
>>> airo(eth1): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
>>> airo(eth1): Doing fast bap_reads
>>> airo(eth1): WPA is supported.
>>> airo(eth1): Couldn't register_netdev
>>> airo(): Finished probing for PCI adapters
>>> airo(): Probing for PCI adapters
>>> airo(eth0): cmd:111 status:7f11 rsp0:2 rsp1:0 rsp2:0
>>> airo(eth0): Doing fast bap_reads
>>> airo(eth0): WPA is supported.
>>> airo(eth0): MAC enabled 0:d:29:4f:c:be
>>> airo(): Finished probing for PCI adapters
>>> root@raver:~#
>>>   
>> Hi Matteo,
>>
>> It looks like some other network interface gets renamed from "eth0" to
>> "eth1" at the same time the airo driver is initializing the card. Does
>> it happen always after booting? Do you have other network interfaces?
>> This patch should fix it. Can you test it?
> 
> Wow, that's somewhat ugly.  Do other drivers just not have this problem,
> or have they worked around it in similar or different ways?  Just trying
> to understand if the scope of the issue is wider than just airo.

For instance, drivers/net/wireless/atmel.c looks very similar, so I
guess it has the same race. With airo the race is just more visible,
because of all the long sleeps it does between getting the name with
dev_alloc_name and finally registering it.

eepro100.c takes rtnl_lock to avoid the issue. So does ns83820.c - it
has a big comment about how ugly it is.

Michal

Re: airo

[Michal Schmidt]

Michal Schmidt wrote:
> Dan Williams wrote:
>> Wow, that's somewhat ugly.  Do other drivers just not have this problem,
>> or have they worked around it in similar or different ways?  Just trying
>> to understand if the scope of the issue is wider than just airo.
> 
> For instance, drivers/net/wireless/atmel.c looks very similar, so I
> guess it has the same race. With airo the race is just more visible,
> because of all the long sleeps it does between getting the name with
> dev_alloc_name and finally registering it.
> 
> eepro100.c takes rtnl_lock to avoid the issue. So does ns83820.c - it
> has a big comment about how ugly it is.

A cleaner way would be to do most of the initialization later, from the
->open() method. I'll explore how workable this approach would be.

Michal

Re: airo

[Matteo Croce]

On Monday 25 June 2007 16:38:26 you wrote:
> It looks like some other network interface gets renamed from "eth0" to
> "eth1" at the same time the airo driver is initializing the card. Does
> it happen always after booting? Do you have other network interfaces?
> This patch should fix it. Can you test it?

Exactly, the boot process goes so:
1) udev loads forcedeth.ko and it gets eth0
2) udev also loads airo.ko
3) forcedeth loads very fast and udev renames it to eth1 according to /etc/iftab
4) airo slowly init the card and try to get eth1 as name, but found it busy and die()

Personally I solved this by deleting forcedeth.ko. I know that it's ugly, but I don't use wired etherned
and i hate do "rmmod airo ; modprobe airo" at every boot, also in a shell script.

Cheers,
Matteo

Re: airo

[Larry Finger]

Matteo Croce wrote:
> Exactly, the boot process goes so:
> 1) udev loads forcedeth.ko and it gets eth0
> 2) udev also loads airo.ko
> 3) forcedeth loads very fast and udev renames it to eth1 according to /etc/iftab
> 4) airo slowly init the card and try to get eth1 as name, but found it busy and die()
> 
> Personally I solved this by deleting forcedeth.ko. I know that it's ugly, but I don't use wired etherned
> and i hate do "rmmod airo ; modprobe airo" at every boot, also in a shell script.

What does /etc/udev/rules.d/30-net_persistent_names.rules say about the name for the MAC address of 
the wired interface? On my system, the wired one is forced to be eth0 and the wireless to be eth1.

Larry

Re: airo

[Dan Williams]

On Mon, 2007-06-25 at 14:50 -0500, Larry Finger wrote:
> Matteo Croce wrote:
> > Exactly, the boot process goes so:
> > 1) udev loads forcedeth.ko and it gets eth0
> > 2) udev also loads airo.ko
> > 3) forcedeth loads very fast and udev renames it to eth1 according to /etc/iftab
> > 4) airo slowly init the card and try to get eth1 as name, but found it busy and die()
> > 
> > Personally I solved this by deleting forcedeth.ko. I know that it's ugly, but I don't use wired etherned
> > and i hate do "rmmod airo ; modprobe airo" at every boot, also in a shell script.
> 
> What does /etc/udev/rules.d/30-net_persistent_names.rules say about the name for the MAC address of 
> the wired interface? On my system, the wired one is forced to be eth0 and the wireless to be eth1.

Even so, drivers need to be safe against device renames at any stage.

Dan

Re: airo

[Stephen Hemminger]

On Mon, 25 Jun 2007 17:10:34 -0400
Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Mon, 2007-06-25 at 14:50 -0500, Larry Finger wrote:
> > Matteo Croce wrote:
> > > Exactly, the boot process goes so:
> > > 1) udev loads forcedeth.ko and it gets eth0
> > > 2) udev also loads airo.ko
> > > 3) forcedeth loads very fast and udev renames it to eth1 according to /etc/iftab
> > > 4) airo slowly init the card and try to get eth1 as name, but found it busy and die()
> > > 
> > > Personally I solved this by deleting forcedeth.ko. I know that it's ugly, but I don't use wired etherned
> > > and i hate do "rmmod airo ; modprobe airo" at every boot, also in a shell script.
> > 
> > What does /etc/udev/rules.d/30-net_persistent_names.rules say about the name for the MAC address of 
> > the wired interface? On my system, the wired one is forced to be eth0 and the wireless to be eth1.
> 
> Even so, drivers need to be safe against device renames at any stage.
> 
> Dan
> 
> 

Name should NOT be used for anything but messages (and request_irq).
The driver should use ifindex if it needs some persistent token.
Using rtnl_lock() works but it can't be held across hotplug or other events.