USB: serial: ark3116: ioctl changes

PATCH In review Mikhail Zaytsev 3 participants 8 messages Jan 6, 2018 – Jan 9, 2018

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Mikhail Zaytsev

author

2018-01-06 17:22 UTC #0 lore raw

v2 cover USB: serial: ark3116: ioctl changes

The patch removes unused TIOCSSERIAL case from ioctl. TIOCGSERIAL case moves to the get_serial_info() function.

Mikhail Zaytsev (2):
USB: serial: ark3116: Remove unused TIOCSSERIAL ioctl case.
USB: serial: ark3116: Move TIOCGSERIAL ioctl case to function.

 drivers/usb/serial/ark3116.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

Signature · 1 line

2.11.0

Mikhail Zaytsev

author

2018-01-06 17:14 UTC #1 lore raw

PATCH v2 1/2 USB: serial: ark3116: Remove unused TIOCSSERIAL ioctl case.

The patch removes unused TIOCSSERIAL ioctl case and adds the default block to the switch. This will make the ioctl return -ENOTTY to user space (e.g. setserial), because TIOCSSERIAL really isn't supported for these devices currently.

Signed-off-by Mikhail Zaytsev <[email protected]>

---
 drivers/usb/serial/ark3116.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/usb/serial/ark3116.c +2 -4

index 23d46ef87..2e957c76f 100644
@@ -418,10 +418,8 @@ static int ark3116_ioctl(struct tty_struct *tty,
 			return -EFAULT;
 
 		return 0;
-	case TIOCSSERIAL:
-		if (copy_from_user(&serstruct, user_arg, sizeof(serstruct)))
-			return -EFAULT;
-		return 0;
+	default:
+		break;
 	}
 
 	return -ENOIOCTLCMD;

Oliver Neukum

2018-01-08 10:33 UTC #2 lore raw

Re: [PATCH v2 1/2] USB: serial: ark3116: Remove unused TIOCSSERIAL ioctl case.

Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:

Quoted 4 lines

The patch removes unused TIOCSSERIAL ioctl case and adds the default block to the switch. This will make the ioctl return -ENOTTY to user space (e.g. setserial), because TIOCSSERIAL really isn't supported for these devices currently.

Hi,

this will break software that is now running on these devices, won't it? Do you know why those devices basically ignore the ioctl?

Regards
Oliver
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johan Hovold

maintainer

2018-01-08 15:28 UTC #3 lore raw

Re: [PATCH v2 1/2] USB: serial: ark3116: Remove unused TIOCSSERIAL ioctl case.

Quoted 11 lines · Oliver Neukum

Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:
Quoted 4 lines
The patch removes unused TIOCSSERIAL ioctl case and adds the default block to the switch. This will make the ioctl return -ENOTTY to user space (e.g. setserial), because TIOCSSERIAL really isn't supported for these devices currently.
Hi,
this will break software that is now running on these devices, won't it? Do you know why those devices basically ignore the ioctl?

Yeah, that was my initial reactions as well, but then again, any sane user space cannot rely on these ioctl being implemented for all tty devices.

I did some digging now and these (dummy) ioctl implementations where added by commit 2f430b4bbae7 ("USB: ark3116: Add TIOCGSERIAL and TIOCSSERIAL ioctl calls.") back in 2006. This in turn appears to have been triggered by a change in a user space tool, wvdial, which started erroring out if either was missing.

I found a couple of bug reports about that through google, and looking at the wvstreams (library) code now, it looks like the issue has indeed been resolved by handling errors more gracefully (e.g. just logging them).

So I'm willing to give this a try, and if anyone complains later we add back (or implement) TIOCSSERIAL.

Sounds reasonable?

Thanks,
Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mikhail Zaytsev

author

2018-01-08 21:45 UTC #4 lore raw

Re: [PATCH v2 1/2] USB: serial: ark3116: Remove unused TIOCSSERIAL ioctl case.

Quoted 23 lines · On Mon, 8 Jan 2018 16:28:58 +0100 Johan Hovold <[email protected]> wrote:

Quoted 11 lines · Oliver Neukum
Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:
Quoted 4 lines
The patch removes unused TIOCSSERIAL ioctl case and adds the default block to the switch. This will make the ioctl return -ENOTTY to user space (e.g. setserial), because TIOCSSERIAL really isn't supported for these devices currently.
Hi,
this will break software that is now running on these devices, won't it? Do you know why those devices basically ignore the ioctl?
Yeah, that was my initial reactions as well, but then again, any sane user space cannot rely on these ioctl being implemented for all tty devices.
I did some digging now and these (dummy) ioctl implementations where added by commit 2f430b4bbae7 ("USB: ark3116: Add TIOCGSERIAL and TIOCSSERIAL ioctl calls.") back in 2006. This in turn appears to have been triggered by a change in a user space tool, wvdial, which started erroring out if either was missing.

Quoted 8 lines

I found a couple of bug reports about that through google, and looking at the wvstreams (library) code now, it looks like the issue has indeed been resolved by handling errors more gracefully (e.g. just logging them).
So I'm willing to give this a try, and if anyone complains later we add back (or implement) TIOCSSERIAL.

Thanks Johan. I looked the commit 2f430b4bbae7. Author just did a cut'n'paste
from other USB serial drivers. I think that it would be better remove
the TIOCGSERIAL implementation too.

Mikhail
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johan Hovold

maintainer

2018-01-09 11:55 UTC #5 lore raw

Re: [PATCH v2 1/2] USB: serial: ark3116: Remove unused TIOCSSERIAL ioctl case.

Quoted 30 lines · Mikhail Zaytsev

Quoted 28 lines · On Mon, 8 Jan 2018 16:28:58 +0100 Johan Hovold <[email protected]> wrote:
Quoted 11 lines · Oliver Neukum
Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:
Quoted 4 lines
The patch removes unused TIOCSSERIAL ioctl case and adds the default block to the switch. This will make the ioctl return -ENOTTY to user space (e.g. setserial), because TIOCSSERIAL really isn't supported for these devices currently.
Hi,
this will break software that is now running on these devices, won't it? Do you know why those devices basically ignore the ioctl?
Yeah, that was my initial reactions as well, but then again, any sane user space cannot rely on these ioctl being implemented for all tty devices.
I did some digging now and these (dummy) ioctl implementations where added by commit 2f430b4bbae7 ("USB: ark3116: Add TIOCGSERIAL and TIOCSSERIAL ioctl calls.") back in 2006. This in turn appears to have been triggered by a change in a user space tool, wvdial, which started erroring out if either was missing.
I found a couple of bug reports about that through google, and looking at the wvstreams (library) code now, it looks like the issue has indeed been resolved by handling errors more gracefully (e.g. just logging them).

Quoted 7 lines

Quoted 3 lines
So I'm willing to give this a try, and if anyone complains later we add back (or implement) TIOCSSERIAL.
Thanks Johan. I looked the commit 2f430b4bbae7. Author just did a cut'n'paste
from other USB serial drivers. I think that it would be better remove
the TIOCGSERIAL implementation too.

I've applied this one now after adding some of the backstory from above to the commit message.

Thanks,
Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mikhail Zaytsev

author

2018-01-06 17:15 UTC #6 lore raw

PATCH v2 2/2 USB: serial: ark3116: Move TIOCGSERIAL ioctl case to function.

In reply to Mikhail Zaytsev

The patch moves TIOCGSERIAL ioctl case to get_serial_info function.

Signed-off-by Mikhail Zaytsev <[email protected]>

---
 drivers/usb/serial/ark3116.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

drivers/usb/serial/ark3116.c +20 ~~-14~~

index 2e957c76f..7683c76a1 100644
@@ -397,27 +397,33 @@ static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port)
 	return result;
 }
 
+static int ark3116_get_serial_info(struct usb_serial_port *port,
+			struct serial_struct __user *retinfo)
+{
+	struct serial_struct tmp;
+
+	memset(&tmp, 0, sizeof(tmp));
+
+	tmp.type = PORT_16654;
+	tmp.line = port->minor;
+	tmp.port = port->port_number;
+	tmp.baud_base = 460800;
+
+	if (copy_to_user(retinfo, &tmp, sizeof(tmp)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int ark3116_ioctl(struct tty_struct *tty,
 			 unsigned int cmd, unsigned long arg)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct serial_struct serstruct;
-	void __user *user_arg = (void __user *)arg;
 
 	switch (cmd) {
 	case TIOCGSERIAL:
-		/* XXX: Some of these values are probably wrong. */
-		memset(&serstruct, 0, sizeof(serstruct));
-		serstruct.type = PORT_16654;
-		serstruct.line = port->minor;
-		serstruct.port = port->port_number;
-		serstruct.custom_divisor = 0;
-		serstruct.baud_base = 460800;
-
-		if (copy_to_user(user_arg, &serstruct, sizeof(serstruct)))
-			return -EFAULT;
-
-		return 0;
+		return ark3116_get_serial_info(port,
+					(struct serial_struct __user *)arg);
 	default:
 		break;
 	}

Johan Hovold

maintainer

2018-01-09 11:57 UTC #7 lore raw

Re: [PATCH v2 2/2] USB: serial: ark3116: Move TIOCGSERIAL ioctl case to function.

drivers/usb/serial/ark3116.c static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port) · line 405

Quoted 10 lines · Mikhail Zaytsev

The patch moves TIOCGSERIAL ioctl case to get_serial_info function.
Signed-off-by Mikhail Zaytsev <[email protected]>
static int ark3116_ioctl(struct tty_struct *tty,
drivers/usb/serial/ark3116.c +0 -2
 			 unsigned int cmd, unsigned long arg)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct serial_struct serstruct;
-	void __user *user_arg = (void __user *)arg;

I prefer keeping this pointer here to avoid adding casts to every helper function call, so I added it back before applying.

drivers/usb/serial/ark3116.c static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port) · line 426

Quoted 6 lines

drivers/usb/serial/ark3116.c +2 -0

 	switch (cmd) {
 	case TIOCGSERIAL:
 
+		return ark3116_get_serial_info(port,
+					(struct serial_struct __user *)arg);

Thanks,
Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html