Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752865Ab3FFQdn (ORCPT ); Thu, 6 Jun 2013 12:33:43 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:50314 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752574Ab3FFQdm (ORCPT ); Thu, 6 Jun 2013 12:33:42 -0400 Date: Thu, 6 Jun 2013 09:33:40 -0700 From: Greg KH To: Johan Hovold Cc: Tobias Winter , =?iso-8859-1?Q?Bj=F8rn?= Mork , Rob Landley , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3] USB: serial: make minor allocation dynamic Message-ID: <20130606163340.GA24144@kroah.com> References: <20130605175426.GA13461@kroah.com> <20130605175455.GB13461@kroah.com> <20130606121718.GJ2566@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130606121718.GJ2566@localhost> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4069 Lines: 131 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 > > > > 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 > > [...] > > > -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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/