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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
