[PATCH 00/11] hso: fix some problems with reset/disconnect path

[PATCH 00/11] hso: fix some problems with reset/disconnect path

[Olivier Sobrie]

These patches attempt to fix some problems I observed when the hso
device is disconnected or when a usb reset is queued by the hso driver
Several patches of this serie are fixing crashes or memleaks in hso.
The last patch of this serie fix a race condition occurring when multiple
usb resets are queued on an usb interface.
This serie of patches is based on v3.18.

Olivier Sobrie (11):
  hso: remove useless header file timer.h
  hso: fix crash when device disappears while serial port is open
  hso: fix memory leak when device disconnects
  hso: fix memory leak in hso_create_rfkill()
  hso: fix small indentation error
  hso: rename hso_dev into serial in hso_free_interface()
  hso: replace reset_device work by usb_queue_reset_device()
  hso: move tty_unregister outside hso_serial_common_free()
  hso: update serial_table in usb disconnect method
  hso: add missing cancel_work_sync in disconnect()
  usb: core: fix a race with usb_queue_reset_device()

 drivers/net/usb/hso.c      | 88 ++++++++++++++++++++--------------------------
 drivers/usb/core/driver.c  |  4 ++-
 drivers/usb/core/hub.c     |  4 +--
 drivers/usb/core/message.c |  4 +--
 include/linux/usb.h        |  2 +-
 5 files changed, 46 insertions(+), 56 deletions(-)

-- 
2.2.0

[PATCH 01/11] hso: remove useless header file timer.h

[Olivier Sobrie]

No timer related function is used in this driver.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index babda7d..cb0bcc1 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -58,7 +58,6 @@
 #include <linux/module.h>
 #include <linux/ethtool.h>
 #include <linux/usb.h>
-#include <linux/timer.h>
 #include <linux/tty.h>
 #include <linux/tty_driver.h>
 #include <linux/tty_flip.h>
-- 
2.2.0

[PATCH 02/11] hso: fix crash when device disappears while serial port is open

[Olivier Sobrie]

When the device disappear, the function hso_disconnect() is called to
perform cleanup. In the cleanup function, hso_free_interface() calls
tty_port_tty_hangup() in view of scheduling a work to hang up the tty if
needed. If the port was not open then hso_serial_ref_free() is called
directly to cleanup everything. Otherwise, hso_serial_ref_free() is called
when the last fd associated to the port is closed.

For each open port, tty_release() will call the close method,
hso_serial_close(), which drops the last kref and call
hso_serial_ref_free() which unregisters, destroys the tty port
and finally frees the structure in which the tty_port structure
is included. Later, in tty_release(), more precisely when release_tty()
is called, the tty_port previously freed is accessed to cancel
the tty buf workqueue and it leads to a crash.

In view of avoiding this crash, we add a cleanup method that is called
at the end of the hangup process and we drop the last kref in this
function when all the ports have been closed, when tty_port is no
more needed and when it is safe to free the structure containing the
tty_port structure.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index cb0bcc1..3a6c630 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -1270,7 +1270,6 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		goto err_out;
 
 	D1("Opening %d", serial->minor);
-	kref_get(&serial->parent->ref);
 
 	/* setup */
 	tty->driver_data = serial;
@@ -1289,7 +1288,8 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		if (result) {
 			hso_stop_serial_device(serial->parent);
 			serial->port.count--;
-			kref_put(&serial->parent->ref, hso_serial_ref_free);
+		} else {
+			kref_get(&serial->parent->ref);
 		}
 	} else {
 		D1("Port was already open");
@@ -1339,8 +1339,6 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 		usb_autopm_put_interface(serial->parent->interface);
 
 	mutex_unlock(&serial->parent->mutex);
-
-	kref_put(&serial->parent->ref, hso_serial_ref_free);
 }
 
 /* close the requested serial port */
@@ -1391,6 +1389,16 @@ static int hso_serial_write_room(struct tty_struct *tty)
 	return room;
 }
 
+static void hso_serial_cleanup(struct tty_struct *tty)
+{
+	struct hso_serial *serial = tty->driver_data;
+
+	if (!serial)
+		return;
+
+	kref_put(&serial->parent->ref, hso_serial_ref_free);
+}
+
 /* setup the term */
 static void hso_serial_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
@@ -3215,6 +3223,7 @@ static const struct tty_operations hso_serial_ops = {
 	.close = hso_serial_close,
 	.write = hso_serial_write,
 	.write_room = hso_serial_write_room,
+	.cleanup = hso_serial_cleanup,
 	.ioctl = hso_serial_ioctl,
 	.set_termios = hso_serial_set_termios,
 	.chars_in_buffer = hso_serial_chars_in_buffer,
-- 
2.2.0

[PATCH 03/11] hso: fix memory leak when device disconnects

[Olivier Sobrie]

In the disconnect path, tx_buffer should freed like tx_data to avoid
a memory leak when the device disconnects.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 3a6c630..470ef9e 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2253,6 +2253,7 @@ static void hso_serial_common_free(struct hso_serial *serial)
 
 	/* unlink and free TX URB */
 	usb_free_urb(serial->tx_urb);
+	kfree(serial->tx_buffer);
 	kfree(serial->tx_data);
 	tty_port_destroy(&serial->port);
 }
-- 
2.2.0

[PATCH 04/11] hso: fix memory leak in hso_create_rfkill()

[Olivier Sobrie]

When the rfkill interface was created, a buffer containing the name
of the rfkill node was allocated. This buffer was never freed when the
device disappears.

To fix the problem, we put the name given to rfkill_alloc() in
the hso_net structure.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 470ef9e..a49ac2e 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -153,6 +153,7 @@ struct hso_net {
 	struct hso_device *parent;
 	struct net_device *net;
 	struct rfkill *rfkill;
+	char name[8];
 
 	struct usb_endpoint_descriptor *in_endp;
 	struct usb_endpoint_descriptor *out_endp;
@@ -2467,27 +2468,20 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
 {
 	struct hso_net *hso_net = dev2net(hso_dev);
 	struct device *dev = &hso_net->net->dev;
-	char *rfkn;
 
-	rfkn = kzalloc(20, GFP_KERNEL);
-	if (!rfkn)
-		dev_err(dev, "%s - Out of memory\n", __func__);
-
-	snprintf(rfkn, 20, "hso-%d",
+	snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
 		 interface->altsetting->desc.bInterfaceNumber);
 
-	hso_net->rfkill = rfkill_alloc(rfkn,
+	hso_net->rfkill = rfkill_alloc(hso_net->name,
 				       &interface_to_usbdev(interface)->dev,
 				       RFKILL_TYPE_WWAN,
 				       &hso_rfkill_ops, hso_dev);
 	if (!hso_net->rfkill) {
 		dev_err(dev, "%s - Out of memory\n", __func__);
-		kfree(rfkn);
 		return;
 	}
 	if (rfkill_register(hso_net->rfkill) < 0) {
 		rfkill_destroy(hso_net->rfkill);
-		kfree(rfkn);
 		hso_net->rfkill = NULL;
 		dev_err(dev, "%s - Failed to register rfkill\n", __func__);
 		return;
-- 
2.2.0

[PATCH 05/11] hso: fix small indentation error

[Olivier Sobrie]

Simply remove the useless extra tab.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index a49ac2e..2f2343d 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2206,8 +2206,8 @@ static int hso_stop_serial_device(struct hso_device *hso_dev)
 
 	for (i = 0; i < serial->num_rx_urbs; i++) {
 		if (serial->rx_urb[i]) {
-				usb_kill_urb(serial->rx_urb[i]);
-				serial->rx_urb_filled[i] = 0;
+			usb_kill_urb(serial->rx_urb[i]);
+			serial->rx_urb_filled[i] = 0;
 		}
 	}
 	serial->curr_rx_urb_idx = 0;
-- 
2.2.0

[PATCH 06/11] hso: rename hso_dev into serial in hso_free_interface()

[Olivier Sobrie]

In other functions of the driver, variables of type "struct hso_serial"
are denoted by "serial" and variables of type "struct hso_device" are
denoted by "hso_dev". This patch makes the hso_free_interface()
consistent with these notations.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 2f2343d..484e423 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -3115,17 +3115,17 @@ static void hso_serial_ref_free(struct kref *ref)
 
 static void hso_free_interface(struct usb_interface *interface)
 {
-	struct hso_serial *hso_dev;
+	struct hso_serial *serial;
 	int i;
 
 	for (i = 0; i < HSO_SERIAL_TTY_MINORS; i++) {
 		if (serial_table[i] &&
 		    (serial_table[i]->interface == interface)) {
-			hso_dev = dev2ser(serial_table[i]);
-			tty_port_tty_hangup(&hso_dev->port, false);
-			mutex_lock(&hso_dev->parent->mutex);
-			hso_dev->parent->usb_gone = 1;
-			mutex_unlock(&hso_dev->parent->mutex);
+			serial = dev2ser(serial_table[i]);
+			tty_port_tty_hangup(&serial->port, false);
+			mutex_lock(&serial->parent->mutex);
+			serial->parent->usb_gone = 1;
+			mutex_unlock(&serial->parent->mutex);
 			kref_put(&serial_table[i]->ref, hso_serial_ref_free);
 		}
 	}
-- 
2.2.0

[PATCH 07/11] hso: replace reset_device work by usb_queue_reset_device()

[Olivier Sobrie]

There is no need for a dedicated reset work in the hso driver since
there is already a reset work foreseen in usb_interface that does
the same.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 484e423..55074da 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -274,7 +274,6 @@ struct hso_device {
 	u8 usb_gone;
 	struct work_struct async_get_intf;
 	struct work_struct async_put_intf;
-	struct work_struct reset_device;
 
 	struct usb_device *usb;
 	struct usb_interface *interface;
@@ -340,7 +339,6 @@ static void async_put_intf(struct work_struct *data);
 static int hso_put_activity(struct hso_device *hso_dev);
 static int hso_get_activity(struct hso_device *hso_dev);
 static void tiocmget_intr_callback(struct urb *urb);
-static void reset_device(struct work_struct *data);
 /*****************************************************************************/
 /* Helping functions                                                         */
 /*****************************************************************************/
@@ -696,7 +694,7 @@ static void handle_usb_error(int status, const char *function,
 	case -ETIMEDOUT:
 		explanation = "protocol error";
 		if (hso_dev)
-			schedule_work(&hso_dev->reset_device);
+			usb_queue_reset_device(hso_dev->interface);
 		break;
 	default:
 		explanation = "unknown status";
@@ -2347,7 +2345,6 @@ static struct hso_device *hso_create_device(struct usb_interface *intf,
 
 	INIT_WORK(&hso_dev->async_get_intf, async_get_intf);
 	INIT_WORK(&hso_dev->async_put_intf, async_put_intf);
-	INIT_WORK(&hso_dev->reset_device, reset_device);
 
 	return hso_dev;
 }
@@ -3086,26 +3083,6 @@ out:
 	return result;
 }
 
-static void reset_device(struct work_struct *data)
-{
-	struct hso_device *hso_dev =
-	    container_of(data, struct hso_device, reset_device);
-	struct usb_device *usb = hso_dev->usb;
-	int result;
-
-	if (hso_dev->usb_gone) {
-		D1("No reset during disconnect\n");
-	} else {
-		result = usb_lock_device_for_reset(usb, hso_dev->interface);
-		if (result < 0)
-			D1("unable to lock device for reset: %d\n", result);
-		else {
-			usb_reset_device(usb);
-			usb_unlock_device(usb);
-		}
-	}
-}
-
 static void hso_serial_ref_free(struct kref *ref)
 {
 	struct hso_device *hso_dev = container_of(ref, struct hso_device, ref);
-- 
2.2.0

[PATCH 08/11] hso: move tty_unregister outside hso_serial_common_free()

[Olivier Sobrie]

The function hso_serial_common_free() is called either by the cleanup
method of the tty or by the usb disconnect method.
In the former case, the usb_disconnect() has been already called
and the sysfs group associated to the device has been removed.
By calling tty_unregister directly from the usb_disconnect() method,
we avoid a warning due to the removal of the sysfs group of the usb
device.

Example of warning:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 778 at fs/sysfs/group.c:225 sysfs_remove_group+0x50/0x94()
sysfs group c0645a88 not found for kobject 'ttyHS5'
Modules linked in:
CPU: 0 PID: 778 Comm: kworker/0:3 Tainted: G        W      3.18.0+ #105
Workqueue: events release_one_tty
[<c000dfe4>] (unwind_backtrace) from [<c000c014>] (show_stack+0x14/0x1c)
[<c000c014>] (show_stack) from [<c0016bac>] (warn_slowpath_common+0x5c/0x7c)
[<c0016bac>] (warn_slowpath_common) from [<c0016c60>] (warn_slowpath_fmt+0x30/0x40)
[<c0016c60>] (warn_slowpath_fmt) from [<c00ddd14>] (sysfs_remove_group+0x50/0x94)
[<c00ddd14>] (sysfs_remove_group) from [<c0221e44>] (device_del+0x30/0x190)
[<c0221e44>] (device_del) from [<c0221fb0>] (device_unregister+0xc/0x18)
[<c0221fb0>] (device_unregister) from [<c0221fec>] (device_destroy+0x30/0x3c)
[<c0221fec>] (device_destroy) from [<c01fe1dc>] (tty_unregister_device+0x2c/0x5c)
[<c01fe1dc>] (tty_unregister_device) from [<c029a428>] (hso_serial_common_free+0x2c/0x88)
[<c029a428>] (hso_serial_common_free) from [<c029a4c0>] (hso_serial_ref_free+0x3c/0xb8)
[<c029a4c0>] (hso_serial_ref_free) from [<c01ff430>] (release_one_tty+0x30/0x84)
[<c01ff430>] (release_one_tty) from [<c00271d4>] (process_one_work+0x21c/0x3c8)
[<c00271d4>] (process_one_work) from [<c0027758>] (worker_thread+0x3d8/0x560)
[<c0027758>] (worker_thread) from [<c002be4c>] (kthread+0xc0/0xcc)
[<c002be4c>] (kthread) from [<c0009630>] (ret_from_fork+0x14/0x24)
---[ end trace cb88537fdc8fa208 ]---

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 55074da..5b157ad 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2234,14 +2234,17 @@ static int hso_stop_serial_device(struct hso_device *hso_dev)
 	return 0;
 }
 
-static void hso_serial_common_free(struct hso_serial *serial)
+static void hso_serial_tty_unregister(struct hso_serial *serial)
 {
-	int i;
-
 	if (serial->parent->dev)
 		device_remove_file(serial->parent->dev, &dev_attr_hsotype);
 
 	tty_unregister_device(tty_drv, serial->minor);
+}
+
+static void hso_serial_common_free(struct hso_serial *serial)
+{
+	int i;
 
 	for (i = 0; i < serial->num_rx_urbs; i++) {
 		/* unlink and free RX URB */
@@ -2323,6 +2326,7 @@ static int hso_serial_common_create(struct hso_serial *serial, int num_urbs,
 
 	return 0;
 exit:
+	hso_serial_tty_unregister(serial);
 	hso_serial_common_free(serial);
 	return -1;
 }
@@ -2683,6 +2687,7 @@ static struct hso_device *hso_create_bulk_serial_device(
 	return hso_dev;
 
 exit2:
+	hso_serial_tty_unregister(serial);
 	hso_serial_common_free(serial);
 exit:
 	hso_free_tiomget(serial);
@@ -3103,6 +3108,7 @@ static void hso_free_interface(struct usb_interface *interface)
 			mutex_lock(&serial->parent->mutex);
 			serial->parent->usb_gone = 1;
 			mutex_unlock(&serial->parent->mutex);
+			hso_serial_tty_unregister(serial);
 			kref_put(&serial_table[i]->ref, hso_serial_ref_free);
 		}
 	}
-- 
2.2.0

[PATCH 09/11] hso: update serial_table in usb disconnect method

[Olivier Sobrie]

The serial_table is used to map the minor number of the usb serial device
to its associated context. The table is updated in the probe method and
in hso_serial_ref_free() which is called either from the tty cleanup
method or from the usb disconnect method.
This patch ensures that the serial_table is updated in the disconnect
method and no more from the cleanup method to avoid the following
potential race condition.

 - hso_disconnect() is called for usb interface "x". Because the serial
   port was open and because the cleanup method of the tty_port hasn't
   been called yet, hso_serial_ref_free() is not run.
 - hso_probe() is called and fails for a new hso serial usb interface
   "y". The function hso_free_interface() is called and iterates
   over the element of serial_table to find the device associated to
   the usb interface context.
   If the usb interface context of usb interface "y" has been created
   at the same place as for usb interface "x", then the cleanup
   functions are called for usb interfaces "x" and "y" and
   hso_serial_ref_free() is called for both interfaces.
 - release_tty() is called for serial port linked to usb interface "x"
   and possibly crash because the tty_port structure contained in the
   hso_device structure has been freed.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 5b157ad..c916ab5 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2597,7 +2597,6 @@ static void hso_free_serial_device(struct hso_device *hso_dev)
 
 	if (!serial)
 		return;
-	set_serial_by_index(serial->minor, NULL);
 
 	hso_serial_common_free(serial);
 
@@ -3110,6 +3109,7 @@ static void hso_free_interface(struct usb_interface *interface)
 			mutex_unlock(&serial->parent->mutex);
 			hso_serial_tty_unregister(serial);
 			kref_put(&serial_table[i]->ref, hso_serial_ref_free);
+			set_serial_by_index(i, NULL);
 		}
 	}
 
-- 
2.2.0

[PATCH 10/11] hso: add missing cancel_work_sync in disconnect()

[Olivier Sobrie]

For hso serial devices, two cancel_work_sync were missing in the
disconnect method.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index c916ab5..c14fc80 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -3107,6 +3107,8 @@ static void hso_free_interface(struct usb_interface *interface)
 			mutex_lock(&serial->parent->mutex);
 			serial->parent->usb_gone = 1;
 			mutex_unlock(&serial->parent->mutex);
+			cancel_work_sync(&serial_table[i]->async_put_intf);
+			cancel_work_sync(&serial_table[i]->async_get_intf);
 			hso_serial_tty_unregister(serial);
 			kref_put(&serial_table[i]->ref, hso_serial_ref_free);
 			set_serial_by_index(i, NULL);
-- 
2.2.0

[PATCH 11/11] usb: core: fix a race with usb_queue_reset_device()

[Olivier Sobrie]

When usb_queue_reset() is called it schedules a work in view of
resetting the usb interface. When the reset work is running, it
can be scheduled again (e.g. by the usb disconnect method of
the driver).

Consider that the reset work is queued again while the reset work
is running and that this work leads to a forced unbinding of the
usb interface (e.g. because a driver is bound to the interface
and has no pre/post_reset methods - see usb_reset_device()).
In such condition, usb_unbind_interface() gets called and this
function calls usb_cancel_queued_reset() which does nothing
because the flag "reset_running" is set to 1. The second reset
work that has been scheduled is therefore not cancelled.
Later, the usb_reset_device() tries to rebind the interface.
If it fails, then the usb interface context which contain the
reset work struct is freed and it most likely crash when the
second reset work tries to be run.

The following flow shows the problem:
* usb_queue_reset_device()
* __usb_queue_reset_device() <- If the reset work is queued after here, then
    reset_running = 1           it will never be cancelled.
    usb_reset_device()
      usb_forced_unbind_intf()
        usb_driver_release_interface()
          usb_unbind_interface()
            driver->disconnect()
              usb_queue_reset_device() <- second reset
            usb_cancel_queued_reset() <- does nothing because
                                         the flag reset_running
                                         is set
      usb_unbind_and_rebind_marked_interfaces()
        usb_rebind_intf()
          device_attach()
            driver->probe() <- fails (no more drivers hold a reference to
				      the usb interface)
    reset_running = 0
* hub_event()
    usb_disconnect()
      usb_disable_device()
        kobject_release()
          device_release()
            usb_release_interface()
              kfree(intf) <- usb interface context is released
                             while we still have a pending reset
                             work that should be run

To avoid this problem, we use a delayed work so that if the reset
work is currently run, we can avoid further call to
__usb_queue_reset_device() work by using cancel_delayed_work().
Unfortunately it increases the size of the usb_interface structure...

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/usb/core/driver.c  | 4 +++-
 drivers/usb/core/hub.c     | 4 ++--
 drivers/usb/core/message.c | 4 ++--
 include/linux/usb.h        | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 9bffd26..c93fbbbb 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -287,7 +287,9 @@ static int usb_unbind_device(struct device *dev)
 static void usb_cancel_queued_reset(struct usb_interface *iface)
 {
 	if (iface->reset_running == 0)
-		cancel_work_sync(&iface->reset_ws);
+		cancel_delayed_work_sync(&iface->reset_ws);
+	else
+		cancel_delayed_work(&iface->reset_ws);
 }
 
 /* called from driver core with dev locked */
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b649fef..52fba97 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5604,13 +5604,13 @@ EXPORT_SYMBOL_GPL(usb_reset_device);
  * NOTE: We don't do any reference count tracking because it is not
  *     needed. The lifecycle of the work_struct is tied to the
  *     usb_interface. Before destroying the interface we cancel the
- *     work_struct, so the fact that work_struct is queued and or
+ *     delayed_work, so the fact that delayed_work is queued and or
  *     running means the interface (and thus, the device) exist and
  *     are referenced.
  */
 void usb_queue_reset_device(struct usb_interface *iface)
 {
-	schedule_work(&iface->reset_ws);
+	schedule_delayed_work(&iface->reset_ws, 0);
 }
 EXPORT_SYMBOL_GPL(usb_queue_reset_device);
 
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index f7b7713..d9f3f68 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1650,7 +1650,7 @@ static void __usb_queue_reset_device(struct work_struct *ws)
 {
 	int rc;
 	struct usb_interface *iface =
-		container_of(ws, struct usb_interface, reset_ws);
+		container_of(ws, struct usb_interface, reset_ws.work);
 	struct usb_device *udev = interface_to_usbdev(iface);
 
 	rc = usb_lock_device_for_reset(udev, iface);
@@ -1847,7 +1847,7 @@ free_interfaces:
 		intf->dev.type = &usb_if_device_type;
 		intf->dev.groups = usb_interface_groups;
 		intf->dev.dma_mask = dev->dev.dma_mask;
-		INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
+		INIT_DELAYED_WORK(&intf->reset_ws, __usb_queue_reset_device);
 		intf->minor = -1;
 		device_initialize(&intf->dev);
 		pm_runtime_no_callbacks(&intf->dev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 447a7e2..ffb3da1 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -187,7 +187,7 @@ struct usb_interface {
 	struct device dev;		/* interface specific device info */
 	struct device *usb_dev;
 	atomic_t pm_usage_cnt;		/* usage counter for autosuspend */
-	struct work_struct reset_ws;	/* for resets in atomic context */
+	struct delayed_work reset_ws;	/* for resets in atomic context */
 };
 #define	to_usb_interface(d) container_of(d, struct usb_interface, dev)
 
-- 
2.2.0

Re: [PATCH 11/11] usb: core: fix a race with usb_queue_reset_device()

[Oliver Neukum]

On Tue, 2015-01-20 at 13:29 +0100, Olivier Sobrie wrote:
> When usb_queue_reset() is called it schedules a work in view of
> resetting the usb interface. When the reset work is running, it
> can be scheduled again (e.g. by the usb disconnect method of
> the driver).
> 
> Consider that the reset work is queued again while the reset work
> is running and that this work leads to a forced unbinding of the
> usb interface (e.g. because a driver is bound to the interface
> and has no pre/post_reset methods - see usb_reset_device()).
> In such condition, usb_unbind_interface() gets called and this
> function calls usb_cancel_queued_reset() which does nothing
> because the flag "reset_running" is set to 1. The second reset
> work that has been scheduled is therefore not cancelled.
> Later, the usb_reset_device() tries to rebind the interface.
> If it fails, then the usb interface context which contain the
> reset work struct is freed and it most likely crash when the
> second reset work tries to be run.
> 
> The following flow shows the problem:
> * usb_queue_reset_device()
> * __usb_queue_reset_device() <- If the reset work is queued after here, then
>     reset_running = 1           it will never be cancelled.
>     usb_reset_device()
>       usb_forced_unbind_intf()
>         usb_driver_release_interface()
>           usb_unbind_interface()
>             driver->disconnect()
>               usb_queue_reset_device() <- second reset

That is the sledgehammer approach. Wouldn't it be better to guarantee
that usb_queue_reset_device() be a nop when reset_running==1 ?

>             usb_cancel_queued_reset() <- does nothing because
>                                          the flag reset_running
>                                          is set
>       usb_unbind_and_rebind_marked_interfaces()
>         usb_rebind_intf()
>           device_attach()
>             driver->probe() <- fails (no more drivers hold a reference to
> 				      the usb interface)
>     reset_running = 0
> * hub_event()
>     usb_disconnect()
>       usb_disable_device()
>         kobject_release()
>           device_release()
>             usb_release_interface()
>               kfree(intf) <- usb interface context is released
>                              while we still have a pending reset
>                              work that should be run
> 
> To avoid this problem, we use a delayed work so that if the reset
> work is currently run, we can avoid further call to
> __usb_queue_reset_device() work by using cancel_delayed_work().
> Unfortunately it increases the size of the usb_interface structure...

	Regards
		Oliver

-- 
Oliver Neukum <oneukum@suse.de>

Re: [PATCH 11/11] usb: core: fix a race with usb_queue_reset_device()

[Olivier Sobrie]

Hi Oliver,

On Tue, Jan 20, 2015 at 02:48:37PM +0100, Oliver Neukum wrote:
> On Tue, 2015-01-20 at 13:29 +0100, Olivier Sobrie wrote:
> > When usb_queue_reset() is called it schedules a work in view of
> > resetting the usb interface. When the reset work is running, it
> > can be scheduled again (e.g. by the usb disconnect method of
> > the driver).
> > 
> > Consider that the reset work is queued again while the reset work
> > is running and that this work leads to a forced unbinding of the
> > usb interface (e.g. because a driver is bound to the interface
> > and has no pre/post_reset methods - see usb_reset_device()).
> > In such condition, usb_unbind_interface() gets called and this
> > function calls usb_cancel_queued_reset() which does nothing
> > because the flag "reset_running" is set to 1. The second reset
> > work that has been scheduled is therefore not cancelled.
> > Later, the usb_reset_device() tries to rebind the interface.
> > If it fails, then the usb interface context which contain the
> > reset work struct is freed and it most likely crash when the
> > second reset work tries to be run.
> > 
> > The following flow shows the problem:
> > * usb_queue_reset_device()
> > * __usb_queue_reset_device() <- If the reset work is queued after here, then
> >     reset_running = 1           it will never be cancelled.
> >     usb_reset_device()
> >       usb_forced_unbind_intf()
> >         usb_driver_release_interface()
> >           usb_unbind_interface()
> >             driver->disconnect()
> >               usb_queue_reset_device() <- second reset
> 
> That is the sledgehammer approach. Wouldn't it be better to guarantee
> that usb_queue_reset_device() be a nop when reset_running==1 ?

If I'm right, we have to prevent that usb_queue_reset_device() shedules
the work a second time before the variable reset_running is set.
An other task can requeue a reset while the work __usb_queue_reset_device()
is busy but when the flag reset_running hasn't been set yet.

I see different other approaches to solve the problem:

 * Setting a flag in the usb_queue_reset_device() when a reset has been
   scheduled and resetting this flag when the reset is done. This implies
   a locking mechanism around the flag.

 * Avoid that the hso driver queues multiple resets by using a flag. It
   also requires locking. It comes more or less to the same solution
   as the previous one but the patch is done in the hso driver.

 * using get_device() and put_device() to avoid that the usb interface
   structure get freed before the second reset is run.
   I mean:
	void usb_queue_reset_device(struct usb_interface *iface)
	{
		get_device()
		if (!schedule_work(&iface->reset_ws))
			put_device()
	}

	static void __usb_queue_reset_device(struct work_struct *ws)
	{
		...
		put_device()
	}

   But this solution does not avoid the second reset...

If you have other better ideas, let me know.
Correct me if I'm wrong.

Thank you,

Olivier

> 
> >             usb_cancel_queued_reset() <- does nothing because
> >                                          the flag reset_running
> >                                          is set
> >       usb_unbind_and_rebind_marked_interfaces()
> >         usb_rebind_intf()
> >           device_attach()
> >             driver->probe() <- fails (no more drivers hold a reference to
> > 				      the usb interface)
> >     reset_running = 0
> > * hub_event()
> >     usb_disconnect()
> >       usb_disable_device()
> >         kobject_release()
> >           device_release()
> >             usb_release_interface()
> >               kfree(intf) <- usb interface context is released
> >                              while we still have a pending reset
> >                              work that should be run
> > 
> > To avoid this problem, we use a delayed work so that if the reset
> > work is currently run, we can avoid further call to
> > __usb_queue_reset_device() work by using cancel_delayed_work().
> > Unfortunately it increases the size of the usb_interface structure...
> 
> 	Regards
> 		Oliver
> 
> -- 
> Oliver Neukum <oneukum@suse.de>
> 

-- 
Olivier

Re: [PATCH 11/11] usb: core: fix a race with usb_queue_reset_device()

[Alan Stern]

On Tue, 20 Jan 2015, Olivier Sobrie wrote:

> When usb_queue_reset() is called it schedules a work in view of
> resetting the usb interface. When the reset work is running, it
> can be scheduled again (e.g. by the usb disconnect method of
> the driver).
> 
> Consider that the reset work is queued again while the reset work
> is running and that this work leads to a forced unbinding of the
> usb interface (e.g. because a driver is bound to the interface
> and has no pre/post_reset methods - see usb_reset_device()).
> In such condition, usb_unbind_interface() gets called and this
> function calls usb_cancel_queued_reset() which does nothing
> because the flag "reset_running" is set to 1. The second reset
> work that has been scheduled is therefore not cancelled.
> Later, the usb_reset_device() tries to rebind the interface.
> If it fails, then the usb interface context which contain the
> reset work struct is freed and it most likely crash when the
> second reset work tries to be run.

There was an earlier patch posted for testing (no results yet)  
affecting this same region of code:

	http://marc.info/?l=linux-usb&m=142064533924019&w=2

It should fix the problem described here, because (among other things) 
it adds usb_get/put_intf calls to the delayed-reset routines.

Alan Stern

Re: [PATCH 11/11] usb: core: fix a race with usb_queue_reset_device()

[Olivier Sobrie]

On Tue, Jan 20, 2015 at 10:26:30AM -0500, Alan Stern wrote:
> On Tue, 20 Jan 2015, Olivier Sobrie wrote:
> 
> > When usb_queue_reset() is called it schedules a work in view of
> > resetting the usb interface. When the reset work is running, it
> > can be scheduled again (e.g. by the usb disconnect method of
> > the driver).
> > 
> > Consider that the reset work is queued again while the reset work
> > is running and that this work leads to a forced unbinding of the
> > usb interface (e.g. because a driver is bound to the interface
> > and has no pre/post_reset methods - see usb_reset_device()).
> > In such condition, usb_unbind_interface() gets called and this
> > function calls usb_cancel_queued_reset() which does nothing
> > because the flag "reset_running" is set to 1. The second reset
> > work that has been scheduled is therefore not cancelled.
> > Later, the usb_reset_device() tries to rebind the interface.
> > If it fails, then the usb interface context which contain the
> > reset work struct is freed and it most likely crash when the
> > second reset work tries to be run.
> 
> There was an earlier patch posted for testing (no results yet)  
> affecting this same region of code:
> 
> 	http://marc.info/?l=linux-usb&m=142064533924019&w=2
> 
> It should fix the problem described here, because (among other things) 
> it adds usb_get/put_intf calls to the delayed-reset routines.

Ok sorry I didn't see that patch. It looks indeed that it should
solve my problem.
I'll give it a try tomorrow.

Thank you,

-- 
Olivier

Re: [PATCH 11/11] usb: core: fix a race with usb_queue_reset_device()

[Olivier Sobrie]

Hello Alan,

On Tue, Jan 20, 2015 at 10:26:30AM -0500, Alan Stern wrote:
> On Tue, 20 Jan 2015, Olivier Sobrie wrote:
> 
> > When usb_queue_reset() is called it schedules a work in view of
> > resetting the usb interface. When the reset work is running, it
> > can be scheduled again (e.g. by the usb disconnect method of
> > the driver).
> > 
> > Consider that the reset work is queued again while the reset work
> > is running and that this work leads to a forced unbinding of the
> > usb interface (e.g. because a driver is bound to the interface
> > and has no pre/post_reset methods - see usb_reset_device()).
> > In such condition, usb_unbind_interface() gets called and this
> > function calls usb_cancel_queued_reset() which does nothing
> > because the flag "reset_running" is set to 1. The second reset
> > work that has been scheduled is therefore not cancelled.
> > Later, the usb_reset_device() tries to rebind the interface.
> > If it fails, then the usb interface context which contain the
> > reset work struct is freed and it most likely crash when the
> > second reset work tries to be run.
> 
> There was an earlier patch posted for testing (no results yet)  
> affecting this same region of code:
> 
> 	http://marc.info/?l=linux-usb&m=142064533924019&w=2
> 
> It should fix the problem described here, because (among other things) 
> it adds usb_get/put_intf calls to the delayed-reset routines.

I tested your patch. It also fixes the problem I observed.
You can drop mine.

For your info:

My test consists in powering down a usb hso modem while one of its
serial port is opened. It leads to two URB failures, each urb callback
queues a reset.
Without your fix (or without the one I sent), a crash happens after
less than ~20 power up/down sequences. With your fix, after more than
1000 power up/down I don't see any crash.

Thanks,

-- 
Olivier

Re: [PATCH 11/11] usb: core: fix a race with usb_queue_reset_device()

[Alan Stern]

On Wed, 21 Jan 2015, Olivier Sobrie wrote:

> I tested your patch. It also fixes the problem I observed.
> You can drop mine.
> 
> For your info:
> 
> My test consists in powering down a usb hso modem while one of its
> serial port is opened. It leads to two URB failures, each urb callback
> queues a reset.
> Without your fix (or without the one I sent), a crash happens after
> less than ~20 power up/down sequences. With your fix, after more than
> 1000 power up/down I don't see any crash.

Okay, I'll submit the patch.  Thanks for testing it.

Alan Stern

Re: [PATCH 04/11] hso: fix memory leak in hso_create_rfkill()

[Oliver Neukum]

On Tue, 2015-01-20 at 13:29 +0100, Olivier Sobrie wrote:
> When the rfkill interface was created, a buffer containing the name
> of the rfkill node was allocated. This buffer was never freed when the
> device disappears.
> 
> To fix the problem, we put the name given to rfkill_alloc() in
> the hso_net structure.
> 
> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
>  drivers/net/usb/hso.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 470ef9e..a49ac2e 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -153,6 +153,7 @@ struct hso_net {
>  	struct hso_device *parent;
>  	struct net_device *net;
>  	struct rfkill *rfkill;
> +	char name[8];
>  
>  	struct usb_endpoint_descriptor *in_endp;
>  	struct usb_endpoint_descriptor *out_endp;
> @@ -2467,27 +2468,20 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
>  {
>  	struct hso_net *hso_net = dev2net(hso_dev);
>  	struct device *dev = &hso_net->net->dev;
> -	char *rfkn;
>  
> -	rfkn = kzalloc(20, GFP_KERNEL);
> -	if (!rfkn)
> -		dev_err(dev, "%s - Out of memory\n", __func__);
> -
> -	snprintf(rfkn, 20, "hso-%d",
> +	snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
>  		 interface->altsetting->desc.bInterfaceNumber);

That number is not unique. Indeed it will be identical for all devices.

	Regards
		Oliver

-- 
Oliver Neukum <oneukum@suse.de>

Re: [PATCH 04/11] hso: fix memory leak in hso_create_rfkill()

[Olivier Sobrie]

On Tue, Jan 20, 2015 at 02:13:17PM +0100, Oliver Neukum wrote:
> On Tue, 2015-01-20 at 13:29 +0100, Olivier Sobrie wrote:
> > When the rfkill interface was created, a buffer containing the name
> > of the rfkill node was allocated. This buffer was never freed when the
> > device disappears.
> > 
> > To fix the problem, we put the name given to rfkill_alloc() in
> > the hso_net structure.
> > 
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> >  drivers/net/usb/hso.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index 470ef9e..a49ac2e 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -153,6 +153,7 @@ struct hso_net {
> >  	struct hso_device *parent;
> >  	struct net_device *net;
> >  	struct rfkill *rfkill;
> > +	char name[8];
> >  
> >  	struct usb_endpoint_descriptor *in_endp;
> >  	struct usb_endpoint_descriptor *out_endp;
> > @@ -2467,27 +2468,20 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
> >  {
> >  	struct hso_net *hso_net = dev2net(hso_dev);
> >  	struct device *dev = &hso_net->net->dev;
> > -	char *rfkn;
> >  
> > -	rfkn = kzalloc(20, GFP_KERNEL);
> > -	if (!rfkn)
> > -		dev_err(dev, "%s - Out of memory\n", __func__);
> > -
> > -	snprintf(rfkn, 20, "hso-%d",
> > +	snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
> >  		 interface->altsetting->desc.bInterfaceNumber);
> 
> That number is not unique. Indeed it will be identical for all devices.

Indeed. That should be corrected too.
Thank you,

Olivier

Re: [PATCH 04/11] hso: fix memory leak in hso_create_rfkill()

[Dan Williams]

On Tue, 2015-01-20 at 14:13 +0100, Oliver Neukum wrote:
> On Tue, 2015-01-20 at 13:29 +0100, Olivier Sobrie wrote:
> > When the rfkill interface was created, a buffer containing the name
> > of the rfkill node was allocated. This buffer was never freed when the
> > device disappears.
> > 
> > To fix the problem, we put the name given to rfkill_alloc() in
> > the hso_net structure.
> > 
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> >  drivers/net/usb/hso.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index 470ef9e..a49ac2e 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -153,6 +153,7 @@ struct hso_net {
> >  	struct hso_device *parent;
> >  	struct net_device *net;
> >  	struct rfkill *rfkill;
> > +	char name[8];
> >  
> >  	struct usb_endpoint_descriptor *in_endp;
> >  	struct usb_endpoint_descriptor *out_endp;
> > @@ -2467,27 +2468,20 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
> >  {
> >  	struct hso_net *hso_net = dev2net(hso_dev);
> >  	struct device *dev = &hso_net->net->dev;
> > -	char *rfkn;
> >  
> > -	rfkn = kzalloc(20, GFP_KERNEL);
> > -	if (!rfkn)
> > -		dev_err(dev, "%s - Out of memory\n", __func__);
> > -
> > -	snprintf(rfkn, 20, "hso-%d",
> > +	snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
> >  		 interface->altsetting->desc.bInterfaceNumber);
> 
> That number is not unique. Indeed it will be identical for all devices.

I would say just do "static u32 rfkill_counter = 0" and 

+	snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
+  		 rfkill_counter++);

We can't just use the netdev's name because that may have conflicts.
eg, the netdev will get hso0 when plugged in (and thus rfkill would get
hso-0) but then udev will rename that to something like wwp0s26f7u2i8.
Then the second HSO you plug in will get the name 'hso0', and so the
second rfkill would get 'hso-0', but that's already taken by the first
rfkill...  Which is why I just suggest a counter.

Dan

[PATCH v2 00/11] hso: fix some problems in the disconnect path

[Olivier Sobrie]

These patches attempt to fix some problems I observed when the hso
device is disconnected.
Several patches of this serie are fixing crashes or memleaks when a
hso device is disconnected.
This serie of patches is based on v3.18.

changes in v2:
 - Last patch of the serie dropped since another patch fix the issue.
   See http://marc.info/?l=linux-usb&m=142186699418489 for more info.

 - Added an extra patch avoiding name conflicts for the rfkill interface.

Olivier Sobrie (11):
  hso: remove useless header file timer.h
  hso: fix crash when device disappears while serial port is open
  hso: fix memory leak when device disconnects
  hso: fix memory leak in hso_create_rfkill()
  hso: fix small indentation error
  hso: rename hso_dev into serial in hso_free_interface()
  hso: replace reset_device work by usb_queue_reset_device()
  hso: move tty_unregister outside hso_serial_common_free()
  hso: update serial_table in usb disconnect method
  hso: add missing cancel_work_sync in disconnect()
  hso: fix rfkill name conflicts

 drivers/net/usb/hso.c | 91 ++++++++++++++++++++++-----------------------------
 1 file changed, 40 insertions(+), 51 deletions(-)

-- 
2.2.0

[PATCH v2 01/11] hso: remove useless header file timer.h

[Olivier Sobrie]

No timer related function is used in this driver.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index babda7d..cb0bcc1 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -58,7 +58,6 @@
 #include <linux/module.h>
 #include <linux/ethtool.h>
 #include <linux/usb.h>
-#include <linux/timer.h>
 #include <linux/tty.h>
 #include <linux/tty_driver.h>
 #include <linux/tty_flip.h>
-- 
2.2.0

[PATCH v2 02/11] hso: fix crash when device disappears while serial port is open

[Olivier Sobrie]

When the device disappear, the function hso_disconnect() is called to
perform cleanup. In the cleanup function, hso_free_interface() calls
tty_port_tty_hangup() in view of scheduling a work to hang up the tty if
needed. If the port was not open then hso_serial_ref_free() is called
directly to cleanup everything. Otherwise, hso_serial_ref_free() is called
when the last fd associated to the port is closed.

For each open port, tty_release() will call the close method,
hso_serial_close(), which drops the last kref and call
hso_serial_ref_free() which unregisters, destroys the tty port
and finally frees the structure in which the tty_port structure
is included. Later, in tty_release(), more precisely when release_tty()
is called, the tty_port previously freed is accessed to cancel
the tty buf workqueue and it leads to a crash.

In view of avoiding this crash, we add a cleanup method that is called
at the end of the hangup process and we drop the last kref in this
function when all the ports have been closed, when tty_port is no
more needed and when it is safe to free the structure containing the
tty_port structure.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index cb0bcc1..3a6c630 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -1270,7 +1270,6 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		goto err_out;
 
 	D1("Opening %d", serial->minor);
-	kref_get(&serial->parent->ref);
 
 	/* setup */
 	tty->driver_data = serial;
@@ -1289,7 +1288,8 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		if (result) {
 			hso_stop_serial_device(serial->parent);
 			serial->port.count--;
-			kref_put(&serial->parent->ref, hso_serial_ref_free);
+		} else {
+			kref_get(&serial->parent->ref);
 		}
 	} else {
 		D1("Port was already open");
@@ -1339,8 +1339,6 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 		usb_autopm_put_interface(serial->parent->interface);
 
 	mutex_unlock(&serial->parent->mutex);
-
-	kref_put(&serial->parent->ref, hso_serial_ref_free);
 }
 
 /* close the requested serial port */
@@ -1391,6 +1389,16 @@ static int hso_serial_write_room(struct tty_struct *tty)
 	return room;
 }
 
+static void hso_serial_cleanup(struct tty_struct *tty)
+{
+	struct hso_serial *serial = tty->driver_data;
+
+	if (!serial)
+		return;
+
+	kref_put(&serial->parent->ref, hso_serial_ref_free);
+}
+
 /* setup the term */
 static void hso_serial_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
@@ -3215,6 +3223,7 @@ static const struct tty_operations hso_serial_ops = {
 	.close = hso_serial_close,
 	.write = hso_serial_write,
 	.write_room = hso_serial_write_room,
+	.cleanup = hso_serial_cleanup,
 	.ioctl = hso_serial_ioctl,
 	.set_termios = hso_serial_set_termios,
 	.chars_in_buffer = hso_serial_chars_in_buffer,
-- 
2.2.0

[PATCH v2 03/11] hso: fix memory leak when device disconnects

[Olivier Sobrie]

In the disconnect path, tx_buffer should freed like tx_data to avoid
a memory leak when the device disconnects.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 3a6c630..470ef9e 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2253,6 +2253,7 @@ static void hso_serial_common_free(struct hso_serial *serial)
 
 	/* unlink and free TX URB */
 	usb_free_urb(serial->tx_urb);
+	kfree(serial->tx_buffer);
 	kfree(serial->tx_data);
 	tty_port_destroy(&serial->port);
 }
-- 
2.2.0

[PATCH v2 04/11] hso: fix memory leak in hso_create_rfkill()

[Olivier Sobrie]

When the rfkill interface was created, a buffer containing the name
of the rfkill node was allocated. This buffer was never freed when the
device disappears.

To fix the problem, we put the name given to rfkill_alloc() in
the hso_net structure.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 470ef9e..a49ac2e 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -153,6 +153,7 @@ struct hso_net {
 	struct hso_device *parent;
 	struct net_device *net;
 	struct rfkill *rfkill;
+	char name[8];
 
 	struct usb_endpoint_descriptor *in_endp;
 	struct usb_endpoint_descriptor *out_endp;
@@ -2467,27 +2468,20 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
 {
 	struct hso_net *hso_net = dev2net(hso_dev);
 	struct device *dev = &hso_net->net->dev;
-	char *rfkn;
 
-	rfkn = kzalloc(20, GFP_KERNEL);
-	if (!rfkn)
-		dev_err(dev, "%s - Out of memory\n", __func__);
-
-	snprintf(rfkn, 20, "hso-%d",
+	snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
 		 interface->altsetting->desc.bInterfaceNumber);
 
-	hso_net->rfkill = rfkill_alloc(rfkn,
+	hso_net->rfkill = rfkill_alloc(hso_net->name,
 				       &interface_to_usbdev(interface)->dev,
 				       RFKILL_TYPE_WWAN,
 				       &hso_rfkill_ops, hso_dev);
 	if (!hso_net->rfkill) {
 		dev_err(dev, "%s - Out of memory\n", __func__);
-		kfree(rfkn);
 		return;
 	}
 	if (rfkill_register(hso_net->rfkill) < 0) {
 		rfkill_destroy(hso_net->rfkill);
-		kfree(rfkn);
 		hso_net->rfkill = NULL;
 		dev_err(dev, "%s - Failed to register rfkill\n", __func__);
 		return;
-- 
2.2.0

[PATCH v2 05/11] hso: fix small indentation error

[Olivier Sobrie]

Simply remove the useless extra tab.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index a49ac2e..2f2343d 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2206,8 +2206,8 @@ static int hso_stop_serial_device(struct hso_device *hso_dev)
 
 	for (i = 0; i < serial->num_rx_urbs; i++) {
 		if (serial->rx_urb[i]) {
-				usb_kill_urb(serial->rx_urb[i]);
-				serial->rx_urb_filled[i] = 0;
+			usb_kill_urb(serial->rx_urb[i]);
+			serial->rx_urb_filled[i] = 0;
 		}
 	}
 	serial->curr_rx_urb_idx = 0;
-- 
2.2.0

[PATCH v2 06/11] hso: rename hso_dev into serial in hso_free_interface()

[Olivier Sobrie]

In other functions of the driver, variables of type "struct hso_serial"
are denoted by "serial" and variables of type "struct hso_device" are
denoted by "hso_dev". This patch makes the hso_free_interface()
consistent with these notations.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 2f2343d..484e423 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -3115,17 +3115,17 @@ static void hso_serial_ref_free(struct kref *ref)
 
 static void hso_free_interface(struct usb_interface *interface)
 {
-	struct hso_serial *hso_dev;
+	struct hso_serial *serial;
 	int i;
 
 	for (i = 0; i < HSO_SERIAL_TTY_MINORS; i++) {
 		if (serial_table[i] &&
 		    (serial_table[i]->interface == interface)) {
-			hso_dev = dev2ser(serial_table[i]);
-			tty_port_tty_hangup(&hso_dev->port, false);
-			mutex_lock(&hso_dev->parent->mutex);
-			hso_dev->parent->usb_gone = 1;
-			mutex_unlock(&hso_dev->parent->mutex);
+			serial = dev2ser(serial_table[i]);
+			tty_port_tty_hangup(&serial->port, false);
+			mutex_lock(&serial->parent->mutex);
+			serial->parent->usb_gone = 1;
+			mutex_unlock(&serial->parent->mutex);
 			kref_put(&serial_table[i]->ref, hso_serial_ref_free);
 		}
 	}
-- 
2.2.0

[PATCH v2 07/11] hso: replace reset_device work by usb_queue_reset_device()

[Olivier Sobrie]

There is no need for a dedicated reset work in the hso driver since
there is already a reset work foreseen in usb_interface that does
the same.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 484e423..55074da 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -274,7 +274,6 @@ struct hso_device {
 	u8 usb_gone;
 	struct work_struct async_get_intf;
 	struct work_struct async_put_intf;
-	struct work_struct reset_device;
 
 	struct usb_device *usb;
 	struct usb_interface *interface;
@@ -340,7 +339,6 @@ static void async_put_intf(struct work_struct *data);
 static int hso_put_activity(struct hso_device *hso_dev);
 static int hso_get_activity(struct hso_device *hso_dev);
 static void tiocmget_intr_callback(struct urb *urb);
-static void reset_device(struct work_struct *data);
 /*****************************************************************************/
 /* Helping functions                                                         */
 /*****************************************************************************/
@@ -696,7 +694,7 @@ static void handle_usb_error(int status, const char *function,
 	case -ETIMEDOUT:
 		explanation = "protocol error";
 		if (hso_dev)
-			schedule_work(&hso_dev->reset_device);
+			usb_queue_reset_device(hso_dev->interface);
 		break;
 	default:
 		explanation = "unknown status";
@@ -2347,7 +2345,6 @@ static struct hso_device *hso_create_device(struct usb_interface *intf,
 
 	INIT_WORK(&hso_dev->async_get_intf, async_get_intf);
 	INIT_WORK(&hso_dev->async_put_intf, async_put_intf);
-	INIT_WORK(&hso_dev->reset_device, reset_device);
 
 	return hso_dev;
 }
@@ -3086,26 +3083,6 @@ out:
 	return result;
 }
 
-static void reset_device(struct work_struct *data)
-{
-	struct hso_device *hso_dev =
-	    container_of(data, struct hso_device, reset_device);
-	struct usb_device *usb = hso_dev->usb;
-	int result;
-
-	if (hso_dev->usb_gone) {
-		D1("No reset during disconnect\n");
-	} else {
-		result = usb_lock_device_for_reset(usb, hso_dev->interface);
-		if (result < 0)
-			D1("unable to lock device for reset: %d\n", result);
-		else {
-			usb_reset_device(usb);
-			usb_unlock_device(usb);
-		}
-	}
-}
-
 static void hso_serial_ref_free(struct kref *ref)
 {
 	struct hso_device *hso_dev = container_of(ref, struct hso_device, ref);
-- 
2.2.0

[PATCH v2 08/11] hso: move tty_unregister outside hso_serial_common_free()

[Olivier Sobrie]

The function hso_serial_common_free() is called either by the cleanup
method of the tty or by the usb disconnect method.
In the former case, the usb_disconnect() has been already called
and the sysfs group associated to the device has been removed.
By calling tty_unregister directly from the usb_disconnect() method,
we avoid a warning due to the removal of the sysfs group of the usb
device.

Example of warning:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 778 at fs/sysfs/group.c:225 sysfs_remove_group+0x50/0x94()
sysfs group c0645a88 not found for kobject 'ttyHS5'
Modules linked in:
CPU: 0 PID: 778 Comm: kworker/0:3 Tainted: G        W      3.18.0+ #105
Workqueue: events release_one_tty
[<c000dfe4>] (unwind_backtrace) from [<c000c014>] (show_stack+0x14/0x1c)
[<c000c014>] (show_stack) from [<c0016bac>] (warn_slowpath_common+0x5c/0x7c)
[<c0016bac>] (warn_slowpath_common) from [<c0016c60>] (warn_slowpath_fmt+0x30/0x40)
[<c0016c60>] (warn_slowpath_fmt) from [<c00ddd14>] (sysfs_remove_group+0x50/0x94)
[<c00ddd14>] (sysfs_remove_group) from [<c0221e44>] (device_del+0x30/0x190)
[<c0221e44>] (device_del) from [<c0221fb0>] (device_unregister+0xc/0x18)
[<c0221fb0>] (device_unregister) from [<c0221fec>] (device_destroy+0x30/0x3c)
[<c0221fec>] (device_destroy) from [<c01fe1dc>] (tty_unregister_device+0x2c/0x5c)
[<c01fe1dc>] (tty_unregister_device) from [<c029a428>] (hso_serial_common_free+0x2c/0x88)
[<c029a428>] (hso_serial_common_free) from [<c029a4c0>] (hso_serial_ref_free+0x3c/0xb8)
[<c029a4c0>] (hso_serial_ref_free) from [<c01ff430>] (release_one_tty+0x30/0x84)
[<c01ff430>] (release_one_tty) from [<c00271d4>] (process_one_work+0x21c/0x3c8)
[<c00271d4>] (process_one_work) from [<c0027758>] (worker_thread+0x3d8/0x560)
[<c0027758>] (worker_thread) from [<c002be4c>] (kthread+0xc0/0xcc)
[<c002be4c>] (kthread) from [<c0009630>] (ret_from_fork+0x14/0x24)
---[ end trace cb88537fdc8fa208 ]---

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 55074da..5b157ad 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2234,14 +2234,17 @@ static int hso_stop_serial_device(struct hso_device *hso_dev)
 	return 0;
 }
 
-static void hso_serial_common_free(struct hso_serial *serial)
+static void hso_serial_tty_unregister(struct hso_serial *serial)
 {
-	int i;
-
 	if (serial->parent->dev)
 		device_remove_file(serial->parent->dev, &dev_attr_hsotype);
 
 	tty_unregister_device(tty_drv, serial->minor);
+}
+
+static void hso_serial_common_free(struct hso_serial *serial)
+{
+	int i;
 
 	for (i = 0; i < serial->num_rx_urbs; i++) {
 		/* unlink and free RX URB */
@@ -2323,6 +2326,7 @@ static int hso_serial_common_create(struct hso_serial *serial, int num_urbs,
 
 	return 0;
 exit:
+	hso_serial_tty_unregister(serial);
 	hso_serial_common_free(serial);
 	return -1;
 }
@@ -2683,6 +2687,7 @@ static struct hso_device *hso_create_bulk_serial_device(
 	return hso_dev;
 
 exit2:
+	hso_serial_tty_unregister(serial);
 	hso_serial_common_free(serial);
 exit:
 	hso_free_tiomget(serial);
@@ -3103,6 +3108,7 @@ static void hso_free_interface(struct usb_interface *interface)
 			mutex_lock(&serial->parent->mutex);
 			serial->parent->usb_gone = 1;
 			mutex_unlock(&serial->parent->mutex);
+			hso_serial_tty_unregister(serial);
 			kref_put(&serial_table[i]->ref, hso_serial_ref_free);
 		}
 	}
-- 
2.2.0

[PATCH v2 09/11] hso: update serial_table in usb disconnect method

[Olivier Sobrie]

The serial_table is used to map the minor number of the usb serial device
to its associated context. The table is updated in the probe method and
in hso_serial_ref_free() which is called either from the tty cleanup
method or from the usb disconnect method.
This patch ensures that the serial_table is updated in the disconnect
method and no more from the cleanup method to avoid the following
potential race condition.

 - hso_disconnect() is called for usb interface "x". Because the serial
   port was open and because the cleanup method of the tty_port hasn't
   been called yet, hso_serial_ref_free() is not run.
 - hso_probe() is called and fails for a new hso serial usb interface
   "y". The function hso_free_interface() is called and iterates
   over the element of serial_table to find the device associated to
   the usb interface context.
   If the usb interface context of usb interface "y" has been created
   at the same place as for usb interface "x", then the cleanup
   functions are called for usb interfaces "x" and "y" and
   hso_serial_ref_free() is called for both interfaces.
 - release_tty() is called for serial port linked to usb interface "x"
   and possibly crash because the tty_port structure contained in the
   hso_device structure has been freed.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 5b157ad..c916ab5 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2597,7 +2597,6 @@ static void hso_free_serial_device(struct hso_device *hso_dev)
 
 	if (!serial)
 		return;
-	set_serial_by_index(serial->minor, NULL);
 
 	hso_serial_common_free(serial);
 
@@ -3110,6 +3109,7 @@ static void hso_free_interface(struct usb_interface *interface)
 			mutex_unlock(&serial->parent->mutex);
 			hso_serial_tty_unregister(serial);
 			kref_put(&serial_table[i]->ref, hso_serial_ref_free);
+			set_serial_by_index(i, NULL);
 		}
 	}
 
-- 
2.2.0

[PATCH v2 10/11] hso: add missing cancel_work_sync in disconnect()

[Olivier Sobrie]

For hso serial devices, two cancel_work_sync were missing in the
disconnect method.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index c916ab5..c14fc80 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -3107,6 +3107,8 @@ static void hso_free_interface(struct usb_interface *interface)
 			mutex_lock(&serial->parent->mutex);
 			serial->parent->usb_gone = 1;
 			mutex_unlock(&serial->parent->mutex);
+			cancel_work_sync(&serial_table[i]->async_put_intf);
+			cancel_work_sync(&serial_table[i]->async_get_intf);
 			hso_serial_tty_unregister(serial);
 			kref_put(&serial_table[i]->ref, hso_serial_ref_free);
 			set_serial_by_index(i, NULL);
-- 
2.2.0

[PATCH v2 11/11] hso: fix rfkill name conflicts

[Olivier Sobrie]

By using only the usb interface number for the rfkill name, we might
have a name conflicts in case two similar hso devices are connected.

In this patch, the name of the hso rfkill interface embed the value
of a counter that is incremented each time a new rfkill interface is
added.

Suggested-by: Dan Williams <dcbw@redhat.com>
Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/usb/hso.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index c14fc80..d31a165 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -153,7 +153,7 @@ struct hso_net {
 	struct hso_device *parent;
 	struct net_device *net;
 	struct rfkill *rfkill;
-	char name[8];
+	char name[24];
 
 	struct usb_endpoint_descriptor *in_endp;
 	struct usb_endpoint_descriptor *out_endp;
@@ -2469,9 +2469,10 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
 {
 	struct hso_net *hso_net = dev2net(hso_dev);
 	struct device *dev = &hso_net->net->dev;
+	static u32 rfkill_counter;
 
 	snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
-		 interface->altsetting->desc.bInterfaceNumber);
+		 rfkill_counter++);
 
 	hso_net->rfkill = rfkill_alloc(hso_net->name,
 				       &interface_to_usbdev(interface)->dev,
-- 
2.2.0

Re: [PATCH v2 11/11] hso: fix rfkill name conflicts

[Dan Williams]

On Fri, 2015-01-30 at 13:22 +0100, Olivier Sobrie wrote:
> By using only the usb interface number for the rfkill name, we might
> have a name conflicts in case two similar hso devices are connected.
> 
> In this patch, the name of the hso rfkill interface embed the value
> of a counter that is incremented each time a new rfkill interface is
> added.
> 
> Suggested-by: Dan Williams <dcbw@redhat.com>
> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
>  drivers/net/usb/hso.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index c14fc80..d31a165 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -153,7 +153,7 @@ struct hso_net {
>  	struct hso_device *parent;
>  	struct net_device *net;
>  	struct rfkill *rfkill;
> -	char name[8];
> +	char name[24];
>  
>  	struct usb_endpoint_descriptor *in_endp;
>  	struct usb_endpoint_descriptor *out_endp;
> @@ -2469,9 +2469,10 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
>  {
>  	struct hso_net *hso_net = dev2net(hso_dev);
>  	struct device *dev = &hso_net->net->dev;
> +	static u32 rfkill_counter;

It'll probably be initialized to 0, but still, it would feel safer with
an explicit "rfkill_counter = 0"...

Dan
 
>  	snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
> -		 interface->altsetting->desc.bInterfaceNumber);
> +		 rfkill_counter++);
>  
>  	hso_net->rfkill = rfkill_alloc(hso_net->name,
>  				       &interface_to_usbdev(interface)->dev,

Re: [PATCH v2 11/11] hso: fix rfkill name conflicts

[Olivier Sobrie]

Hello Dan,

On Fri, Jan 30, 2015 at 09:47:59AM -0600, Dan Williams wrote:
> On Fri, 2015-01-30 at 13:22 +0100, Olivier Sobrie wrote:
> > By using only the usb interface number for the rfkill name, we might
> > have a name conflicts in case two similar hso devices are connected.
> > 
> > In this patch, the name of the hso rfkill interface embed the value
> > of a counter that is incremented each time a new rfkill interface is
> > added.
> > 
> > Suggested-by: Dan Williams <dcbw@redhat.com>
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> >  drivers/net/usb/hso.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index c14fc80..d31a165 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -153,7 +153,7 @@ struct hso_net {
> >  	struct hso_device *parent;
> >  	struct net_device *net;
> >  	struct rfkill *rfkill;
> > -	char name[8];
> > +	char name[24];
> >  
> >  	struct usb_endpoint_descriptor *in_endp;
> >  	struct usb_endpoint_descriptor *out_endp;
> > @@ -2469,9 +2469,10 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
> >  {
> >  	struct hso_net *hso_net = dev2net(hso_dev);
> >  	struct device *dev = &hso_net->net->dev;
> > +	static u32 rfkill_counter;
> 
> It'll probably be initialized to 0, but still, it would feel safer with
> an explicit "rfkill_counter = 0"...
> 

If I set explicitly rfkill_counter = 0, checkpatch triggers an error:

  ERROR: do not initialise statics to 0 or NULL
  #36: FILE: drivers/net/usb/hso.c:2472:
  +	static u32 rfkill_counter = 0;

Olivier

Re: [PATCH v2 11/11] hso: fix rfkill name conflicts

[Dan Williams]

On Fri, 2015-01-30 at 17:15 +0100, Olivier Sobrie wrote:
> Hello Dan,
> 
> On Fri, Jan 30, 2015 at 09:47:59AM -0600, Dan Williams wrote:
> > On Fri, 2015-01-30 at 13:22 +0100, Olivier Sobrie wrote:
> > > By using only the usb interface number for the rfkill name, we might
> > > have a name conflicts in case two similar hso devices are connected.
> > > 
> > > In this patch, the name of the hso rfkill interface embed the value
> > > of a counter that is incremented each time a new rfkill interface is
> > > added.
> > > 
> > > Suggested-by: Dan Williams <dcbw@redhat.com>
> > > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > > ---
> > >  drivers/net/usb/hso.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > > index c14fc80..d31a165 100644
> > > --- a/drivers/net/usb/hso.c
> > > +++ b/drivers/net/usb/hso.c
> > > @@ -153,7 +153,7 @@ struct hso_net {
> > >  	struct hso_device *parent;
> > >  	struct net_device *net;
> > >  	struct rfkill *rfkill;
> > > -	char name[8];
> > > +	char name[24];
> > >  
> > >  	struct usb_endpoint_descriptor *in_endp;
> > >  	struct usb_endpoint_descriptor *out_endp;
> > > @@ -2469,9 +2469,10 @@ static void hso_create_rfkill(struct hso_device *hso_dev,
> > >  {
> > >  	struct hso_net *hso_net = dev2net(hso_dev);
> > >  	struct device *dev = &hso_net->net->dev;
> > > +	static u32 rfkill_counter;
> > 
> > It'll probably be initialized to 0, but still, it would feel safer with
> > an explicit "rfkill_counter = 0"...
> > 
> 
> If I set explicitly rfkill_counter = 0, checkpatch triggers an error:
> 
>   ERROR: do not initialise statics to 0 or NULL
>   #36: FILE: drivers/net/usb/hso.c:2472:
>   +	static u32 rfkill_counter = 0;

Well OK then, life has changed since I last submitted a patch with a
static variable :)  If checkpatch complains, then it is almost always
correct, and you can ignore me.

Dan

Re: [PATCH v2 00/11] hso: fix some problems in the disconnect path

[David Miller]

From: Olivier Sobrie <olivier@sobrie.be>
Date: Fri, 30 Jan 2015 13:21:52 +0100

> These patches attempt to fix some problems I observed when the hso
> device is disconnected.
> Several patches of this serie are fixing crashes or memleaks when a
> hso device is disconnected.
> This serie of patches is based on v3.18.
> 
> changes in v2:
>  - Last patch of the serie dropped since another patch fix the issue.
>    See http://marc.info/?l=linux-usb&m=142186699418489 for more info.
> 
>  - Added an extra patch avoiding name conflicts for the rfkill interface.

Series applied to net-next, thanks.