[PATCH] USB speedtouch: don't open a connection if no firmware

[PATCH] USB speedtouch: don't open a connection if no firmware

[Duncan Sands]

People using Alcatel's closed source firmware loader "speedmgmt" may need
to sleep longer before launching pppd.

--- redux-2.5/drivers/usb/misc/speedtch.c	2003-04-08 08:49:33.000000000 +0200
+++ bollux-2.5/drivers/usb/misc/speedtch.c	2003-04-04 13:44:11.000000000 +0200
@@ -816,9 +816,6 @@
 		return -ENODEV;
 	}
 
-	if (!instance->firmware_loaded)
-		return -EAGAIN;
-
 	if (vcc->qos.aal != ATM_AAL5) {
 		dbg ("unsupported ATM type %d!", vcc->qos.aal);
 		return -EINVAL;
@@ -933,6 +930,11 @@
 	if (vcc->qos.aal != ATM_AAL5)
 		return -EINVAL;
 
+	if (!instance->firmware_loaded) {
+		dbg ("firmware not loaded!");
+		return -EAGAIN;
+	}
+
 	down (&instance->serialize); /* vs self, udsl_atm_close */
 
 	if (udsl_find_vcc (instance, vpi, vci)) {
@@ -967,13 +969,12 @@
 
 	dbg ("Allocated new SARLib vcc 0x%p with vpi %d vci %d", new, vpi, vci);
 
-	MOD_INC_USE_COUNT;
-
-	if (instance->firmware_loaded)
-		udsl_fire_receivers (instance);
+	udsl_fire_receivers (instance);
 
 	dbg ("udsl_atm_open successful");
 
+	MOD_INC_USE_COUNT;
+
 	return 0;
 }
 
@@ -1041,6 +1042,7 @@
 		int ret;
 
 		if ((ret = usb_set_interface (instance->usb_dev, 1, 1)) < 0) {
+			dbg ("usb_set_interface returned %d!", ret);
 			up (&instance->serialize);
 			return ret;
 		}

Re: [linux-usb-devel] [PATCH] USB speedtouch: don't open a connection if no firmware

[Greg KH]

On Tue, Apr 08, 2003 at 09:26:43AM +0200, Duncan Sands wrote:
> @@ -967,13 +969,12 @@
>  
>  	dbg ("Allocated new SARLib vcc 0x%p with vpi %d vci %d", new, vpi, vci);
>  
> -	MOD_INC_USE_COUNT;
> -
> -	if (instance->firmware_loaded)
> -		udsl_fire_receivers (instance);
> +	udsl_fire_receivers (instance);
>  
>  	dbg ("udsl_atm_open successful");
>  
> +	MOD_INC_USE_COUNT;
> +
>  	return 0;
>  }
>  

Any way you can convert this driver to not use MOD_INC_USE_COUNT, as
it's racy and not really supported anymore?  But if you _really_ have to
use it, you need to call it at the first possible chance to make any
race window smaller.

thanks,

greg k-h

Re: [linux-usb-devel] [PATCH] USB speedtouch: don't open a connection if no firmware

[Duncan Sands]

> > +	udsl_fire_receivers (instance);
> >
> >  	dbg ("udsl_atm_open successful");
> >
> > +	MOD_INC_USE_COUNT;
> > +
> >  	return 0;
> >  }
>
> Any way you can convert this driver to not use MOD_INC_USE_COUNT, as
> it's racy and not really supported anymore?  But if you _really_ have to
> use it, you need to call it at the first possible chance to make any
> race window smaller.

Hi Greg, I'm waiting on the fixes to the ATM layer (coming soon to a kernel
near you).  As for the position of MOD_INC_USE_COUNT, did you ever hear
of anyone getting bitten by a race like this?  If it makes you feel better, I
will move it up, probably just before I take the semaphore (since that is the
first place we can sleep).  I will do it tomorrow, OK?

All the best,

Duncan.

Re: [linux-usb-devel] [PATCH] USB speedtouch: don't open a connection if no firmware

[Greg KH]

On Tue, Apr 08, 2003 at 10:22:10PM +0200, Duncan Sands wrote:
> > > +	udsl_fire_receivers (instance);
> > >
> > >  	dbg ("udsl_atm_open successful");
> > >
> > > +	MOD_INC_USE_COUNT;
> > > +
> > >  	return 0;
> > >  }
> >
> > Any way you can convert this driver to not use MOD_INC_USE_COUNT, as
> > it's racy and not really supported anymore?  But if you _really_ have to
> > use it, you need to call it at the first possible chance to make any
> > race window smaller.
> 
> Hi Greg, I'm waiting on the fixes to the ATM layer (coming soon to a kernel
> near you).

Ah, ok, that makes sense.

> As for the position of MOD_INC_USE_COUNT, did you ever hear
> of anyone getting bitten by a race like this?  If it makes you feel better, I
> will move it up, probably just before I take the semaphore (since that is the
> first place we can sleep).  I will do it tomorrow, OK?

Yes, it needs to be before any function that can sleep.  I'll hold off
applying this patch then.

thanks,

greg k-h

Re: [linux-usb-devel] [PATCH] USB speedtouch: don't open a connection if no firmware

[Duncan Sands]

> > Hi Greg, I'm waiting on the fixes to the ATM layer (coming soon to a
> > kernel near you).
>
> Ah, ok, that makes sense.
>
> > As for the position of MOD_INC_USE_COUNT, did you ever hear
> > of anyone getting bitten by a race like this?  If it makes you feel
> > better, I will move it up, probably just before I take the semaphore
> > (since that is the first place we can sleep).  I will do it tomorrow, OK?
>
> Yes, it needs to be before any function that can sleep.  I'll hold off
> applying this patch then.

How about this one instead.  MOD_INC_USE_COUNT is placed before I call
any functions that can sleep.  Let's just hope that the call to me doesn't
come after some sleeping in the higher layers...

Duncan.

--- redux-2.5/drivers/usb/misc/speedtch.c	2003-04-08 08:49:33.000000000 +0200
+++ bollux-2.5/drivers/usb/misc/speedtch.c	2003-04-09 10:08:27.000000000 +0200
@@ -933,15 +933,24 @@
 	if (vcc->qos.aal != ATM_AAL5)
 		return -EINVAL;
 
+	if (!instance->firmware_loaded) {
+		dbg ("firmware not loaded!");
+		return -EAGAIN;
+	}
+
+	MOD_INC_USE_COUNT;
+
 	down (&instance->serialize); /* vs self, udsl_atm_close */
 
 	if (udsl_find_vcc (instance, vpi, vci)) {
 		up (&instance->serialize);
+		MOD_DEC_USE_COUNT;
 		return -EADDRINUSE;
 	}
 
 	if (!(new = kmalloc (sizeof (struct udsl_vcc_data), GFP_KERNEL))) {
 		up (&instance->serialize);
+		MOD_DEC_USE_COUNT;
 		return -ENOMEM;
 	}
 
@@ -967,10 +976,7 @@
 
 	dbg ("Allocated new SARLib vcc 0x%p with vpi %d vci %d", new, vpi, vci);
 
-	MOD_INC_USE_COUNT;
-
-	if (instance->firmware_loaded)
-		udsl_fire_receivers (instance);
+	udsl_fire_receivers (instance);
 
 	dbg ("udsl_atm_open successful");
 
@@ -1041,6 +1047,7 @@
 		int ret;
 
 		if ((ret = usb_set_interface (instance->usb_dev, 1, 1)) < 0) {
+			dbg ("usb_set_interface returned %d!", ret);
 			up (&instance->serialize);
 			return ret;
 		}

Re: [linux-usb-devel] [PATCH] USB speedtouch: don't open a connection if no firmware

[Greg KH]

On Wed, Apr 09, 2003 at 10:28:25AM +0200, Duncan Sands wrote:
> > > Hi Greg, I'm waiting on the fixes to the ATM layer (coming soon to a
> > > kernel near you).
> >
> > Ah, ok, that makes sense.
> >
> > > As for the position of MOD_INC_USE_COUNT, did you ever hear
> > > of anyone getting bitten by a race like this?  If it makes you feel
> > > better, I will move it up, probably just before I take the semaphore
> > > (since that is the first place we can sleep).  I will do it tomorrow, OK?
> >
> > Yes, it needs to be before any function that can sleep.  I'll hold off
> > applying this patch then.
> 
> How about this one instead.  MOD_INC_USE_COUNT is placed before I call
> any functions that can sleep.  Let's just hope that the call to me doesn't
> come after some sleeping in the higher layers...

Much better, thanks.  Applied.

greg k-h

Re: [linux-usb-devel] [PATCH] USB speedtouch: don't open a connection if no firmware

[Duncan Sands]

On Tuesday 08 April 2003 23:16, Dwaine_Garden@wsib.on.ca wrote:
> I have noticed with the usbvision code, that when a device like it's own
> is called.  Webcam (Bttv) and the usbvision device.   It take forever to
> load and initialize the driver.
>
> People complain it sometimes it takes two minutes.   At first I thought
> they were making up things.
>
> I have another video and audio capture device.   I only see problem when
> both device have their drivers loaded.
>
> I'm like you.  I'm going to get to removing all the MOD_INC_USE_COUNT;
> code.

Hi Dwaine, I don't understand your email too well, sorry.  Many USB devices
take a while before they are usable, because first you need to load some
firmware into them.  MOD_INC_USE_COUNT will not slow down initialisation.
It is to do with unloading the driver, and stops the driver being unloaded when
it would be dangerous to do so.

All the best,

Duncan.