Re: [PATCH 2/3] USB: serial: make minor allocation dynamic

[Johan Hovold]

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.

> +		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?

>  }
>  
>  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,
Johan
--
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/