[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