| Message ID | ea1a13ad-a1e0-540a-e97a-4c44f6d2d33b@0882a8b5-c6c3-11e9-b005-00805fc181fe.uuid.home.arpa |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: cdc-acm: expose serial close_delay and closing_wait in sysfs | expand |
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>
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
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.
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
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
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
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,
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.
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;
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(-)