USB: cdc-acm: TIOCSSERIAL fixes

With HZ=250, the default 0.5 second value of close_delay is converted to 125 jiffies when set and is converted back to 50 centiseconds by TIOCGSERIAL as expected (not 12 cs as was claimed).

For completeness: With different default values for these parameters or with a HZ value not divisible by two, the lack of rounding when setting the default values in tty_port_init() could result in an -EPERM being returned, but this is hardly something we need to worry about.

Quoted 3 lines · Johan Hovold

With HZ=250, the default 0.5 second value of close_delay is converted to 125 jiffies when set and is converted back to 50 centiseconds by TIOCGSERIAL as expected (not 12 cs as was claimed).

It was "12" (instead of 50) because the conversion gor TIOCGSERIAL was initially broken, and that was fixed in the previous commit 633e2b2ded739a34bd0fb1d8b5b871f7e489ea29

Quoted 4 lines

For completeness: With different default values for these parameters or with a HZ value not divisible by two, the lack of rounding when setting the default values in tty_port_init() could result in an -EPERM being returned, but this is hardly something we need to worry about.

The -EPERM is harmful when a regular user wants to update other members of serial_struct without changing the close delays, e.g. ASYNC_LOW_LATENCY, which is granted to regular users.

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most serial devices is only useful for setting the close_delay and closing_wait parameters.

A non-privileged user has only ever been able to set the since long deprecated ASYNC_SPD flags and trying to change any other *supported* feature should result in -EPERM being returned. Setting the current values for any supported features should return success.

Fix the cdc-acm implementation which instead indicated that the TIOCSSERIAL ioctl was not even implemented when a non-privileged user set the current values.

Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold:

Quoted 4 lines

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most serial devices is only useful for setting the close_delay and closing_wait parameters.

Quoted 8 lines

A non-privileged user has only ever been able to set the since long deprecated ASYNC_SPD flags and trying to change any other *supported* feature should result in -EPERM being returned. Setting the current values for any supported features should return success.

Fix the cdc-acm implementation which instead indicated that the TIOCSSERIAL ioctl was not even implemented when a non-privileged user set the current values.

Hi,

the idea here was that you are setting something else, if you are not changing a parameter that can be changed. That conclusion is dubious, but at the same time, this implementation can change only these two parameters. So can the test really be dropped as opposed to be modified?

Quoted 14 lines

Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold:

Quoted 4 lines

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most serial devices is only useful for setting the close_delay and closing_wait parameters.

Quoted 8 lines

A non-privileged user has only ever been able to set the since long deprecated ASYNC_SPD flags and trying to change any other *supported* feature should result in -EPERM being returned. Setting the current values for any supported features should return success.

Fix the cdc-acm implementation which instead indicated that the TIOCSSERIAL ioctl was not even implemented when a non-privileged user set the current values.

Quoted 7 lines

Hi,

the idea here was that you are setting something else, if you are not changing a parameter that can be changed. That conclusion is dubious, but at the same time, this implementation can change only these two parameters. So can the test really be dropped as opposed to be modified?

The de-facto standard for how to handle change requests for non-supported features (e.g. changing the I/O port or IRQ) is to simply ignore them and return 0.

For most (non-legacy) serial devices the only relevant parameters are close_delay and closing_wait. And as we need to return -EPERM when a non-privileged user tries to change these, we cannot drop the test.

(And returning -EOPNOTSUPP was never correct as the ioctl is indeed supported.)

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most serial devices is only useful for setting the close_delay and closing_wait parameters.

The xmit_fifo_size parameter could be used to set the hardware transmit fifo size of a legacy UART when it could not be detected, but the interface is limited to eight bits and should be left unset when it is not used.

Similarly, baud_base could be used to set the uart base clock when it could not be detected, but might as well be left unset when it is not known.

Fix the cdc-acm TIOCGSERIAL implementation by dropping its custom interpretation of the unused xmit_fifo_size and baud_base fields, which overflowed the former with the URB buffer size and set the latter to the current line speed. Also return the port line number, which is the only other value used besides the close parameters.

Fixes 18c75720e667 ("USB: allow users to run setserial with cdc-acm")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/class/cdc-acm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/usb/class/cdc-acm.c +0 -0

index 43e31dad4831..b74713518b3a 100644

Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold:

Quoted 8 lines

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most serial devices is only useful for setting the close_delay and closing_wait parameters.

The xmit_fifo_size parameter could be used to set the hardware transmit fifo size of a legacy UART when it could not be detected, but the interface is limited to eight bits and should be left unset when it is not used.

OK.

Quoted 3 lines

Similarly, baud_base could be used to set the uart base clock when it could not be detected, but might as well be left unset when it is not known.

Well, the devices report it. It is part of the standard.

Quoted 20 lines

Fix the cdc-acm TIOCGSERIAL implementation by dropping its custom interpretation of the unused xmit_fifo_size and baud_base fields, which overflowed the former with the URB buffer size and set the latter to the current line speed. Also return the port line number, which is the only other value used besides the close parameters.

Fixes 18c75720e667 ("USB: allow users to run setserial with cdc-acm")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/class/cdc-acm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/usb/class/cdc-acm.c +0 -0

index 43e31dad4831..b74713518b3a 100644
@@ -929,8 +929,7 @@ static int get_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 {
 	struct acm *acm = tty->driver_data;
 

drivers/usb/class/cdc-acm.c static int get_serial_info(struct tty_struct *tty, struct serial_struct *ss) · line 933

Quoted 2 lines

drivers/usb/class/cdc-acm.c +0 -2

-	ss->xmit_fifo_size = acm->writesize;
-	ss->baud_base = le32_to_cpu(acm->line.dwDTERate);

If we do this, we have a value that can be set, but is not reported. That looks a bit strange to me.

Quoted 3 lines

Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold:

Well, the devices report it. It is part of the standard.

No, the standard doesn't include anything about a baud-base clock AFAICT.

Am Donnerstag, den 08.04.2021, 11:48 +0200 schrieb Johan Hovold:

Quoted 7 lines · Oliver Neukum

Quoted 3 lines

Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold:

Well, the devices report it. It is part of the standard.

No, the standard doesn't include anything about a baud-base clock AFAICT.

Unfortunately it does.
dwDTERate - chapter 6.3.11 - table 17

If we does this wrongly, we should certainly fix it, but just removing the reporting doesn't look right to me.

Quoted 10 lines

Am Donnerstag, den 08.04.2021, 11:48 +0200 schrieb Johan Hovold:

Quoted 6 lines · Oliver Neukum

Quoted 2 lines

Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold:
Well, the devices report it. It is part of the standard.

No, the standard doesn't include anything about a baud-base clock AFAICT.

Unfortunately it does.
dwDTERate - chapter 6.3.11 - table 17

That's not the base clock rate, that's just the currently configured line speed which you can read from termios

Quoted 2 lines

If we does this wrongly, we should certainly fix it, but just removing the reporting doesn't look right to me.

The driver got its interpretation of baud_base wrong, and CDC doesn't even have a concept of base clock rate so removing it is the right thing to do.

Again, baud_base is really only relevant with legacy UARTs and when using the deprecated ASYNC_SPD_CUST.

And if the user wants to knows the current line speed we have a different interface for that.

Am Donnerstag, den 08.04.2021, 13:54 +0200 schrieb Johan Hovold:

Quoted 21 lines · Oliver Neukum

Quoted 10 lines

Am Donnerstag, den 08.04.2021, 11:48 +0200 schrieb Johan Hovold:

Quoted 6 lines · Oliver Neukum

Quoted 2 lines

Am Mittwoch, den 07.04.2021, 12:28 +0200 schrieb Johan Hovold:
Well, the devices report it. It is part of the standard.

No, the standard doesn't include anything about a baud-base clock AFAICT.

Unfortunately it does.
dwDTERate - chapter 6.3.11 - table 17

That's not the base clock rate, that's just the currently configured line speed which you can read from termios

Quoted 2 lines

If we does this wrongly, we should certainly fix it, but just removing the reporting doesn't look right to me.

The driver got its interpretation of baud_base wrong, and CDC doesn't even have a concept of base clock rate so removing it is the right thing to do.

Quoted 5 lines

Again, baud_base is really only relevant with legacy UARTs and when using the deprecated ASYNC_SPD_CUST.

And if the user wants to knows the current line speed we have a different interface for that.

Hi,

thank you, that clarifies things. I am happy with the patch itself, but could I ask you to do two things:

1. Edit the commit description
making clear that the difference
between the base clock rate and the line speed.

2. Mark the patch specially to NOT be included in stable. We may have
users misusing the current API.