[PATCH] Raise the maximum number of usb-serial devices to 256

[PATCH] Raise the maximum number of usb-serial devices to 256

[Tobias Winter]

Raise the maximum number of usb-serial devices to 256, which is the
actual limit supported by the codebase.

Signed-off-by: Jakob-Tobias Winter <tobias@linuxdingsda.de>
Tested-by: Jakob-Tobias Winter <tobias@linuxdingsda.de>

---
 include/linux/usb/serial.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 302ddf5..c0ce5ed 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -20,7 +20,7 @@
 #include <linux/kfifo.h>

 #define SERIAL_TTY_MAJOR       188     /* Nice legal number now */
-#define SERIAL_TTY_MINORS      254     /* loads of devices :) */
+#define SERIAL_TTY_MINORS      256     /* loads of devices :) */
 #define SERIAL_TTY_NO_MINOR    255     /* No minor was assigned */

 /* The maximum number of ports one device can grab at once */
-- 
1.7.10.4

Re: [PATCH] Raise the maximum number of usb-serial devices to 256

[Rob Landley]

On 05/27/2013 04:30:12 AM, Tobias Winter wrote:
> Raise the maximum number of usb-serial devices to 256, which is the
> actual limit supported by the codebase.
> 
> Signed-off-by: Jakob-Tobias Winter <tobias@linuxdingsda.de>
> Tested-by: Jakob-Tobias Winter <tobias@linuxdingsda.de>
> 
> ---
>  include/linux/usb/serial.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
> index 302ddf5..c0ce5ed 100644
> --- a/include/linux/usb/serial.h
> +++ b/include/linux/usb/serial.h
> @@ -20,7 +20,7 @@
>  #include <linux/kfifo.h>
> 
>  #define SERIAL_TTY_MAJOR       188     /* Nice legal number now */
> -#define SERIAL_TTY_MINORS      254     /* loads of devices :) */
> +#define SERIAL_TTY_MINORS      256     /* loads of devices :) */
>  #define SERIAL_TTY_NO_MINOR    255     /* No minor was assigned */

So SERIAL_TTY_NO_MINOR is now a valid minor?

Rob

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Greg KH]

On Mon, May 27, 2013 at 02:28:51PM +0200, Bjørn Mork wrote:
> But, IMHO, a nicer approach would be to make the allocation completely
> dynamic, using e.g. the idr subsystem. Static tables are always feel
> like straight jackets to me, no matter how big they are :)

You are right, I didn't change the code to use idr (it predates idr by
about a decade or so), because I thought we needed the "rage" logic that
the usb-serial minor reservation does.

But I'm not so sure anymore, so here's a patch to change to use the idr
code, and should remove all minor number limitations (well 65k is the
limit the tty core should be setting I think.)

Tobias, can you test this patch out?  Note, I only compiled it, did not
get the chance to actually run it, so it might not work at all.

thanks,

greg k-h

Subject: [PATCH] usb: serial: remove minor number limitation of 255


Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

 drivers/usb/serial/usb-serial.c |   86 +++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 48 deletions(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 4753c00..74b6f08 100644
--- 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,7 +50,7 @@
    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);
 
@@ -61,59 +62,52 @@ static LIST_HEAD(usb_serial_driver_list);
 struct usb_serial *usb_serial_get_by_index(unsigned index)
 {
 	struct usb_serial *serial;
+	struct usb_serial_port *port;
 
 	mutex_lock(&table_lock);
-	serial = serial_table[index];
-
-	if (serial) {
-		mutex_lock(&serial->disc_mutex);
-		if (serial->disconnected) {
-			mutex_unlock(&serial->disc_mutex);
-			serial = NULL;
-		} else {
-			kref_get(&serial->kref);
-		}
-	}
+	port = idr_find(&serial_minors, index);
 	mutex_unlock(&table_lock);
+	if (!port)
+		return NULL;
+
+	serial = port->serial;
+	kref_get(&serial->kref);
 	return serial;
 }
 
-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)
+		return -EEXIST;
+	port->number = i;
+	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 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) {
-			serial_table[i] = serial;
-			serial->port[j++]->number = i;
-		}
-		mutex_unlock(&table_lock);
-		return serial;
+	dev_dbg(&serial->interface->dev, "%s %d\n", __func__, num_ports);
+
+	*minor = 0xffffffff;
+	for (i = 0; i < num_ports; ++i) {
+		x = get_free_port(serial->port[i]);
+		if (x < 0)
+			goto error;
+		if (*minor == 0xffffffff)
+			*minor = x;
 	}
-	mutex_unlock(&table_lock);
-	return NULL;
+	return 0;
+error:
+	// FIXME unwind the already allocated minors
+	return -ENODEV;
 }
 
 static void return_serial(struct usb_serial *serial)
@@ -122,7 +116,7 @@ static void return_serial(struct usb_serial *serial)
 
 	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]->number);
 	mutex_unlock(&table_lock);
 }
 
@@ -1041,7 +1035,7 @@ static int usb_serial_probe(struct usb_interface *interface,
 	 */
 	serial->disconnected = 1;
 
-	if (get_free_serial(serial, num_ports, &minor) == NULL) {
+	if (get_free_serial(serial, num_ports, &minor)) {
 		dev_err(ddev, "No more free serial devices\n");
 		goto probe_error;
 	}
@@ -1225,7 +1219,6 @@ static struct usb_driver usb_serial_driver = {
 
 static int __init usb_serial_init(void)
 {
-	int i;
 	int result;
 
 	usb_serial_tty_driver = alloc_tty_driver(SERIAL_TTY_MINORS);
@@ -1233,9 +1226,6 @@ static int __init usb_serial_init(void)
 		return -ENOMEM;
 
 	/* Initialize our global data */
-	for (i = 0; i < SERIAL_TTY_MINORS; ++i)
-		serial_table[i] = NULL;
-
 	result = bus_register(&usb_serial_bus_type);
 	if (result) {
 		pr_err("%s - registering bus driver failed\n", __func__);

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Dave Jones]

On Mon, Jun 03, 2013 at 07:49:59PM -0700, Greg Kroah-Hartman wrote:
 > On Mon, May 27, 2013 at 02:28:51PM +0200, Bjørn Mork wrote:
 > > But, IMHO, a nicer approach would be to make the allocation completely
 > > dynamic, using e.g. the idr subsystem. Static tables are always feel
 > > like straight jackets to me, no matter how big they are :)
 > 
 > You are right, I didn't change the code to use idr (it predates idr by
 > about a decade or so), because I thought we needed the "rage" logic that
 > the usb-serial minor reservation does.

Rage logic sounds like my kinda code.


 > +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)
 > +		return -EEXIST;
 > +	port->number = i;
 > +	mutex_unlock(&table_lock);
 > +	return i;
 > +}

-EEXIST case misses the mutex unlock.

	Dave

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Greg KH]

On Mon, Jun 03, 2013 at 10:59:08PM -0400, Dave Jones wrote:
> On Mon, Jun 03, 2013 at 07:49:59PM -0700, Greg Kroah-Hartman wrote:
>  > On Mon, May 27, 2013 at 02:28:51PM +0200, Bjørn Mork wrote:
>  > > But, IMHO, a nicer approach would be to make the allocation completely
>  > > dynamic, using e.g. the idr subsystem. Static tables are always feel
>  > > like straight jackets to me, no matter how big they are :)
>  > 
>  > You are right, I didn't change the code to use idr (it predates idr by
>  > about a decade or so), because I thought we needed the "rage" logic that
>  > the usb-serial minor reservation does.
> 
> Rage logic sounds like my kinda code.

Late nite typo :)

>  > +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)
>  > +		return -EEXIST;
>  > +	port->number = i;
>  > +	mutex_unlock(&table_lock);
>  > +	return i;
>  > +}
> 
> -EEXIST case misses the mutex unlock.

Thanks, now fixed.

greg k-h

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Johan Hovold]

On Mon, Jun 03, 2013 at 07:49:59PM -0700, Greg KH wrote:
> On Mon, May 27, 2013 at 02:28:51PM +0200, Bjørn Mork wrote:
> > But, IMHO, a nicer approach would be to make the allocation completely
> > dynamic, using e.g. the idr subsystem. Static tables are always feel
> > like straight jackets to me, no matter how big they are :)
> 
> You are right, I didn't change the code to use idr (it predates idr by
> about a decade or so), because I thought we needed the "rage" logic that
> the usb-serial minor reservation does.
> 
> But I'm not so sure anymore, so here's a patch to change to use the idr
> code, and should remove all minor number limitations (well 65k is the
> limit the tty core should be setting I think.)
> 
> Tobias, can you test this patch out?  Note, I only compiled it, did not
> get the chance to actually run it, so it might not work at all.

I'm afraid this won't work in it's current form. Several drivers and
parts of usb-serial core still depend on the minor numbers being
consecutive for multi-port devices. There are also still references to
SERIAL_TTY_NO_MINOR (255) as well as SERIAL_TTY_MINORS that need to be
addressed.

> 
> thanks,
> 
> greg k-h
> 
> Subject: [PATCH] usb: serial: remove minor number limitation of 255
> 
> 
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
>  drivers/usb/serial/usb-serial.c |   86 +++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
> index 4753c00..74b6f08 100644
> --- 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,7 +50,7 @@
>     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);
>  
> @@ -61,59 +62,52 @@ static LIST_HEAD(usb_serial_driver_list);
>  struct usb_serial *usb_serial_get_by_index(unsigned index)
>  {
>  	struct usb_serial *serial;
> +	struct usb_serial_port *port;
>  
>  	mutex_lock(&table_lock);
> -	serial = serial_table[index];
> -
> -	if (serial) {
> -		mutex_lock(&serial->disc_mutex);
> -		if (serial->disconnected) {
> -			mutex_unlock(&serial->disc_mutex);
> -			serial = NULL;
> -		} else {
> -			kref_get(&serial->kref);
> -		}
> -	}
> +	port = idr_find(&serial_minors, index);
>  	mutex_unlock(&table_lock);
> +	if (!port)
> +		return NULL;
> +
> +	serial = port->serial;
> +	kref_get(&serial->kref);
>  	return serial;
>  }

We would still need to handle disconnect, and make sure to return with
the disc_mutex held unless disconnected.

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

Missing mutex unlock (as already Dave noted).

> +		return -EEXIST;
> +	port->number = i;
> +	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 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) {
> -			serial_table[i] = serial;
> -			serial->port[j++]->number = i;
> -		}
> -		mutex_unlock(&table_lock);
> -		return serial;
> +	dev_dbg(&serial->interface->dev, "%s %d\n", __func__, num_ports);
> +
> +	*minor = 0xffffffff;
> +	for (i = 0; i < num_ports; ++i) {
> +		x = get_free_port(serial->port[i]);
> +		if (x < 0)
> +			goto error;
> +		if (*minor == 0xffffffff)
> +			*minor = x;
>  	}
> -	mutex_unlock(&table_lock);
> -	return NULL;
> +	return 0;
> +error:
> +	// FIXME unwind the already allocated minors
> +	return -ENODEV;
>  }

As mentioned above, usb-serial core and several drivers currently depend
on us returning the first minor in a consecutive range. It's mostly used
to determine the per device port index, so storing that index in the port
structure could possibly be sufficient.

>  
>  static void return_serial(struct usb_serial *serial)
> @@ -122,7 +116,7 @@ static void return_serial(struct usb_serial *serial)
>  
>  	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]->number);
>  	mutex_unlock(&table_lock);
>  }
>  
> @@ -1041,7 +1035,7 @@ static int usb_serial_probe(struct usb_interface *interface,
>  	 */
>  	serial->disconnected = 1;
>  
> -	if (get_free_serial(serial, num_ports, &minor) == NULL) {
> +	if (get_free_serial(serial, num_ports, &minor)) {
>  		dev_err(ddev, "No more free serial devices\n");
>  		goto probe_error;
>  	}
> @@ -1225,7 +1219,6 @@ static struct usb_driver usb_serial_driver = {
>  
>  static int __init usb_serial_init(void)
>  {
> -	int i;
>  	int result;
>  
>  	usb_serial_tty_driver = alloc_tty_driver(SERIAL_TTY_MINORS);
> @@ -1233,9 +1226,6 @@ static int __init usb_serial_init(void)
>  		return -ENOMEM;
>  
>  	/* Initialize our global data */

You could remove the above comment as well.

> -	for (i = 0; i < SERIAL_TTY_MINORS; ++i)
> -		serial_table[i] = NULL;
> -
>  	result = bus_register(&usb_serial_bus_type);
>  	if (result) {
>  		pr_err("%s - registering bus driver failed\n", __func__);

Thanks,
Johan

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Greg KH]

On Tue, Jun 04, 2013 at 01:04:01PM +0200, Johan Hovold wrote:
> On Mon, Jun 03, 2013 at 07:49:59PM -0700, Greg KH wrote:
> > On Mon, May 27, 2013 at 02:28:51PM +0200, Bjørn Mork wrote:
> > > But, IMHO, a nicer approach would be to make the allocation completely
> > > dynamic, using e.g. the idr subsystem. Static tables are always feel
> > > like straight jackets to me, no matter how big they are :)
> > 
> > You are right, I didn't change the code to use idr (it predates idr by
> > about a decade or so), because I thought we needed the "rage" logic that
> > the usb-serial minor reservation does.
> > 
> > But I'm not so sure anymore, so here's a patch to change to use the idr
> > code, and should remove all minor number limitations (well 65k is the
> > limit the tty core should be setting I think.)
> > 
> > Tobias, can you test this patch out?  Note, I only compiled it, did not
> > get the chance to actually run it, so it might not work at all.
> 
> I'm afraid this won't work in it's current form. Several drivers and
> parts of usb-serial core still depend on the minor numbers being
> consecutive for multi-port devices.

You are right, let me go fix up the "port->number" assumption first,
which will let this become easier.  That should have been fixed a long
time ago.

> There are also still references to SERIAL_TTY_NO_MINOR (255) as well
> as SERIAL_TTY_MINORS that need to be addressed.

Yeah, I knew it was too good to be true that a simple 45 line patch
would solve this properly :)

> > +	port = idr_find(&serial_minors, index);
> >  	mutex_unlock(&table_lock);
> > +	if (!port)
> > +		return NULL;
> > +
> > +	serial = port->serial;
> > +	kref_get(&serial->kref);
> >  	return serial;
> >  }
> 
> We would still need to handle disconnect, and make sure to return with
> the disc_mutex held unless disconnected.

Alan noticed this as well, I've now fixed that, thanks.

> > +	dev_dbg(&serial->interface->dev, "%s %d\n", __func__, num_ports);
> > +
> > +	*minor = 0xffffffff;
> > +	for (i = 0; i < num_ports; ++i) {
> > +		x = get_free_port(serial->port[i]);
> > +		if (x < 0)
> > +			goto error;
> > +		if (*minor == 0xffffffff)
> > +			*minor = x;
> >  	}
> > -	mutex_unlock(&table_lock);
> > -	return NULL;
> > +	return 0;
> > +error:
> > +	// FIXME unwind the already allocated minors
> > +	return -ENODEV;
> >  }
> 
> As mentioned above, usb-serial core and several drivers currently depend
> on us returning the first minor in a consecutive range. It's mostly used
> to determine the per device port index, so storing that index in the port
> structure could possibly be sufficient.

Yes, I'll go fix that up first, before making these changes, as it's
independant.

> > @@ -1233,9 +1226,6 @@ static int __init usb_serial_init(void)
> >  		return -ENOMEM;
> >  
> >  	/* Initialize our global data */
> 
> You could remove the above comment as well.
> 
> > -	for (i = 0; i < SERIAL_TTY_MINORS; ++i)
> > -		serial_table[i] = NULL;
> > -
> >  	result = bus_register(&usb_serial_bus_type);

I thought about it, but we also register the bus variables, and some
other things here as well, so I left it in.

thanks for the review.

greg k-h

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1945 bytes --]

On Mon, 3 Jun 2013, Greg KH wrote:

> On Mon, May 27, 2013 at 02:28:51PM +0200, Bjørn Mork wrote:
> > But, IMHO, a nicer approach would be to make the allocation completely
> > dynamic, using e.g. the idr subsystem. Static tables are always feel
> > like straight jackets to me, no matter how big they are :)
> 
> You are right, I didn't change the code to use idr (it predates idr by
> about a decade or so), because I thought we needed the "rage" logic that
> the usb-serial minor reservation does.
> 
> But I'm not so sure anymore, so here's a patch to change to use the idr
> code, and should remove all minor number limitations (well 65k is the
> limit the tty core should be setting I think.)
> 
> Tobias, can you test this patch out?  Note, I only compiled it, did not
> get the chance to actually run it, so it might not work at all.
> 
> thanks,
> 
> greg k-h


> @@ -61,59 +62,52 @@ static LIST_HEAD(usb_serial_driver_list);
>  struct usb_serial *usb_serial_get_by_index(unsigned index)
>  {
>  	struct usb_serial *serial;
> +	struct usb_serial_port *port;
>  
>  	mutex_lock(&table_lock);
> -	serial = serial_table[index];
> -
> -	if (serial) {
> -		mutex_lock(&serial->disc_mutex);
> -		if (serial->disconnected) {
> -			mutex_unlock(&serial->disc_mutex);
> -			serial = NULL;
> -		} else {
> -			kref_get(&serial->kref);
> -		}
> -	}
> +	port = idr_find(&serial_minors, index);
>  	mutex_unlock(&table_lock);
> +	if (!port)
> +		return NULL;
> +
> +	serial = port->serial;
> +	kref_get(&serial->kref);
>  	return serial;
>  }

The test for serial->disconnected got lost.  And the locking isn't 
right; the routine is documented to return with serial->disc_mutex held 
(in the case where the device hasn't been disconnected).

Also, the kref_get() needs to occur within the scope of the table_lock.

I didn't check the rest of the patch for similar errors.  Finding three 
in the first function seemed like enough.  :-)

Alan Stern

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Greg KH]

On Tue, Jun 04, 2013 at 10:13:47AM -0400, Alan Stern wrote:
> On Mon, 3 Jun 2013, Greg KH wrote:
> 
> > On Mon, May 27, 2013 at 02:28:51PM +0200, Bj?rn Mork wrote:
> > > But, IMHO, a nicer approach would be to make the allocation completely
> > > dynamic, using e.g. the idr subsystem. Static tables are always feel
> > > like straight jackets to me, no matter how big they are :)
> > 
> > You are right, I didn't change the code to use idr (it predates idr by
> > about a decade or so), because I thought we needed the "rage" logic that
> > the usb-serial minor reservation does.
> > 
> > But I'm not so sure anymore, so here's a patch to change to use the idr
> > code, and should remove all minor number limitations (well 65k is the
> > limit the tty core should be setting I think.)
> > 
> > Tobias, can you test this patch out?  Note, I only compiled it, did not
> > get the chance to actually run it, so it might not work at all.
> > 
> > thanks,
> > 
> > greg k-h
> 
> 
> > @@ -61,59 +62,52 @@ static LIST_HEAD(usb_serial_driver_list);
> >  struct usb_serial *usb_serial_get_by_index(unsigned index)
> >  {
> >  	struct usb_serial *serial;
> > +	struct usb_serial_port *port;
> >  
> >  	mutex_lock(&table_lock);
> > -	serial = serial_table[index];
> > -
> > -	if (serial) {
> > -		mutex_lock(&serial->disc_mutex);
> > -		if (serial->disconnected) {
> > -			mutex_unlock(&serial->disc_mutex);
> > -			serial = NULL;
> > -		} else {
> > -			kref_get(&serial->kref);
> > -		}
> > -	}
> > +	port = idr_find(&serial_minors, index);
> >  	mutex_unlock(&table_lock);
> > +	if (!port)
> > +		return NULL;
> > +
> > +	serial = port->serial;
> > +	kref_get(&serial->kref);
> >  	return serial;
> >  }
> 
> The test for serial->disconnected got lost.  And the locking isn't 
> right; the routine is documented to return with serial->disc_mutex held 
> (in the case where the device hasn't been disconnected).
> 
> Also, the kref_get() needs to occur within the scope of the table_lock.

Thanks, for some reason I ignored this when converting the code, that's
what I get for not even testing...

> I didn't check the rest of the patch for similar errors.  Finding three 
> in the first function seemed like enough.  :-)

Fair enough, I've now fixed this up, and will see if it runs properly.

thanks,

greg k-h

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Tobias Winter]

On 04.06.2013 04:49, Greg KH wrote:
> Tobias, can you test this patch out?  Note, I only compiled it, did not
> get the chance to actually run it, so it might not work at all.

Sure, I'll gladly give it a try. Seeing the comments on the code, I'm
just wondering if there might be a more recent version to run?

> But I'm not so sure anymore, so here's a patch to change to use the
> idr code, and should remove all minor number limitations (well 65k is
> the limit the tty core should be setting I think.)

That's a tough one to break :)

Tobias

Re: [RFC] raise the maximum number of usb-serial devices to 512

[Greg KH]

On Tue, Jun 04, 2013 at 07:27:20PM +0200, Tobias Winter wrote:
> On 04.06.2013 04:49, Greg KH wrote:
> > Tobias, can you test this patch out?  Note, I only compiled it, did not
> > get the chance to actually run it, so it might not work at all.
> 
> Sure, I'll gladly give it a try. Seeing the comments on the code, I'm
> just wondering if there might be a more recent version to run?

Give me a day, it's going to take some more reworks to get this all to
work properly, I'll send a new patch series then.

thanks,

greg k-h