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

2013-06-07 Thread Johan Hovold 
On Thu, Jun 06, 2013 at 10:31:21AM -0700, Greg KH wrote:
 From: Greg Kroah-Hartman gre...@linuxfoundation.org

[...]

 --- 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 gre...@linuxfoundation.org
 @@ -49,72 +50,75 @@
 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
 + * Look up the serial port structure.  If it is found and it hasn't been
   * disconnected, return with its disc_mutex held and its refcount

Second sentence needs to be updated as well as it is the disc_mutex and
refcount of the owning usb_serial struct we're referring to.

   * 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;
 + 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);
 + serial = NULL;

You want to set port rather than serial to NULL here now.

 + } 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 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 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);
 +
 + for (i = 0; i  num_ports; ++i) {
 + x = get_free_port(serial-port[i]);
 + if (x  0)
 + goto error;
 + serial-port[i]-port_number = i;
   }

What do you think about removing get_free_port altogether and simply
call idr_alloc directly in the loop above instead? This would have the
benefit of only acquiring the table_lock once per device, which would
also prevent the possibility of higher port numbers receiving smaller
minors (e.g. due to a parallel disconnect).

 + serial-minors_reserved = 1;
 + return 0;
 +error:
 + /* unwind the already allocated minors */
 + mutex_lock(table_lock);
 + for (j = 0; j  i; ++j)
 + idr_remove(serial_minors, serial-port[j]-minor);
   mutex_unlock(table_lock);
 - return NULL;
 + return x;
  }

[...]

 --- a/include/linux/usb/serial.h
 +++ b/include/linux/usb/serial.h
 @@ -21,7 +21,6 @@
  
  #define SERIAL_TTY_MAJOR 188 /* Nice legal number now */
  #define SERIAL_TTY_MINORS254 /* loads of devices :) */
 -#define

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

2013-06-07 Thread Johan Hovold 
On Fri, Jun 07, 2013 at 11:40:25AM +0200, Johan Hovold wrote:
 On Thu, Jun 06, 2013 at 10:31:21AM -0700, Greg KH wrote:
  From: Greg Kroah-Hartman gre...@linuxfoundation.org
 
 [...]
 
   /* Functions needed by other parts of the usbserial core */
  -extern struct usb_serial *usb_serial_get_by_index(unsigned int minor);
  +extern struct usb_serial_port *usb_serial_port_get_by_minor(unsigned int 
  minor);
 
 As Tobias already noted, this breaks the USB console driver. However, it
 looks like the call in that driver is not really needed and that
 therefore usb_serial_port_get_by_minor need not be exported at all.

Forget that last sentence. It is still needed, of course.

Johan
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-07 Thread Johan Hovold 
On Thu, Jun 06, 2013 at 10:31:21AM -0700, Greg KH wrote:
 From: Greg Kroah-Hartman gre...@linuxfoundation.org

 @@ -1040,11 +1044,10 @@ static int usb_serial_probe(struct usb_i
*/
   serial-disconnected = 1;
  
 - if (get_free_serial(serial, num_ports, minor) == NULL) {
 + if (get_free_serial(serial, num_ports)) {
   dev_err(ddev, No more free serial devices\n);
   goto probe_error;
   }
 - serial-minor = minor;

This gives a warning as minor is no longer initialised, but is still
used to initialise the console a bit further down.

usb_serial_console_init(minor);

Should probably just drop minor, and use serial-port[0]-minor instead.

Thanks,
Johan
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-07 Thread Greg KH 
On Fri, Jun 07, 2013 at 12:00:47PM +0200, Johan Hovold wrote:
 On Thu, Jun 06, 2013 at 10:31:21AM -0700, Greg KH wrote:
  From: Greg Kroah-Hartman gre...@linuxfoundation.org
 
  @@ -1040,11 +1044,10 @@ static int usb_serial_probe(struct usb_i
   */
  serial-disconnected = 1;
   
  -   if (get_free_serial(serial, num_ports, minor) == NULL) {
  +   if (get_free_serial(serial, num_ports)) {
  dev_err(ddev, No more free serial devices\n);
  goto probe_error;
  }
  -   serial-minor = minor;
 
 This gives a warning as minor is no longer initialised, but is still
 used to initialise the console a bit further down.
 
   usb_serial_console_init(minor);
 
 Should probably just drop minor, and use serial-port[0]-minor instead.

Ah crap, I knew I was going to have to convert the console code, but I
just forgot about it as I didn't have it selected so I didn't get any
build errors...

I'll go fix that up and do a v3 of the patches...

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-07 Thread Greg KH 
On Fri, Jun 07, 2013 at 11:40:25AM +0200, Johan Hovold wrote:
 On Thu, Jun 06, 2013 at 10:31:21AM -0700, Greg KH wrote:
  From: Greg Kroah-Hartman gre...@linuxfoundation.org
 
 [...]
 
  --- 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 gre...@linuxfoundation.org
  @@ -49,72 +50,75 @@
  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
  + * Look up the serial port structure.  If it is found and it hasn't been
* disconnected, return with its disc_mutex held and its refcount
 
 Second sentence needs to be updated as well as it is the disc_mutex and
 refcount of the owning usb_serial struct we're referring to.

Good point, now updated.

* 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;
  +   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);
  +   serial = NULL;
 
 You want to set port rather than serial to NULL here now.

Good eye, now fixed.

  +   } 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 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 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);
  +
  +   for (i = 0; i  num_ports; ++i) {
  +   x = get_free_port(serial-port[i]);
  +   if (x  0)
  +   goto error;
  +   serial-port[i]-port_number = i;
  }
 
 What do you think about removing get_free_port altogether and simply
 call idr_alloc directly in the loop above instead? This would have the
 benefit of only acquiring the table_lock once per device, which would
 also prevent the possibility of higher port numbers receiving smaller
 minors (e.g. due to a parallel disconnect).

That's a good idea.  I've also renamed this function to allocate_minors
as that's what it really is doing.

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-06 Thread Greg KH 
From: Greg Kroah-Hartman gre...@linuxfoundation.org

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.

We remove the 'minor' field from the usb_serial structure, as it no
longer makes any sense for it (use the field in the usb_serial_port
structure if you really want to know this number), and take the fact
that we were overloading a number in this field to determine if we had
initialized the minor numbers or not, and just use a flag variable
instead.

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 gre...@linuxfoundation.org

---
 drivers/staging/serqt_usb2/serqt_usb2.c |   15 +--
 drivers/usb/serial/ark3116.c|2 
 drivers/usb/serial/f81232.c |2 
 drivers/usb/serial/io_edgeport.c|2 
 drivers/usb/serial/io_ti.c  |2 
 drivers/usb/serial/mos7720.c|2 
 drivers/usb/serial/mos7840.c|7 -
 drivers/usb/serial/opticon.c|2 
 drivers/usb/serial/pl2303.c |2 
 drivers/usb/serial/quatech2.c   |2 
 drivers/usb/serial/ssu100.c |2 
 drivers/usb/serial/ti_usb_3410_5052.c   |2 
 drivers/usb/serial/usb-serial.c |  123 +++-
 drivers/usb/serial/usb_wwan.c   |2 
 drivers/usb/serial/whiteheat.c  |2 
 include/linux/usb/serial.h  |6 -
 16 files changed, 85 insertions(+), 90 deletions(-)

--- a/drivers/staging/serqt_usb2/serqt_usb2.c
+++ b/drivers/staging/serqt_usb2/serqt_usb2.c
@@ -906,7 +906,7 @@ static int qt_open(struct tty_struct *tt
qt_submit_urb_from_open(serial, port);
}
 
-   dev_dbg(port-dev, serial number is %d\n, port-serial-minor);
+   dev_dbg(port-dev, minor number is %d\n, port-minor);
dev_dbg(port-dev,
Bulkin endpoint is %d\n, port-bulk_in_endpointAddress);
dev_dbg(port-dev,
@@ -1002,7 +1002,7 @@ static void qt_close(struct usb_serial_p
status = 0;
 
tty = tty_port_tty_get(port-port);
-   index = tty-index - serial-minor;
+   index = port-port_number;
 
qt_port = qt_get_port_private(port);
port0 = qt_get_port_private(serial-port[0]);
@@ -1129,12 +1129,11 @@ static int qt_ioctl(struct tty_struct *t
 {
struct usb_serial_port *port = tty-driver_data;
struct quatech_port *qt_port = qt_get_port_private(port);
-   struct usb_serial *serial = get_usb_serial(port, __func__);
unsigned int index;
 
dev_dbg(port-dev, %s cmd 0x%04x\n, __func__, cmd);
 
-   index = tty-index - serial-minor;
+   index = port-port_number;
 
if (cmd == TIOCMIWAIT) {
while (qt_port != NULL) {
@@ -1180,7 +1179,7 @@ static void qt_set_termios(struct tty_st
int baud, divisor, remainder;
int status;
 
-   index = tty-index - port-serial-minor;
+   index = port-port_number;
 
switch (cflag  CSIZE) {
case CS5:
@@ -1296,7 +1295,7 @@ static void qt_break(struct tty_struct *
u16 index, onoff;
unsigned int result;
 
-   index = tty-index - serial-minor;
+   index = port-port_number;
 
qt_port = qt_get_port_private(port);
 
@@ -1325,7 +1324,7 @@ static inline int qt_real_tiocmget(struc
int status;
unsigned int index;
 
-   index = tty-index - serial-minor;
+   index = port-port_number;
status =
BoxGetRegister(port-serial, index, MODEM_CONTROL_REGISTER, mcr);
if (status = 0) {
@@ -1364,7 +1363,7 @@ static inline int qt_real_tiocmset(struc
int status;
unsigned int index;
 
-   index = tty-index - serial-minor;
+   index = port-port_number;
status =
BoxGetRegister(port-serial, index, MODEM_CONTROL_REGISTER, mcr);
if (status  0)
--- a/drivers/usb/serial/ark3116.c
+++ b/drivers/usb/serial/ark3116.c
@@ -413,7 +413,7 @@ static int ark3116_ioctl(struct tty_stru
/* XXX: Some of these values are probably wrong. */
memset(serstruct, 0, sizeof(serstruct));
serstruct.type = PORT_16654;
-   serstruct.line = port-serial-minor;
+   serstruct.line = port-minor;
serstruct.port = port-port_number;
serstruct.custom_divisor = 0;
serstruct.baud_base = 460800;
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -294,7 +294,7 @@ static int f81232_ioctl(struct tty_struc
case TIOCGSERIAL:
memset(ser, 0, sizeof ser);
ser.type = PORT_16654;
-   ser.line =