[PATCH v2 0/2] USB: serial: ark3116: ioctl changes

[PATCH v2 0/2] USB: serial: ark3116: ioctl changes

[Mikhail Zaytsev]

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

-- 
2.11.0

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

[Mikhail Zaytsev]

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 <flashed@mail.ru>
---
 drivers/usb/serial/ark3116.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
index 23d46ef87..2e957c76f 100644
--- a/drivers/usb/serial/ark3116.c
+++ b/drivers/usb/serial/ark3116.c
@@ -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;
-- 
2.11.0

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

[Oliver Neukum]

Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:
> 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

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

[Johan Hovold]

On Mon, Jan 08, 2018 at 11:33:32AM +0100, Oliver Neukum wrote:
> Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:
> > 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

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

[Mikhail Zaytsev]

On Mon, 8 Jan 2018 16:28:58 +0100 Johan Hovold <johan@kernel.org> wrote:

> On Mon, Jan 08, 2018 at 11:33:32AM +0100, Oliver Neukum wrote:
> > Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:  
> > > 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.
> 

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

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

[Johan Hovold]

On Tue, Jan 09, 2018 at 12:45:30AM +0300, Mikhail Zaytsev wrote:
> On Mon, 8 Jan 2018 16:28:58 +0100 Johan Hovold <johan@kernel.org> wrote:
> 
> > On Mon, Jan 08, 2018 at 11:33:32AM +0100, Oliver Neukum wrote:
> > > Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:  
> > > > 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.
> > 
> 
> 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

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

[Mikhail Zaytsev]

The patch moves TIOCGSERIAL ioctl case to get_serial_info function.

Signed-off-by: Mikhail Zaytsev <flashed@mail.ru>
---
 drivers/usb/serial/ark3116.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
index 2e957c76f..7683c76a1 100644
--- a/drivers/usb/serial/ark3116.c
+++ b/drivers/usb/serial/ark3116.c
@@ -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;
 	}
-- 
2.11.0

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

[Johan Hovold]

On Sat, Jan 06, 2018 at 08:15:22PM +0300, Mikhail Zaytsev wrote:
> The patch moves TIOCGSERIAL ioctl case to get_serial_info function.
> 
> Signed-off-by: Mikhail Zaytsev <flashed@mail.ru>

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

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

>  
>  	switch (cmd) {
>  	case TIOCGSERIAL:

> +		return ark3116_get_serial_info(port,
> +					(struct serial_struct __user *)arg);

Thanks,
Johan