Re: [RFC] raise the maximum number of usb-serial devices to 512

[Greg KH]

On Tue, Jun 04, 2013 at 10:13:47AM -0400, Alan Stern wrote:
> On Mon, 3 Jun 2013, Greg KH wrote:
> 
> > On Mon, May 27, 2013 at 02:28:51PM +0200, Bj?rn Mork wrote:
> > > But, IMHO, a nicer approach would be to make the allocation completely
> > > dynamic, using e.g. the idr subsystem. Static tables are always feel
> > > like straight jackets to me, no matter how big they are :)
> > 
> > You are right, I didn't change the code to use idr (it predates idr by
> > about a decade or so), because I thought we needed the "rage" logic that
> > the usb-serial minor reservation does.
> > 
> > But I'm not so sure anymore, so here's a patch to change to use the idr
> > code, and should remove all minor number limitations (well 65k is the
> > limit the tty core should be setting I think.)
> > 
> > Tobias, can you test this patch out?  Note, I only compiled it, did not
> > get the chance to actually run it, so it might not work at all.
> > 
> > thanks,
> > 
> > greg k-h
> 
> 
> > @@ -61,59 +62,52 @@ static LIST_HEAD(usb_serial_driver_list);
> >  struct usb_serial *usb_serial_get_by_index(unsigned index)
> >  {
> >  	struct usb_serial *serial;
> > +	struct usb_serial_port *port;
> >  
> >  	mutex_lock(&table_lock);
> > -	serial = serial_table[index];
> > -
> > -	if (serial) {
> > -		mutex_lock(&serial->disc_mutex);
> > -		if (serial->disconnected) {
> > -			mutex_unlock(&serial->disc_mutex);
> > -			serial = NULL;
> > -		} else {
> > -			kref_get(&serial->kref);
> > -		}
> > -	}
> > +	port = idr_find(&serial_minors, index);
> >  	mutex_unlock(&table_lock);
> > +	if (!port)
> > +		return NULL;
> > +
> > +	serial = port->serial;
> > +	kref_get(&serial->kref);
> >  	return serial;
> >  }
> 
> The test for serial->disconnected got lost.  And the locking isn't 
> right; the routine is documented to return with serial->disc_mutex held 
> (in the case where the device hasn't been disconnected).
> 
> Also, the kref_get() needs to occur within the scope of the table_lock.

Thanks, for some reason I ignored this when converting the code, that's
what I get for not even testing...

> I didn't check the rest of the patch for similar errors.  Finding three 
> in the first function seemed like enough.  :-)

Fair enough, I've now fixed this up, and will see if it runs properly.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/