Re: [PATCH 2/3] USB: serial: make minor allocation dynamic
On Wed, Jun 05, 2013 at 10:54:55AM -0700, Greg KH wrote:
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.
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
[...]
-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 = 0x;
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 == 0x)
+ *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-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 2/3] USB: serial: make minor allocation dynamic
On Thu, Jun 06, 2013 at 02:17:18PM +0200, Johan Hovold wrote:
On Wed, Jun 05, 2013 at 10:54:55AM -0700, Greg KH wrote:
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.
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
[...]
-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 = 0x;
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 == 0x)
+ *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.
Good point. In looking at this further, I really need to drop the
usb_serial structure's minor field completly, as it doesn't make sense
anymore.
I'll go rework all of that and post a v2 of this series, thanks.
+ 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?
Good catch, now fixed.
}
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 for the review, much appreciated.
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 2/3] USB: serial: make minor allocation dynamic
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.
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/usb/serial/usb-serial.c | 97
include/linux/usb/serial.h |4 -
2 files changed, 51 insertions(+), 50 deletions(-)
--- 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,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);
@@ -60,61 +61,65 @@ static LIST_HEAD(usb_serial_driver_list)
*/
struct usb_serial *usb_serial_get_by_index(unsigned index)
{
- 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, index);
+ 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;
+ } else {
+ kref_get(serial-kref);
}
+exit:
mutex_unlock(table_lock);
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)
+ 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 = 0x;
+ for (i = 0; i num_ports; ++i) {
+ x = get_free_port(serial-port[i]);
+ if (x 0)
+ goto error;
+ if (*minor == 0x)
+ *minor = x;
+ 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;
}
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);
