On Thu, Jun 06, 2013 at 02:17:18PM +0200, Johan Hovold wrote:
> On Wed, Jun 05, 2013 at 10:54:55AM -0700, Greg KH wrote:
> > From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >
> > This moves the allocation of minor device numbers from a static array to
> > be dynamic, using the idr interface. This means that you could
> > potentially get "gaps" in a minor number range for a single USB serial
> > device with multiple ports, but all should still work properly.
> >
> > Note, we still have the limitation of 255 USB to serial devices in the
> > system, as that is all we are registering with the TTY layer at this
> > point in time.
> >
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> [...]
>
> > -static struct usb_serial *get_free_serial(struct usb_serial *serial,
> > - int num_ports, unsigned int *minor)
> > +static int get_free_port(struct usb_serial_port *port)
> > {
> > - unsigned int i, j;
> > - int good_spot;
> > -
> > - dev_dbg(&serial->interface->dev, "%s %d\n", __func__, num_ports);
> > + int i;
> >
> > - *minor = 0;
> > mutex_lock(&table_lock);
> > - for (i = 0; i < SERIAL_TTY_MINORS; ++i) {
> > - if (serial_table[i])
> > - continue;
> > + i = idr_alloc(&serial_minors, port, 0, 0, GFP_KERNEL);
> > + if (i < 0)
> > + goto exit;
> > + port->minor = i;
> > +exit:
> > + mutex_unlock(&table_lock);
> > + return i;
> > +}
> >
> > - good_spot = 1;
> > - for (j = 1; j <= num_ports-1; ++j)
> > - if ((i+j >= SERIAL_TTY_MINORS) || (serial_table[i+j])) {
> > - good_spot = 0;
> > - i += j;
> > - break;
> > - }
> > - if (good_spot == 0)
> > - continue;
> > +static int get_free_serial(struct usb_serial *serial, int num_ports,
> > + unsigned int *minor)
> > +{
> > + unsigned int i;
> > + unsigned int j;
> > + int x;
> >
> > - *minor = i;
> > - j = 0;
> > - dev_dbg(&serial->interface->dev, "%s - minor base = %d\n", __func__, *minor);
> > - for (i = *minor; (i < (*minor + num_ports)) && (i < SERIAL_TTY_MINORS); ++i, ++j) {
> > - serial_table[i] = serial;
> > - serial->port[j]->minor = i;
> > - serial->port[j]->port_number = i - *minor;
> > - }
> > - mutex_unlock(&table_lock);
> > - return serial;
> > + dev_dbg(&serial->interface->dev, "%s %d\n", __func__, num_ports);
> > +
> > + *minor = 0xffffffff;
>
> You could use SERIAL_TTY_NO_MINOR here -- if needed at all, as it has
> already been set in create_serial.
>
> > + for (i = 0; i < num_ports; ++i) {
> > + x = get_free_port(serial->port[i]);
> > + if (x < 0)
> > + goto error;
> > + if (*minor == 0xffffffff)
> > + *minor = x;
>
> We must not update *minor until all port minors have been allocated, or
> idr_remove might get called for unallocated minors or even minor numbers
> of other ports in return_serial when the serial struct is destroyed.
Good point. In looking at this further, I really need to drop the
usb_serial structure's minor field completly, as it doesn't make sense
anymore.
I'll go rework all of that and post a v2 of this series, thanks.
> > + serial->port[i]->port_number = i;
> > }
> > - mutex_unlock(&table_lock);
> > - return NULL;
> > + return 0;
> > +error:
> > + /* unwind the already allocated minors */
> > + for (j = 0; j < i; ++j)
> > + idr_remove(&serial_minors, serial->port[j]->minor);
> > + return x;
>
> table_lock?
Good catch, now fixed.
> > }
> >
> > static void return_serial(struct usb_serial *serial)
> > @@ -123,7 +128,7 @@ static void return_serial(struct usb_ser
> >
> > mutex_lock(&table_lock);
> > for (i = 0; i < serial->num_ports; ++i)
> > - serial_table[serial->minor + i] = NULL;
> > + idr_remove(&serial_minors, serial->port[i]->minor);
> > mutex_unlock(&table_lock);
> > }
> >
>
> [...]
>
> Looks good otherwise.
Thanks for the review, much appreciated.
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
