On Tue, Jun 04, 2013 at 01:04:01PM +0200, Johan Hovold wrote:
> On Mon, Jun 03, 2013 at 07:49:59PM -0700, 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.
>
> I'm afraid this won't work in it's current form. Several drivers and
> parts of usb-serial core still depend on the minor numbers being
> consecutive for multi-port devices.
You are right, let me go fix up the "port->number" assumption first,
which will let this become easier. That should have been fixed a long
time ago.
> There are also still references to SERIAL_TTY_NO_MINOR (255) as well
> as SERIAL_TTY_MINORS that need to be addressed.
Yeah, I knew it was too good to be true that a simple 45 line patch
would solve this properly :)
> > + port = idr_find(&serial_minors, index);
> > mutex_unlock(&table_lock);
> > + if (!port)
> > + return NULL;
> > +
> > + serial = port->serial;
> > + kref_get(&serial->kref);
> > return serial;
> > }
>
> We would still need to handle disconnect, and make sure to return with
> the disc_mutex held unless disconnected.
Alan noticed this as well, I've now fixed that, thanks.
> > + dev_dbg(&serial->interface->dev, "%s %d\n", __func__, num_ports);
> > +
> > + *minor = 0xffffffff;
> > + for (i = 0; i < num_ports; ++i) {
> > + x = get_free_port(serial->port[i]);
> > + if (x < 0)
> > + goto error;
> > + if (*minor == 0xffffffff)
> > + *minor = x;
> > }
> > - mutex_unlock(&table_lock);
> > - return NULL;
> > + return 0;
> > +error:
> > + // FIXME unwind the already allocated minors
> > + return -ENODEV;
> > }
>
> As mentioned above, usb-serial core and several drivers currently depend
> on us returning the first minor in a consecutive range. It's mostly used
> to determine the per device port index, so storing that index in the port
> structure could possibly be sufficient.
Yes, I'll go fix that up first, before making these changes, as it's
independant.
> > @@ -1233,9 +1226,6 @@ static int __init usb_serial_init(void)
> > return -ENOMEM;
> >
> > /* Initialize our global data */
>
> You could remove the above comment as well.
>
> > - for (i = 0; i < SERIAL_TTY_MINORS; ++i)
> > - serial_table[i] = NULL;
> > -
> > result = bus_register(&usb_serial_bus_type);
I thought about it, but we also register the bus variables, and some
other things here as well, so I left it in.
thanks for the review.
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
