[PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[Simon Arlott]

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>
---
 Documentation/ABI/testing/sysfs-tty |  21 +++++
 drivers/usb/class/cdc-acm.c         | 135 +++++++++++++++++++++++++---
 2 files changed, 144 insertions(+), 12 deletions(-)

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..07e805df5113 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -953,21 +953,18 @@ 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;
 
@@ -976,9 +973,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 			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;
@@ -987,8 +982,28 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 		acm->port.closing_wait = closing_wait;
 	}
 
+	return 0;
+}
+
+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 retval;
+	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 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;
-- 
2.37.0

-- 
Simon Arlott

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

[Simon Arlott]

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

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;
-- 
2.37.0

-- 
Simon Arlott

[PATCH] docs: ABI: sysfs-tty: close times are in hundredths of a second

[Simon Arlott]

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.
@@ -166,7 +168,8 @@ 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.
+		Set 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.
@@ -175,7 +178,8 @@ 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.
+		Set 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.
-- 
2.37.0

-- 
Simon Arlott

Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[Oliver Neukum]

On 23.08.23 22:37, Simon Arlott wrote:

Hi,

I am terribly sorry, but this has to be a hard NO.
This patch has an unsurmountable problem.
Reasons below.
  
> 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

If the tty core does not export them as writable it presumably has reasons
for that. We cannot circumvent those reasons in a particular driver unless
there are very special circumstances that make the driver a special case.

The correct way to deal with this issue would be a proposal to change
the generic serial code. Even there I am sceptical because we would carry
the code duplication forever. ioctl() is the way you set the parameters
for a serial port in a Unix system. We have tools for that. Adding a second method
is problematic.
But that is not for me to decide. As far as CDC-ACM, however, is concerned,
I must reject this patch, because it fundamentally does something that should
not be done, definitely not at this layer.

	Sorry
		Oliver
  
> Signed-off-by: Simon Arlott <simon@octiron.net>
Nacked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[Greg Kroah-Hartman]

On Wed, Aug 23, 2023 at 09:37:45PM +0100, Simon Arlott wrote:
> 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.

Then why not use that?

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

Adding tty-driver-specific sysfs files for tty devices is a big no-no,
sorry.  We don't want to go down that rabbit hole at all.

If any apis are needed, let's make them for all tty devices, through the
existing ioctl api, so they work for all devices and userspace doesn't
have to try to figure out just exactly what type of tty/serial device it
is talking to (as that will not scale and is exactly the opposite of
what common apis are for.)

sorry, we can't take this, and in the end, you don't want us to as it's
not maintainable.

thanks,

greg k-h

Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[Simon Arlott]

On 24/08/2023 15:48, Greg Kroah-Hartman wrote:
> On Wed, Aug 23, 2023 at 09:37:45PM +0100, Simon Arlott wrote:
>> 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.
> 
> Then why not use that?

It's not practical to use TIOCGSERIAL from a udev rule. Instead of a
sysfs attribute (that udev has built-in support for writing) it would
require a separate compiled process or other non-trivial dependencies
(e.g. Python) to modify the closing_wait value. There's no shell script
support for read-modify-write of a complex ioctl struct.

The ioctl can't be used without opening and closing the tty, which has
side effects. On open() it'll raise DTR/RTS and activate it. For cdc-acm
that will indicate to the device that the serial port has been opened
which will be visible to the software running on the USB device. On
close() it'll be delayed by the close_delay if any process is currently
doing a blocking open() and there's no carrier, then the closing_wait
time if there's been any incomplete transmitted data (by any process).

I want to be able to automatically set close_delay to 0 and closing_wait
to 65535 ("no waiting") on all USB serial devices without these kind of
side effects. I'm sure these have a purpose when connected to a real tty
but forcing a process to wait 30 seconds before it can close the port
(or exit) if the USB device isn't reading data properly is inconvenient.

Those two values require CAP_SYS_ADMIN to modify (which is separately
enforced by many of the tty drivers) so user applications can't change
them even if they're aware of them.

> If any apis are needed, let's make them for all tty devices, through the
> existing ioctl api, so they work for all devices and userspace doesn't
> have to try to figure out just exactly what type of tty/serial device it
> is talking to (as that will not scale and is exactly the opposite of
> what common apis are for.)

Are you ok with adding the same two attributes to sysfs for all ttys?

I'd use a different name (appending "_centisecs") because serial_core
already uses those names on the tty device and I think it's better to
define "infinite wait" and "no wait" as -1 and 0 instead of 0 and 65535.

-- 
Simon Arlott

Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[Greg Kroah-Hartman]

On Thu, Aug 24, 2023 at 07:02:40PM +0100, Simon Arlott wrote:
> On 24/08/2023 15:48, Greg Kroah-Hartman wrote:
> > On Wed, Aug 23, 2023 at 09:37:45PM +0100, Simon Arlott wrote:
> >> 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.
> > 
> > Then why not use that?
> 
> It's not practical to use TIOCGSERIAL from a udev rule. Instead of a
> sysfs attribute (that udev has built-in support for writing) it would
> require a separate compiled process or other non-trivial dependencies
> (e.g. Python) to modify the closing_wait value. There's no shell script
> support for read-modify-write of a complex ioctl struct.

It's this way for all serial ports, cdc-acm is not "special" here,
sorry.

> The ioctl can't be used without opening and closing the tty, which has
> side effects. On open() it'll raise DTR/RTS and activate it. For cdc-acm
> that will indicate to the device that the serial port has been opened
> which will be visible to the software running on the USB device. On
> close() it'll be delayed by the close_delay if any process is currently
> doing a blocking open() and there's no carrier, then the closing_wait
> time if there's been any incomplete transmitted data (by any process).
> 
> I want to be able to automatically set close_delay to 0 and closing_wait
> to 65535 ("no waiting") on all USB serial devices without these kind of
> side effects. I'm sure these have a purpose when connected to a real tty
> but forcing a process to wait 30 seconds before it can close the port
> (or exit) if the USB device isn't reading data properly is inconvenient.
> 
> Those two values require CAP_SYS_ADMIN to modify (which is separately
> enforced by many of the tty drivers) so user applications can't change
> them even if they're aware of them.

So this looks like you feel there should be a way to modify serial port
values without the ioctl api.  That's good, maybe we do need to do this,
but then, if so, it needs to happen for all serial ports, not just one
single device type.

Note that the tty api is really really old, so modifications and
enhancements need to be done carefully.  And be sure that there isn't
already another way to do this.  The open/close DTR/RTS issue has been
brought up many times, and I thought that there was ways to prevent it
already, are you sure that modern tools don't already take this into
consideration?

Or better yet, don't make any change until you actually open the port
for access.  Why wory about these values until you need to use it?  Why
insist on doing it from a udev rule when there is no real user of the
port yet?  Who are you setting the port up for, and why can't they do it
when they open and set the other values that they want?

> > If any apis are needed, let's make them for all tty devices, through the
> > existing ioctl api, so they work for all devices and userspace doesn't
> > have to try to figure out just exactly what type of tty/serial device it
> > is talking to (as that will not scale and is exactly the opposite of
> > what common apis are for.)
> 
> Are you ok with adding the same two attributes to sysfs for all ttys?

Why just these attributes, why not tty settings?  :)

> I'd use a different name (appending "_centisecs") because serial_core
> already uses those names on the tty device and I think it's better to
> define "infinite wait" and "no wait" as -1 and 0 instead of 0 and 65535.

Ah, so this already is an api?  Or is it a different one?

confused,

greg k-h

Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[Oliver Neukum]

On 24.08.23 20:02, Simon Arlott wrote:
  
> It's not practical to use TIOCGSERIAL from a udev rule. Instead of a
> sysfs attribute (that udev has built-in support for writing) it would
> require a separate compiled process or other non-trivial dependencies
> (e.g. Python) to modify the closing_wait value. There's no shell script
> support for read-modify-write of a complex ioctl struct.

That, however, is a deficiency of udev.

> The ioctl can't be used without opening and closing the tty, which has
> side effects. On open() it'll raise DTR/RTS and activate it. For cdc-acm
> that will indicate to the device that the serial port has been opened
> which will be visible to the software running on the USB device. On
> close() it'll be delayed by the close_delay if any process is currently
> doing a blocking open() and there's no carrier, then the closing_wait
> time if there's been any incomplete transmitted data (by any process).

And that is an issue of the generic serial layer.
  
> Those two values require CAP_SYS_ADMIN to modify (which is separately
> enforced by many of the tty drivers) so user applications can't change
> them even if they're aware of them.

That is even more damning. Either something is protected by a capability
or it is not. Such a protection must not be circumvented.

	Regards
		Oliver


  

Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[Alan Stern]

On Fri, Aug 25, 2023 at 01:46:15AM +0200, Oliver Neukum wrote:
> 
> 
> On 24.08.23 20:02, Simon Arlott wrote:
> > The ioctl can't be used without opening and closing the tty, which has
> > side effects. On open() it'll raise DTR/RTS and activate it. For cdc-acm
> > that will indicate to the device that the serial port has been opened
> > which will be visible to the software running on the USB device. On
> > close() it'll be delayed by the close_delay if any process is currently
> > doing a blocking open() and there's no carrier, then the closing_wait
> > time if there's been any incomplete transmitted data (by any process).
> 
> And that is an issue of the generic serial layer.

Is it feasible to add a sysfs attribute for ttys or the serial layer to 
control the side effect of opening (avoid raising DTR/RTS)?  If that 
could be done, a program could use the existing ioctl to set close_delay 
and closing_wait to 0 with no penalties.

This would be racy, but for the purposes of udev that shouldn't matter 
much.

Alan Stern

Re: [PATCH] docs: ABI: sysfs-tty: close times are in hundredths of a second

[Jiri Slaby]

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,
-- 
js
suse labs

Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing_wait in sysfs

[Simon Arlott]

On 24/08/2023 20:22, Greg Kroah-Hartman wrote:
> So this looks like you feel there should be a way to modify serial port
> values without the ioctl api.  That's good, maybe we do need to do this,
> but then, if so, it needs to happen for all serial ports, not just one
> single device type.
> 
> Note that the tty api is really really old, so modifications and
> enhancements need to be done carefully.  And be sure that there isn't
> already another way to do this.  The open/close DTR/RTS issue has been
> brought up many times, and I thought that there was ways to prevent it
> already, are you sure that modern tools don't already take this into
> consideration?

On 25/08/2023 02:53, Alan Stern wrote:
> Is it feasible to add a sysfs attribute for ttys or the serial layer to 
> control the side effect of opening (avoid raising DTR/RTS)?  If that 
> could be done, a program could use the existing ioctl to set close_delay 
> and closing_wait to 0 with no penalties.

The port will still be "activated". That has side effects for an
application running on a microcontroller providing the USB CDC ACM
interface because it may be waiting for that state change to output a
message or start doing something.

> Or better yet, don't make any change until you actually open the port
> for access.  Why wory about these values until you need to use it?  Why
> insist on doing it from a udev rule when there is no real user of the

Because the applications that access the serial port aren't typically
aware that close() may block for 30 seconds. Even if they are, they
can't do much about it because of the next problem.

> port yet?  Who are you setting the port up for, and why can't they do it
> when they open and set the other values that they want?

Because they're not running as root and so don't have CAP_SYS_ADMIN and
can't change these two values.

>>> If any apis are needed, let's make them for all tty devices, through the
>>> existing ioctl api, so they work for all devices and userspace doesn't
>>> have to try to figure out just exactly what type of tty/serial device it
>>> is talking to (as that will not scale and is exactly the opposite of
>>> what common apis are for.)
>>
>> Are you ok with adding the same two attributes to sysfs for all ttys?
> 
> Why just these attributes, why not tty settings?  :)

I assume you mean all tty settings? Looking at termios(3) there are a
lot of them...

Restricting them to just the serial settings (include/uapi/linux/serial.h
serial_struct), some of them only apply to real 16550-like serial ports
("type") and won't be applicable everywhere ("irq").

There would then be several serial devices with attributes that don't do
anything, e.g. "irq" will read as 0 and writes will do nothing. We
wouldn't know at the tty layer which writes will do nothing because
there's only one operation for the whole serial_struct.

The "close_delay" and "closing_wait" values have universal application
because the tty layer uses them. They're set on the tty_port in
tty_port_init() and then the serial port drivers modify them when
TIOCSSERIAL is used. There doesn't appear to be a tty-level API to
modify them.

>> I'd use a different name (appending "_centisecs") because serial_core
>> already uses those names on the tty device and I think it's better to
>> define "infinite wait" and "no wait" as -1 and 0 instead of 0 and 65535.
> 
> Ah, so this already is an api?  Or is it a different one?

The serial_core adds read-only attributes to the tty device for most of
the TIOCGSERIAL values. That includes the tty closing options but
they're exact mirrors of the ioctl API which means it has special values
selected by the range of a u16 value.

A new sysfs attribute should use better special values.

-- 
Simon Arlott

[PATCH (v2)] docs: ABI: sysfs-tty: close times are in centiseconds

[Simon Arlott]

The times for close_delay and closing_wait are in centiseconds, not
milliseconds. Fix the documentation and add details of special values.

Signed-off-by: Simon Arlott <simon@octiron.net>
---
On 25/08/2023 06:33, Jiri Slaby wrote:
> And I would use "centiseconds" instead, which is used (IMO) in these cases.

It's used in a few places, but the documentation has no "centiseconds"
and a couple of "hundredths". I've changed it anyway.

 Documentation/ABI/testing/sysfs-tty | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
index 820e412d38a8..895c47f05f6f 100644
--- a/Documentation/ABI/testing/sysfs-tty
+++ b/Documentation/ABI/testing/sysfs-tty
@@ -87,19 +87,22 @@ 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 centiseconds.
 
-		 These sysfs values expose the TIOCGSERIAL interface via
-		 sysfs rather than via ioctls.
+		These sysfs values expose the TIOCGSERIAL interface via
+		sysfs rather than via ioctls.
 
 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 centiseconds.
 
-		 These sysfs values expose the TIOCGSERIAL interface via
-		 sysfs rather than via ioctls.
+		Waiting forever is represented as 0. If waiting on close is
+		disabled then the value is 65535.
+
+		These sysfs values expose the TIOCGSERIAL interface via
+		sysfs rather than via ioctls.
 
 What:		/sys/class/tty/ttyS<x>/custom_divisor
 Date:		October 2012
-- 
2.37.0

-- 
Simon Arlott