orinoco_usb Request For Comments

orinoco_usb Request For Comments

[Manuel Estrada Sainz]

 Hello,

 As many of you already know, I have been working on a variant of the
 standard orinoco driver to support ORiNOCO USB devices.

 I now believe that it is stable enough for the kernel, and I would like
 to get it integrated in the official kernel tree.

 At first I tried convincing David to accept the changes in the standard
 orinoco driver but he was (rightfully) skeptic. Then Jean Tourrilhes
 opened my eyes, the changes touch carefully crafted locking semantics
 and could give trouble (although it has been working well for quite a
 while), and suggested adding it as an independent (alternative) driver.
 
 It has happened before with rtl8139/8139too and others, while the new
 driver probes it's merits stability conscious people can still use the
 standard driver.

 I know that there are some issues, this is what I plan to do:
 	- Beautify the source
		- Function renaming for consistency
		- Moving code around for structure
	- Remove reg_name()/rid_name()?
	- Cleanup bridge_or_reg/bridge_read_reg/bridge_write_reg to the
	  minimum possible.
	  	- Implement my own orinoco_interrupt using
		  __orinoco_ev_* directly.
	- Add '-exp' to all module names and offer them as an
	  alternative in Kconfig.
	  	- I have renamed a couple of functions so they don't get
		  mixed with the standard orinoco modules.

 Please comment, how much of that or what else needs to be done to get
 it in the kernel?

 Also suggestions on better ways to do the USB vs. PCMCIA abstraction
 would be welcomed, although IMHO that could be polished later.

 Oh, and since I am at it, I wouldn't mind cleaning kcompat.h for
 inclusion in 2.4 kernels to make driver porting easier. I have also
 been working in porting lirc so I could put it all together (the
 kcompat.h stuff) for 2.4 inclusion.

 The code can be downloaded from 

	http://alioth.debian.org/download.php/223/orinoco-usb-0.2.1.tar.bz2

 Or if you want to look at independent files:

	http://orinoco-usb.alioth.debian.org/orinoco-usb-0.2.1/

 And for the record, the web page for the project is:

 	http://orinoco-usb.alioth.debian.org/

 BTW, for comparison purposes, it was last merged with standard
 orinoco-0.13e.

 Have a nice day

 	Manuel

 PS: Sorry for the long message.
-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: orinoco_usb Request For Comments

[Oliver Neukum]

>  Please comment, how much of that or what else needs to be done to get
>  it in the kernel?

	if(dev->read.urb->status == -EINPROGRESS){
		warn("%s: Unlinking pending IN urb", __FUNCTION__);
		retval = bridge_remove_in_urb(dev);
		if(retval){
			dbg("retval %d status %d", retval,
			    dev->read.urb->status);
		}
	}

Unlink unconditionally.

		/* We don't like racing :) */
		ctx->outurb->transfer_flags &= ~URB_ASYNC_UNLINK;
		usb_unlink_urb(ctx->outurb);
		del_timer_sync(&ctx->timer);

But neither do we like sleeping in interrupt. You can't simply unset the flag
if somebody else may be needing it.

More when I am rested :-)

	Regards
		Oliver

Re: [Orinoco-devel] orinoco_usb Request For Comments

[Pavel Roskin]

On Thu, 26 Jun 2003, Manuel Estrada Sainz wrote:

>  I now believe that it is stable enough for the kernel, and I would like
>  to get it integrated in the official kernel tree.
>
>  At first I tried convincing David to accept the changes in the standard
>  orinoco driver but he was (rightfully) skeptic. Then Jean Tourrilhes
>  opened my eyes, the changes touch carefully crafted locking semantics
>  and could give trouble (although it has been working well for quite a
>  while), and suggested adding it as an independent (alternative) driver.

I think it's a reasonable request.  It's a pity that the future work on
the Orinoco driver won't be integrated into your driver automatically.  In
particular, scanning, monitor mode and switching to the separate wireless
handlers may be useful for the USB driver as well.

But indeed, Orinoco USB is very different from other Orinoco cards.  There
is a firmware that stands between the driver and the PCMCIA card, and that
firmware is less transparent than, say, PLX bridges.

It's a tough call, and it's up to you to make.

>  It has happened before with rtl8139/8139too and others, while the new
>  driver probes it's merits stability conscious people can still use the
>  standard driver.

I don't know what happened to rtl8139/8139too but I think the situation is
different from your description.  Unless you are going to make more
development on Orinoco USB than David Gibson does on Orinoco, the Orinoco
USB is not going to be the development version.  Besides, the drivers
support different hardware, so there is no choice for users.

As far as I know, Orinoco USB devices are quite rare, so the pool of
testers is going to be small compared to the standard Orinoco driver that
supports Symbol and Intersil cards as well.

>  Please comment, how much of that or what else needs to be done to get
>  it in the kernel?

If you are going to create a separate driver, you should rename the
module.  I wouldn't bother with separate modules.  Just link hermes,
orinoco and orinoco_usb to one driver, say orinoco-usb.

You may also need to rename all files if you want the driver to live in
drivers/net/wireless rather than in drivers/usb/net.

That's of course assumes that you want to use separate versions of
hermes.c and orinoco.c.  But maybe you want to share them with the Orinoco
driver now or in the future?  Then I'd like to know about your plans.

>  Oh, and since I am at it, I wouldn't mind cleaning kcompat.h for
>  inclusion in 2.4 kernels to make driver porting easier. I have also
>  been working in porting lirc so I could put it all together (the
>  kcompat.h stuff) for 2.4 inclusion.

That's a separate and very interesting topic.  It's good to encourage
developers to write for the current development kernel, but on the other
hand, if kcompat.h is shared between all drivers, changes in it (caused
by further changes in 2.5.x API) would break drivers in the stable
kernels.

Perhaps backported drivers should not share their compatibility code
unless there is some kind of coordination between their maintainers.

-- 
Regards,
Pavel Roskin

Re: [Orinoco-devel] orinoco_usb Request For Comments

[Manuel Estrada Sainz]

On Thu, Jun 26, 2003 at 06:03:23PM -0400, Pavel Roskin wrote:
> On Thu, 26 Jun 2003, Manuel Estrada Sainz wrote:
> 
> >  I now believe that it is stable enough for the kernel, and I would like
> >  to get it integrated in the official kernel tree.
> >
> >  At first I tried convincing David to accept the changes in the standard
> >  orinoco driver but he was (rightfully) skeptic. Then Jean Tourrilhes
> >  opened my eyes, the changes touch carefully crafted locking semantics
> >  and could give trouble (although it has been working well for quite a
> >  while), and suggested adding it as an independent (alternative) driver.
> 
> I think it's a reasonable request.  It's a pity that the future work on
> the Orinoco driver won't be integrated into your driver automatically.  In
> particular, scanning, monitor mode and switching to the separate wireless
> handlers may be useful for the USB driver as well.
> 
> But indeed, Orinoco USB is very different from other Orinoco cards.  There
> is a firmware that stands between the driver and the PCMCIA card, and that
> firmware is less transparent than, say, PLX bridges.
> 
> It's a tough call, and it's up to you to make.

 I am not going that drastic jet, I'll provide all functionality:

	hermes-exp.o       orinoco-exp_cs.o       orinoco-exp_plx.o
	orinoco-exp.o      orinoco-exp_pci.o      orinoco-exp_tmd.o
	orinoco-exp_usb.o

 And keep merging with David's code. If in the end David likes the code
 we merge the two and live happily for ever after, and if not the most
 similar the two variants are, the easier periodic merging will be.

 Well, over time I may get tired of the situation, drop all but usb and
 run apart, but not now.

> >  It has happened before with rtl8139/8139too and others, while the new
> >  driver probes it's merits stability conscious people can still use the
> >  standard driver.
> 
> I don't know what happened to rtl8139/8139too but I think the situation is
> different from your description.  Unless you are going to make more
> development on Orinoco USB than David Gibson does on Orinoco, the Orinoco
> USB is not going to be the development version.  Besides, the drivers
> support different hardware, so there is no choice for users.

 orinoco-exp will support (it already does, though it is not called
 orinoco-exp jet) all the hardware that standard orinoco supports plus
 ORiNOCO USB devices, maybe it could even be extended to support Prism2
 USB devices.

 Actually, orinoco-exp could be used as a test bed for monitor mode,
 scanning, hermesap, ... and merge it back to the standard orinoco as it
 probes to work right. For now it should be a test bed for USB support :)
 
> As far as I know, Orinoco USB devices are quite rare, so the pool of
> testers is going to be small compared to the standard Orinoco driver that
> supports Symbol and Intersil cards as well.

 Not so rare, take a look at the statistics in alioth:

	http://alioth.debian.org/project/showfiles.php?group_id=1245
	http://alioth.debian.org/project/stats/index.php?report=last_30&group_id=1245

 36 Downloads in 24hours, keep in mind that the W200 wireless module is
 the default networking add-on for Compaq Evo series.

 I have around 80 happy subscribers on the orinoco-usb-devel mailing
 list already. And it didn't even get into the kernel jet.

> >  Please comment, how much of that or what else needs to be done to get
> >  it in the kernel?
> 
> If you are going to create a separate driver, you should rename the
> module.  I wouldn't bother with separate modules.  Just link hermes,
> orinoco and orinoco_usb to one driver, say orinoco-usb.

 No, I want to stay as similar to standard orinoco as possible to make
 merging easier.

> You may also need to rename all files if you want the driver to live in
> drivers/net/wireless rather than in drivers/usb/net.
 
 Yes, that was my plan.
 
> That's of course assumes that you want to use separate versions of
> hermes.c and orinoco.c.  But maybe you want to share them with the Orinoco
> driver now or in the future?  Then I'd like to know about your plans.

 For now I plan to duplicate it all, so I can get my code in without
 bothering standard orinoco, but I would like to merge as much as
 possible in the future. I hope that having both versions side by side
 will increase the mind share in finding a good way to do the merge.

 And also get the testing of some adventurous PCI/PCMCIA users.
 
> >  Oh, and since I am at it, I wouldn't mind cleaning kcompat.h for
> >  inclusion in 2.4 kernels to make driver porting easier. I have also
> >  been working in porting lirc so I could put it all together (the
> >  kcompat.h stuff) for 2.4 inclusion.
> 
> That's a separate and very interesting topic.  It's good to encourage
> developers to write for the current development kernel, but on the other
> hand, if kcompat.h is shared between all drivers, changes in it (caused
> by further changes in 2.5.x API) would break drivers in the stable
> kernels.

 Didn't think of that. But 2.5 API should be pretty frozen by now,
 shouldn't it?

> Perhaps backported drivers should not share their compatibility code
> unless there is some kind of coordination between their maintainers.

 But then you duplicate the effort times the number of independent
 drivers. At least there could be a recommended kcompat.h header, even if
 not in the kernel proper, so driver developers can just go get it when
 they are ready to update their drivers, instead of having to updated it
 themselfs. Maybe placing it in the Documentation directory would make
 it clear that it should not be used directly but copied for each driver
 independently.

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: orinoco_usb Request For Comments

[Manuel Estrada Sainz]

On Thu, Jun 26, 2003 at 11:41:18PM +0200, Oliver Neukum wrote:
> 
> >  Please comment, how much of that or what else needs to be done to get
> >  it in the kernel?
> 
> 	if(dev->read.urb->status == -EINPROGRESS){
> 		warn("%s: Unlinking pending IN urb", __FUNCTION__);
> 		retval = bridge_remove_in_urb(dev);
> 		if(retval){
> 			dbg("retval %d status %d", retval,
> 			    dev->read.urb->status);
> 		}
> 	}
> 
> Unlink unconditionally.

 OK, done.

> 		/* We don't like racing :) */
> 		ctx->outurb->transfer_flags &= ~URB_ASYNC_UNLINK;
> 		usb_unlink_urb(ctx->outurb);
> 		del_timer_sync(&ctx->timer);
> 
> But neither do we like sleeping in interrupt. You can't simply unset the flag
> if somebody else may be needing it.

 mmm, but the problem is that the interrupt handler can rearm the timer.
 And it can also complete the request_context freeing the memory, and we
 don't want to free the memory twice or access freed memory.

 Suggestions on how to get this right would be greatly appreciated.

 Maybe more paranoid refcounting?

> More when I am rested :-)

 Thanks a lot, I was really missing some peer review.

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: [Orinoco-devel] orinoco_usb Request For Comments

[David Gibson]

On Thu, Jun 26, 2003 at 06:03:23PM -0400, Pavel Roskin wrote:
> On Thu, 26 Jun 2003, Manuel Estrada Sainz wrote:
> 
> >  I now believe that it is stable enough for the kernel, and I would like
> >  to get it integrated in the official kernel tree.
> >
> >  At first I tried convincing David to accept the changes in the standard
> >  orinoco driver but he was (rightfully) skeptic. Then Jean Tourrilhes
> >  opened my eyes, the changes touch carefully crafted locking semantics
> >  and could give trouble (although it has been working well for quite a
> >  while), and suggested adding it as an independent (alternative) driver.
> 
> I think it's a reasonable request.  It's a pity that the future work on
> the Orinoco driver won't be integrated into your driver automatically.  In
> particular, scanning, monitor mode and switching to the separate wireless
> handlers may be useful for the USB driver as well.

Indeed.  I certainly hope that at some point we can share at least
parts of the code between the drivers.  I just haven't seen a good way
to do it yet.

> But indeed, Orinoco USB is very different from other Orinoco cards.  There
> is a firmware that stands between the driver and the PCMCIA card, and that
> firmware is less transparent than, say, PLX bridges.

Quite true.  I suspect we will never be able to cleanly merge the core
of the Rx and Tx paths.  With luck though, we'll be able to share code
for implementing the wireless extensions, some other support routines,
and maybe parts of initialization.

-- 
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

Re: orinoco_usb Request For Comments

[Manuel Estrada Sainz]

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

On Thu, Jun 26, 2003 at 11:41:18PM +0200, Oliver Neukum wrote:
> 
> >  Please comment, how much of that or what else needs to be done to get
> >  it in the kernel?
> 
> 	if(dev->read.urb->status == -EINPROGRESS){
> 		warn("%s: Unlinking pending IN urb", __FUNCTION__);
> 		retval = bridge_remove_in_urb(dev);
> 		if(retval){
> 			dbg("retval %d status %d", retval,
> 			    dev->read.urb->status);
> 		}
> 	}
> 
> Unlink unconditionally.
> 
> 		/* We don't like racing :) */
> 		ctx->outurb->transfer_flags &= ~URB_ASYNC_UNLINK;
> 		usb_unlink_urb(ctx->outurb);
> 		del_timer_sync(&ctx->timer);
> 
> But neither do we like sleeping in interrupt. You can't simply unset the flag
> if somebody else may be needing it.
> 
> More when I am rested :-)

 How about the attached patch, not pretty, but it should work.

 Have a nice day

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

[-- Attachment #2: diff.diff --]
[-- Type: text/plain, Size: 1198 bytes --]

Index: orinoco_usb.c
===================================================================
RCS file: /usr/local/cvsroot/ranty/orinoco/driver/orinoco_usb.c,v
retrieving revision 1.80
diff -u -r1.80 orinoco_usb.c
--- orinoco_usb.c	25 Jun 2003 18:37:59 -0000	1.80
+++ orinoco_usb.c	27 Jun 2003 08:24:09 -0000
@@ -1858,13 +1858,9 @@
 	dev->udev = NULL;
 	//priv->hw_unavailable = 1;
 
-	if(dev->read.urb->status == -EINPROGRESS){
-		warn("%s: Unlinking pending IN urb", __FUNCTION__);
-		retval = bridge_remove_in_urb(dev);
-		if(retval){
-			dbg("retval %d status %d", retval,
-			    dev->read.urb->status);
-		}
+	retval = bridge_remove_in_urb(dev);
+	if (retval) {
+		dbg("retval %d status %d", retval, dev->read.urb->status);
 	}
 restart_list:
 	spin_lock_irqsave(&dev->ctxq.lock, flags);
@@ -1876,8 +1872,11 @@
 		spin_unlock_irqrestore(&dev->ctxq.lock, flags);
 		
 		/* We don't like racing :) */
-		ctx->outurb->transfer_flags &= ~URB_ASYNC_UNLINK;
 		usb_unlink_urb(ctx->outurb);
+		while (ctx->outurb->status == -EINPROGRESS) {
+			set_current_state (TASK_UNINTERRUPTIBLE);
+			schedule_timeout ((3  /*ms*/ * HZ)/1000)
+		}
 		del_timer_sync(&ctx->timer);
 		
 		if (!list_empty(&ctx->list))

Re: [Orinoco-devel] orinoco_usb Request For Comments

[Pavel Roskin]

On Fri, 27 Jun 2003, Manuel Estrada Sainz wrote:

>  Actually, orinoco-exp could be used as a test bed for monitor mode,
>  scanning, hermesap, ... and merge it back to the standard orinoco as it
>  probes to work right. For now it should be a test bed for USB support :)
[snip]
> > If you are going to create a separate driver, you should rename the
> > module.  I wouldn't bother with separate modules.  Just link hermes,
> > orinoco and orinoco_usb to one driver, say orinoco-usb.
>
>  No, I want to stay as similar to standard orinoco as possible to make
>  merging easier.

OK, I understand you are suggesting to fork an experimental branch.  Then
I suggest that we stop this discussion in LKML and return to orinoco-devel
to discuss the situation.

There is nothing wrong with the fork if all other ways to keep the code
together have been exhausted.  But since this wasn't discussed in the
orinoco-devel mailing list, I think it's too early to fork.

One thing we haven't considered is restructuring the code to separate
common and different parts of the USB and the non-USB drivers.

The firmware issue has been solved in the 2.5 kernels, so it shouldn't
prevent David from including your code.

-- 
Regards,
Pavel Roskin

Re: [Orinoco-devel] orinoco_usb Request For Comments

[Manuel Estrada Sainz]

On Fri, Jun 27, 2003 at 05:49:59PM -0400, Pavel Roskin wrote:
> On Fri, 27 Jun 2003, Manuel Estrada Sainz wrote:
> 
> >  Actually, orinoco-exp could be used as a test bed for monitor mode,
> >  scanning, hermesap, ... and merge it back to the standard orinoco as it
> >  probes to work right. For now it should be a test bed for USB support :)
> [snip]
> > > If you are going to create a separate driver, you should rename the
> > > module.  I wouldn't bother with separate modules.  Just link hermes,
> > > orinoco and orinoco_usb to one driver, say orinoco-usb.
> >
> >  No, I want to stay as similar to standard orinoco as possible to make
> >  merging easier.
> 
> OK, I understand you are suggesting to fork an experimental branch.  Then
> I suggest that we stop this discussion in LKML and return to orinoco-devel
> to discuss the situation.
> 
> There is nothing wrong with the fork if all other ways to keep the code
> together have been exhausted.  But since this wasn't discussed in the
> orinoco-devel mailing list, I think it's too early to fork.
> 
> One thing we haven't considered is restructuring the code to separate
> common and different parts of the USB and the non-USB drivers.
> 
> The firmware issue has been solved in the 2.5 kernels, so it shouldn't
> prevent David from including your code.

 Monday the latest I'll start a thread in orinoco-devel, unless you do
 it first :) 

 Regards

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: orinoco_usb Request For Comments

[Manuel Estrada Sainz]

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

On Fri, Jun 27, 2003 at 12:15:04PM +0200, Oliver Neukum wrote:
> 
> 
> On Fri, 27 Jun 2003, Manuel Estrada Sainz wrote:
> 
> > On Thu, Jun 26, 2003 at 11:41:18PM +0200, Oliver Neukum wrote:
> > > 		/* We don't like racing :) */
> > > 		ctx->outurb->transfer_flags &= ~URB_ASYNC_UNLINK;
> > > 		usb_unlink_urb(ctx->outurb);
> > > 		del_timer_sync(&ctx->timer);
> > >
> > > But neither do we like sleeping in interrupt. You can't simply unset the flag
> > > if somebody else may be needing it.
> > >
> > > More when I am rested :-)
> >
> >  How about the attached patch, not pretty, but it should work.
> 
> It is much too ugly. Please use a struct completion or a waitqueue.
 
 How about this?

 The other choice is to just wait on the completion unconditionally and
 let timers expire on their own if needed.
 
 That would probably be more robust, and waiting a few extra seconds on
 module removal (which would just happen when the card hangs) is
 probably OK.  What do you think?

 Thanks

 	Manuel

 PS: Ideas on how to make the PCMCIA vs. USB integration (specially the
 locking) cleaner would be very, very welcomed. 

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

[-- Attachment #2: orinoco-oliver.diff --]
[-- Type: text/plain, Size: 1631 bytes --]

Index: orinoco_usb.c
===================================================================
RCS file: /usr/local/cvsroot/ranty/orinoco/driver/orinoco_usb.c,v
retrieving revision 1.85
diff -u -r1.85 orinoco_usb.c
--- orinoco_usb.c	1 Jul 2003 23:52:11 -0000	1.85
+++ orinoco_usb.c	2 Jul 2003 10:00:50 -0000
@@ -400,7 +400,7 @@
 			netif_wake_queue(net_dev);
 			break;
 		}
-		complete(&ctx->done);
+		complete_all(&ctx->done);
 		bridge_request_context_put(ctx);
 		break;
 		
@@ -410,7 +410,7 @@
 			/* This is normal, as all request contexts get flushed
 			 * when the device is disconnected */
 			err("Called, CTLX not terminating, but device gone");
-			complete(&ctx->done);
+			complete_all(&ctx->done);
 			bridge_request_context_put(ctx);
 			break;
 		}
@@ -1881,13 +1881,9 @@
 	dev->udev = NULL;
 	//priv->hw_unavailable = 1;
 
-	if(dev->read.urb->status == -EINPROGRESS){
-		warn("%s: Unlinking pending IN urb", __FUNCTION__);
-		retval = bridge_remove_in_urb(dev);
-		if(retval){
-			dbg("retval %d status %d", retval,
-			    dev->read.urb->status);
-		}
+	retval = bridge_remove_in_urb(dev);
+	if (retval) {
+		dbg("retval %d status %d", retval, dev->read.urb->status);
 	}
 restart_list:
 	spin_lock_irqsave(&dev->ctxq.lock, flags);
@@ -1899,8 +1895,10 @@
 		spin_unlock_irqrestore(&dev->ctxq.lock, flags);
 		
 		/* We don't like racing :) */
-		ctx->outurb->transfer_flags &= ~URB_ASYNC_UNLINK;
-		usb_unlink_urb(ctx->outurb);
+		if (ctx->outurb->status == -EINPROGRESS) {
+			usb_unlink_urb(ctx->outurb);
+			wait_for_completion(&ctx->done);
+		}
 		del_timer_sync(&ctx->timer);
 		
 		if (!list_empty(&ctx->list))

Re: orinoco_usb Request For Comments

[Oliver Neukum]

Am Mittwoch, 2. Juli 2003 12:17 schrieb Manuel Estrada Sainz:
> On Fri, Jun 27, 2003 at 12:15:04PM +0200, Oliver Neukum wrote:
> > 
> > 
> > On Fri, 27 Jun 2003, Manuel Estrada Sainz wrote:
> > 
> > > On Thu, Jun 26, 2003 at 11:41:18PM +0200, Oliver Neukum wrote:
> > > > 		/* We don't like racing :) */
> > > > 		ctx->outurb->transfer_flags &= ~URB_ASYNC_UNLINK;
> > > > 		usb_unlink_urb(ctx->outurb);
> > > > 		del_timer_sync(&ctx->timer);
> > > >
> > > > But neither do we like sleeping in interrupt. You can't simply unset the flag
> > > > if somebody else may be needing it.
> > > >
> > > > More when I am rested :-)
> > >
> > >  How about the attached patch, not pretty, but it should work.
> > 
> > It is much too ugly. Please use a struct completion or a waitqueue.
>  
>  How about this?
> 
>  The other choice is to just wait on the completion unconditionally and
>  let timers expire on their own if needed.
>  
>  That would probably be more robust, and waiting a few extra seconds on
>  module removal (which would just happen when the card hangs) is
>  probably OK.  What do you think?

You also need this code path on hotunplugging.
Please take out the test for EINPROGRESS and examine the return
value of usb_unlink_urb() to decide whether you have to wait.
 
>  PS: Ideas on how to make the PCMCIA vs. USB integration (specially the
>  locking) cleaner would be very, very welcomed. 

I know too little about the PCMCIA cards. Sorry.

	Regards
		Oliver