lkml.org 
[lkml]   [2013]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 2/3] USB: serial: make minor allocation dynamic
    On Fri, Jun 07, 2013 at 11:04:28AM -0700, Greg KH wrote:
    > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

    > Changes v2 - v3:
    > - fixed up comments about usb_serial_get_by_minor()
    > - fixed error case in usb_serial_get_by_minor()
    > - folded get_free_port() into get_free_serial()
    > - renamed get_free_serial() to allocate_minors()
    > - fixed console.c build breakage
    > - properly pass in minor port number to usb_serial_console_init()

    > --- a/drivers/usb/serial/usb-serial.c
    > +++ b/drivers/usb/serial/usb-serial.c
    > @@ -37,6 +37,7 @@
    > #include <linux/usb.h>
    > #include <linux/usb/serial.h>
    > #include <linux/kfifo.h>
    > +#include <linux/idr.h>
    > #include "pl2303.h"
    >
    > #define DRIVER_AUTHOR "Greg Kroah-Hartman <gregkh@linuxfoundation.org>"
    > @@ -49,72 +50,64 @@
    > drivers depend on it.
    > */
    >
    > -static struct usb_serial *serial_table[SERIAL_TTY_MINORS];
    > +static DEFINE_IDR(serial_minors);
    > static DEFINE_MUTEX(table_lock);
    > static LIST_HEAD(usb_serial_driver_list);
    >
    > /*
    > - * Look up the serial structure. If it is found and it hasn't been
    > - * disconnected, return with its disc_mutex held and its refcount
    > - * incremented. Otherwise return NULL.
    > + * Look up the serial port structure. If it is found and it hasn't been
    > + * disconnected, return with the parent usb_serial structure's disc_mutex held
    > + * and its refcount incremented. Otherwise return NULL.
    > */
    > -struct usb_serial *usb_serial_get_by_index(unsigned index)
    > +struct usb_serial_port *usb_serial_port_get_by_minor(unsigned minor)
    > {
    > - struct usb_serial *serial;
    > + struct usb_serial *serial = NULL;

    This isn't necessary anymore.

    > + struct usb_serial_port *port;
    >
    > mutex_lock(&table_lock);
    > - serial = serial_table[index];
    > + port = idr_find(&serial_minors, minor);
    > + if (!port)
    > + goto exit;
    >
    > - if (serial) {
    > - mutex_lock(&serial->disc_mutex);
    > - if (serial->disconnected) {
    > - mutex_unlock(&serial->disc_mutex);
    > - serial = NULL;
    > - } else {
    > - kref_get(&serial->kref);
    > - }
    > + serial = port->serial;
    > + mutex_lock(&serial->disc_mutex);
    > + if (serial->disconnected) {
    > + mutex_unlock(&serial->disc_mutex);
    > + port = NULL;
    > + } else {
    > + kref_get(&serial->kref);
    > }
    > +exit:
    > mutex_unlock(&table_lock);
    > - return serial;
    > + return port;
    > }
    >
    > -static struct usb_serial *get_free_serial(struct usb_serial *serial,
    > - int num_ports, unsigned int *minor)
    > +static int allocate_minors(struct usb_serial *serial, int num_ports)
    > {
    > + struct usb_serial_port *port;
    > unsigned int i, j;
    > - int good_spot;
    > + int minor;
    >
    > dev_dbg(&serial->interface->dev, "%s %d\n", __func__, num_ports);
    >
    > - *minor = 0;
    > mutex_lock(&table_lock);
    > - for (i = 0; i < SERIAL_TTY_MINORS; ++i) {
    > - if (serial_table[i])
    > - continue;
    > -
    > - 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;
    > -
    > - *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;
    > + for (i = 0; i < num_ports; ++i) {
    > + port = serial->port[i];
    > + minor = idr_alloc(&serial_minors, port, 0, 0, GFP_KERNEL);
    > + if (minor < 0)
    > + goto error;
    > + port->minor = minor;
    > + port->port_number = i;
    > }
    > + serial->minors_reserved = 1;
    > 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);
    > + mutex_unlock(&table_lock);
    > + return minor;
    > }
    >
    > static void return_serial(struct usb_serial *serial)

    Perhaps rename this one release_minors to match allocate_minors (much
    better name btw)?

    > @@ -123,8 +116,9 @@ 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);
    > + serial->minors_reserved = 0;

    This isn't strictly needed as the serial struct release_serial is only
    called once when the struct is about to be freed.

    > }
    >
    > static void destroy_serial(struct kref *kref)
    > @@ -136,7 +130,7 @@ static void destroy_serial(struct kref *
    > serial = to_usb_serial(kref);
    >
    > /* return the minor range that this device had */
    > - if (serial->minor != SERIAL_TTY_NO_MINOR)
    > + if (serial->minors_reserved)
    > return_serial(serial);
    >
    > if (serial->attached && serial->type->release)

    All three patches look good otherwise. The port-number disambiguation
    was indeed long overdue. Feel free to add

    Reviewed-by: Johan Hovold <jhovold@gmail.com>

    Thanks,
    Johan


    \
     
     \ /
      Last update: 2013-06-08 12:21    [W:6.352 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site