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

2013-06-18 Thread Johan Hovold 
   @@ -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.
 
 Really?  Why were we doing this type of thing before with the not
 allocated flag?  It seems that we were protecting some path that I
 can't remember at the moment.  So to be safe, I'll leave it for now...

It was and is only used when releasing the serial struct to check
whether minors had been allocated or not at probe and if return_serial
(release_minors) should be called. This in done in destroy_serial just
before freeing the struct, so clearing the flag is redundant, but
doesn't hurt anyone, I guess. ;)

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 v3 2/3] USB: serial: make minor allocation dynamic

2013-06-17 Thread Greg KH 
On Sat, Jun 08, 2013 at 12:03:47PM +0200, Johan Hovold wrote:
 On Fri, Jun 07, 2013 at 11:04:28AM -0700, Greg KH wrote:
  From: Greg Kroah-Hartman gre...@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 gre...@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.

Now fixed, thanks.

   static void return_serial(struct usb_serial *serial)
 
 Perhaps rename this one release_minors to match allocate_minors (much
 better name btw)?

Good idea, now done.

 
  @@ -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.

Really?  Why were we doing this type of thing before with the not
allocated flag?  It seems that we were protecting some path that I
can't remember at the moment.  So to be safe, I'll leave it for now...

 All three patches look good otherwise. The port-number disambiguation
 was indeed long overdue. Feel free to add
 
 Reviewed-by: Johan Hovold jhov...@gmail.com

Thanks so much for the review, I'll go make these changes and apply them
now.

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 v3 2/3] USB: serial: make minor allocation dynamic

2013-06-08 Thread Johan Hovold 
On Fri, Jun 07, 2013 at 11:04:28AM -0700, Greg KH wrote:
 From: Greg Kroah-Hartman gre...@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 gre...@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

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

2013-06-07 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

---
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()


 drivers/staging/serqt_usb2/serqt_usb2.c |   15 +---
 drivers/usb/serial/ark3116.c|2 
 drivers/usb/serial/console.c|6 -
 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 |  119 ++--
 drivers/usb/serial/usb_wwan.c   |2 
 drivers/usb/serial/whiteheat.c  |2 
 include/linux/usb/serial.h  |6 -
 17 files changed, 80 insertions(+), 97 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 =