diff mbox series

[(v2)] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

Message ID 40c5c70f-46ff-c5f3-212b-2badc47e49a3@0882a8b5-c6c3-11e9-b005-00805fc181fe.uuid.home.arpa
State New
Headers show
Series [(v2)] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs | expand

Commit Message

Simon Arlott Aug. 23, 2023, 9:12 p.m. UTC 
If the serial device never reads data written to it (because it is "output
only") then the write buffers will still be waiting for the URB to complete
on close(), which will hang for 30s until the closing_wait timeout expires.

This can happen with the ESP32-H2/ESP32-C6 USB serial interface. Instead of
changing all userspace applications to flush (discard) their output in this
specific scenario it would be easier to adjust the closing_wait timeout
with udev rules but the only available interface is the TIOCGSERIAL ioctl.

The serial_core driver (ttySx) exposes its supported ioctl values as
read-only sysfs attributes. Add read-write sysfs attributes "close_delay"
and "closing_wait" to cdc-acm (ttyACMx) devices. These are the same as the
attributes in serial_core except that the "closing_wait" sysfs values are
modified so that "-1" is used for "infinite wait" (instead of 0) and "0"
is used for "no wait" (instead of 65535).

Signed-off-by: Simon Arlott <simon@octiron.net>
---
> @@ -976,9 +973,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
> -	if (!capable(CAP_SYS_ADMIN)) {
> +	if (!admin) {
>  		if ((close_delay != acm->port.close_delay) ||
>  		    (closing_wait != acm->port.closing_wait))
>  			retval = -EPERM;

Sorry, I hadn't tested the ioctl and this return value wasn't actually
getting returned.

 Documentation/ABI/testing/sysfs-tty |  21 +++++
 drivers/usb/class/cdc-acm.c         | 139 +++++++++++++++++++++++++---
 2 files changed, 146 insertions(+), 14 deletions(-)

Comments

Jiri Slaby Aug. 25, 2023, 5:33 a.m. UTC | #1 
On 24. 08. 23, 9:18, Simon Arlott wrote:
> The times for close_delay and closing_wait are in hundredths of a
> second, not milliseconds. Fix the documentation instead of trying
> to use millisecond values (which would have to be rounded).
> 
> Signed-off-by: Simon Arlott <simon@octiron.net>
> ---
> If you'd prefer, I can fold the second part of this into my previous
> patch which shouldn't have documented it as milliseconds in the first
> place (but I copied it from the other entry).
> 
>   Documentation/ABI/testing/sysfs-tty | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
> index e04e322af568..6ee878771f51 100644
> --- a/Documentation/ABI/testing/sysfs-tty
> +++ b/Documentation/ABI/testing/sysfs-tty
> @@ -87,7 +87,8 @@ What:		/sys/class/tty/ttyS<x>/close_delay
>   Date:		October 2012
>   Contact:	Alan Cox <alan@linux.intel.com>
>   Description:
> -		 Show the closing delay time for this port in ms.
> +		 Show the closing delay time for this port in hundredths
> +		 of a second.
>   
>   		 These sysfs values expose the TIOCGSERIAL interface via
>   		 sysfs rather than via ioctls.
> @@ -96,7 +97,8 @@ What:		/sys/class/tty/ttyS<x>/closing_wait
>   Date:		October 2012
>   Contact:	Alan Cox <alan@linux.intel.com>
>   Description:
> -		 Show the close wait time for this port in ms.
> +		 Show the close wait time for this port in hundredths of
> +		 a second.
>   
>   		 These sysfs values expose the TIOCGSERIAL interface via
>   		 sysfs rather than via ioctls.

Could you send these two hunks as a separate patch? It's correct 
regardless of your other patch.

And I would use "centiseconds" instead, which is used (IMO) in these cases.

thanks,
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
index 820e412d38a8..e04e322af568 100644
--- a/Documentation/ABI/testing/sysfs-tty
+++ b/Documentation/ABI/testing/sysfs-tty
@@ -161,3 +161,24 @@  Contact:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
 Description:
 		 Allows user to detach or attach back the given device as
 		 kernel console. It shows and accepts a boolean variable.
+
+What:		/sys/class/tty/ttyACM0/close_delay
+Date:		August 2023
+Contact:	linux-usb@vger.kernel.org
+Description:
+		Set the closing delay time for this port in ms.
+
+		These sysfs values expose the TIOCGSERIAL interface via
+		sysfs rather than via ioctls.
+
+What:		/sys/class/tty/ttyACM0/closing_wait
+Date:		August 2023
+Contact:	linux-usb@vger.kernel.org
+Description:
+		Set the close wait time for this port in ms.
+
+		These sysfs values expose the TIOCGSERIAL interface via
+		sysfs rather than via ioctls.
+
+		Unlike the ioctl interface, waiting forever is represented as
+		-1 and zero is used to disable waiting on close.
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 00db9e1fc7ed..7e0f94e18445 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -953,42 +953,57 @@  static int acm_tty_tiocmset(struct tty_struct *tty,
 	return acm_set_control(acm, acm->ctrlout = newctrl);
 }
 
-static int get_serial_info(struct tty_struct *tty, struct serial_struct *ss)
+static void acm_read_serial_info(struct acm *acm, struct serial_struct *ss)
 {
-	struct acm *acm = tty->driver_data;
-
 	ss->line = acm->minor;
 	ss->close_delay	= jiffies_to_msecs(acm->port.close_delay) / 10;
 	ss->closing_wait = acm->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
 				ASYNC_CLOSING_WAIT_NONE :
 				jiffies_to_msecs(acm->port.closing_wait) / 10;
-	return 0;
 }
 
-static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
+static int acm_write_serial_info(struct acm *acm, struct serial_struct *ss,
+	bool admin)
 {
-	struct acm *acm = tty->driver_data;
 	unsigned int closing_wait, close_delay;
-	int retval = 0;
+	int ret = 0;
 
 	close_delay = msecs_to_jiffies(ss->close_delay * 10);
 	closing_wait = ss->closing_wait == ASYNC_CLOSING_WAIT_NONE ?
 			ASYNC_CLOSING_WAIT_NONE :
 			msecs_to_jiffies(ss->closing_wait * 10);
 
-	mutex_lock(&acm->port.mutex);
-
-	if (!capable(CAP_SYS_ADMIN)) {
+	if (!admin) {
 		if ((close_delay != acm->port.close_delay) ||
 		    (closing_wait != acm->port.closing_wait))
-			retval = -EPERM;
+			ret = -EPERM;
 	} else {
 		acm->port.close_delay  = close_delay;
 		acm->port.closing_wait = closing_wait;
 	}
 
+	return ret;
+}
+
+static int get_serial_info(struct tty_struct *tty, struct serial_struct *ss)
+{
+	struct acm *acm = tty->driver_data;
+
+	mutex_lock(&acm->port.mutex);
+	acm_read_serial_info(acm, ss);
+	mutex_unlock(&acm->port.mutex);
+	return 0;
+}
+
+static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
+{
+	struct acm *acm = tty->driver_data;
+	int ret = 0;
+
+	mutex_lock(&acm->port.mutex);
+	ret = acm_write_serial_info(acm, ss, capable(CAP_SYS_ADMIN));
 	mutex_unlock(&acm->port.mutex);
-	return retval;
+	return ret;
 }
 
 static int wait_serial_change(struct acm *acm, unsigned long arg)
@@ -1162,6 +1177,102 @@  static int acm_write_buffers_alloc(struct acm *acm)
 	return 0;
 }
 
+static ssize_t close_delay_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct acm *acm = dev_get_drvdata(dev);
+	struct serial_struct ss;
+
+	mutex_lock(&acm->port.mutex);
+	acm_read_serial_info(acm, &ss);
+	mutex_unlock(&acm->port.mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", ss.close_delay);
+}
+
+static ssize_t close_delay_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct acm *acm = dev_get_drvdata(dev);
+	struct serial_struct ss;
+	u16 close_delay;
+	int ret;
+
+	ret = kstrtou16(buf, 0, &close_delay);
+	if (ret)
+		return ret;
+
+	mutex_lock(&acm->port.mutex);
+	acm_read_serial_info(acm, &ss);
+	ss.close_delay = close_delay;
+	ret = acm_write_serial_info(acm, &ss, true);
+	mutex_unlock(&acm->port.mutex);
+
+	return ret ? ret : count;
+}
+
+static ssize_t closing_wait_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct acm *acm = dev_get_drvdata(dev);
+	struct serial_struct ss;
+	s32 value;
+
+	mutex_lock(&acm->port.mutex);
+	acm_read_serial_info(acm, &ss);
+	mutex_unlock(&acm->port.mutex);
+
+	if (ss.closing_wait == ASYNC_CLOSING_WAIT_NONE)
+		value = 0;
+	else if (ss.closing_wait == ASYNC_CLOSING_WAIT_INF)
+		value = -1;
+	else
+		value = ss.closing_wait;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static ssize_t closing_wait_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct acm *acm = dev_get_drvdata(dev);
+	struct serial_struct ss;
+	s32 closing_wait;
+	int ret;
+
+	ret = kstrtos32(buf, 0, &closing_wait);
+	if (ret)
+		return ret;
+
+	if (closing_wait == 0) {
+		closing_wait = ASYNC_CLOSING_WAIT_NONE;
+	} else if (closing_wait == -1) {
+		closing_wait = ASYNC_CLOSING_WAIT_INF;
+	} else if (closing_wait == ASYNC_CLOSING_WAIT_NONE
+			|| closing_wait == ASYNC_CLOSING_WAIT_INF /* redundant (0) */
+			|| closing_wait < -1 || closing_wait > U16_MAX) {
+		return -ERANGE;
+	}
+
+	mutex_lock(&acm->port.mutex);
+	acm_read_serial_info(acm, &ss);
+	ss.closing_wait = closing_wait;
+	ret = acm_write_serial_info(acm, &ss, true);
+	mutex_unlock(&acm->port.mutex);
+
+	return ret ? ret : count;
+}
+
+static DEVICE_ATTR_RW(close_delay);
+static DEVICE_ATTR_RW(closing_wait);
+
+static struct attribute *tty_dev_attrs[] = {
+	&dev_attr_close_delay.attr,
+	&dev_attr_closing_wait.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(tty_dev);
+
 static int acm_probe(struct usb_interface *intf,
 		     const struct usb_device_id *id)
 {
@@ -1503,8 +1614,8 @@  static int acm_probe(struct usb_interface *intf,
 			goto err_remove_files;
 	}
 
-	tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, minor,
-			&control_interface->dev);
+	tty_dev = tty_port_register_device_attr(&acm->port, acm_tty_driver,
+			minor, &control_interface->dev, acm, tty_dev_groups);
 	if (IS_ERR(tty_dev)) {
 		rv = PTR_ERR(tty_dev);
 		goto err_release_data_interface;