[PATCH 0/2] USB: serial: allow hung up ports to be suspended

[PATCH 0/2] USB: serial: allow hung up ports to be suspended

[Johan Hovold]

User space can keep a tty open indefinitely and that should not prevent
a hung up port and its USB device from being runtime suspended.

Also clean up a few related error labels in a preparatory patch.

Johan


Johan Hovold (2):
  USB: serial: clean up core error labels
  USB: serial: allow hung up ports to be suspended

 drivers/usb/serial/usb-serial.c | 59 +++++++++++++++++----------------
 1 file changed, 30 insertions(+), 29 deletions(-)

-- 
2.32.0

[PATCH 1/2] USB: serial: clean up core error labels

[Johan Hovold]

Clean up the core error labels by consistently naming them after what
they do rather than after from where they are jumped to.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/usb-serial.c | 34 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index eeb441c77207..abc657fec8c5 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -225,17 +225,17 @@ static int serial_install(struct tty_driver *driver, struct tty_struct *tty)
 
 	serial = port->serial;
 	if (!try_module_get(serial->type->driver.owner))
-		goto error_module_get;
+		goto err_put_serial;
 
 	retval = usb_autopm_get_interface(serial->interface);
 	if (retval)
-		goto error_get_interface;
+		goto err_put_module;
 
 	init_termios = (driver->termios[idx] == NULL);
 
 	retval = tty_standard_install(driver, tty);
 	if (retval)
-		goto error_init_termios;
+		goto err_put_autopm;
 
 	mutex_unlock(&serial->disc_mutex);
 
@@ -247,11 +247,11 @@ static int serial_install(struct tty_driver *driver, struct tty_struct *tty)
 
 	return retval;
 
- error_init_termios:
+err_put_autopm:
 	usb_autopm_put_interface(serial->interface);
- error_get_interface:
+err_put_module:
 	module_put(serial->type->driver.owner);
- error_module_get:
+err_put_serial:
 	usb_serial_put(serial);
 	mutex_unlock(&serial->disc_mutex);
 	return retval;
@@ -1327,7 +1327,7 @@ static int __init usb_serial_init(void)
 	result = bus_register(&usb_serial_bus_type);
 	if (result) {
 		pr_err("%s - registering bus driver failed\n", __func__);
-		goto exit_bus;
+		goto err_put_driver;
 	}
 
 	usb_serial_tty_driver->driver_name = "usbserial";
@@ -1347,25 +1347,23 @@ static int __init usb_serial_init(void)
 	result = tty_register_driver(usb_serial_tty_driver);
 	if (result) {
 		pr_err("%s - tty_register_driver failed\n", __func__);
-		goto exit_reg_driver;
+		goto err_unregister_bus;
 	}
 
 	/* register the generic driver, if we should */
 	result = usb_serial_generic_register();
 	if (result < 0) {
 		pr_err("%s - registering generic driver failed\n", __func__);
-		goto exit_generic;
+		goto err_unregister_driver;
 	}
 
 	return result;
 
-exit_generic:
+err_unregister_driver:
 	tty_unregister_driver(usb_serial_tty_driver);
-
-exit_reg_driver:
+err_unregister_bus:
 	bus_unregister(&usb_serial_bus_type);
-
-exit_bus:
+err_put_driver:
 	pr_err("%s - returning with error %d\n", __func__, result);
 	put_tty_driver(usb_serial_tty_driver);
 	return result;
@@ -1510,13 +1508,13 @@ int usb_serial_register_drivers(struct usb_serial_driver *const serial_drivers[]
 
 	rc = usb_register(udriver);
 	if (rc)
-		goto failed_usb_register;
+		goto err_free_driver;
 
 	for (sd = serial_drivers; *sd; ++sd) {
 		(*sd)->usb_driver = udriver;
 		rc = usb_serial_register(*sd);
 		if (rc)
-			goto failed;
+			goto err_deregister_drivers;
 	}
 
 	/* Now set udriver's id_table and look for matches */
@@ -1524,11 +1522,11 @@ int usb_serial_register_drivers(struct usb_serial_driver *const serial_drivers[]
 	rc = driver_attach(&udriver->drvwrap.driver);
 	return 0;
 
- failed:
+err_deregister_drivers:
 	while (sd-- > serial_drivers)
 		usb_serial_deregister(*sd);
 	usb_deregister(udriver);
-failed_usb_register:
+err_free_driver:
 	kfree(udriver);
 	return rc;
 }
-- 
2.32.0

[PATCH 2/2] USB: serial: allow hung up ports to be suspended

[Johan Hovold]

User space can keep a tty open indefinitely and that should not prevent
a hung up port and its USB device from being runtime suspended.

Fix this by incrementing the PM usage counter when the port it activated
and decrementing the counter when the port is shutdown rather than when
the tty is installed and the last reference is dropped, respectively.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/usb-serial.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index abc657fec8c5..b357361d5d1e 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -208,8 +208,8 @@ void usb_serial_put(struct usb_serial *serial)
  *
  * This is the first place a new tty gets used.  Hence this is where we
  * acquire references to the usb_serial structure and the driver module,
- * where we store a pointer to the port, and where we do an autoresume.
- * All these actions are reversed in serial_cleanup().
+ * where we store a pointer to the port.  All these actions are reversed
+ * in serial_cleanup().
  */
 static int serial_install(struct tty_driver *driver, struct tty_struct *tty)
 {
@@ -227,15 +227,11 @@ static int serial_install(struct tty_driver *driver, struct tty_struct *tty)
 	if (!try_module_get(serial->type->driver.owner))
 		goto err_put_serial;
 
-	retval = usb_autopm_get_interface(serial->interface);
-	if (retval)
-		goto err_put_module;
-
 	init_termios = (driver->termios[idx] == NULL);
 
 	retval = tty_standard_install(driver, tty);
 	if (retval)
-		goto err_put_autopm;
+		goto err_put_module;
 
 	mutex_unlock(&serial->disc_mutex);
 
@@ -247,8 +243,6 @@ static int serial_install(struct tty_driver *driver, struct tty_struct *tty)
 
 	return retval;
 
-err_put_autopm:
-	usb_autopm_put_interface(serial->interface);
 err_put_module:
 	module_put(serial->type->driver.owner);
 err_put_serial:
@@ -265,10 +259,19 @@ static int serial_port_activate(struct tty_port *tport, struct tty_struct *tty)
 	int retval;
 
 	mutex_lock(&serial->disc_mutex);
-	if (serial->disconnected)
+	if (serial->disconnected) {
 		retval = -ENODEV;
-	else
-		retval = port->serial->type->open(tty, port);
+		goto out_unlock;
+	}
+
+	retval = usb_autopm_get_interface(serial->interface);
+	if (retval)
+		goto out_unlock;
+
+	retval = port->serial->type->open(tty, port);
+	if (retval)
+		usb_autopm_put_interface(serial->interface);
+out_unlock:
 	mutex_unlock(&serial->disc_mutex);
 
 	if (retval < 0)
@@ -304,6 +307,8 @@ static void serial_port_shutdown(struct tty_port *tport)
 
 	if (drv->close)
 		drv->close(port);
+
+	usb_autopm_put_interface(port->serial->interface);
 }
 
 static void serial_hangup(struct tty_struct *tty)
@@ -352,8 +357,6 @@ static void serial_cleanup(struct tty_struct *tty)
 	serial = port->serial;
 	owner = serial->type->driver.owner;
 
-	usb_autopm_put_interface(serial->interface);
-
 	usb_serial_put(serial);
 	module_put(owner);
 }
-- 
2.32.0

Re: [PATCH 0/2] USB: serial: allow hung up ports to be suspended

[Greg KH]

On Fri, Sep 10, 2021 at 02:11:26PM +0200, Johan Hovold wrote:
> User space can keep a tty open indefinitely and that should not prevent
> a hung up port and its USB device from being runtime suspended.
> 
> Also clean up a few related error labels in a preparatory patch.
> 
> Johan
> 
> 
> Johan Hovold (2):
>   USB: serial: clean up core error labels
>   USB: serial: allow hung up ports to be suspended
> 
>  drivers/usb/serial/usb-serial.c | 59 +++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> -- 
> 2.32.0
> 

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

Re: [PATCH 0/2] USB: serial: allow hung up ports to be suspended

[Johan Hovold]

On Fri, Sep 10, 2021 at 02:41:24PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 10, 2021 at 02:11:26PM +0200, Johan Hovold wrote:
> > User space can keep a tty open indefinitely and that should not prevent
> > a hung up port and its USB device from being runtime suspended.
> > 
> > Also clean up a few related error labels in a preparatory patch.
> > 
> > Johan
> > 
> > 
> > Johan Hovold (2):
> >   USB: serial: clean up core error labels
> >   USB: serial: allow hung up ports to be suspended
> > 
> >  drivers/usb/serial/usb-serial.c | 59 +++++++++++++++++----------------
> >  1 file changed, 30 insertions(+), 29 deletions(-)
> > 
> > -- 
> > 2.32.0
> > 
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for reviewing. Now applied.

Johan