[PATCH] USB: kobil_sct: Remove unused transfer buffer allocs

Post by Peter Hurley
Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
"USB: kobil_sct: fix control requests without data stage", removed
the bogus data buffer arguments, but still allocate transfer
buffers which are not used.
---
drivers/usb/serial/kobil_sct.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
index 078f9ed..3d2bd65 100644
--- a/drivers/usb/serial/kobil_sct.c
+++ b/drivers/usb/serial/kobil_sct.c
@@ -414,8 +414,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
int result;
int dtr = 0;
int rts = 0;
- unsigned char *transfer_buffer;
- int transfer_buffer_length = 8;
/* FIXME: locking ? */
priv = usb_get_serial_port_data(port);
@@ -425,11 +423,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
return -EINVAL;
}
- /* allocate memory for transfer buffer */
- transfer_buffer = kzalloc(transfer_buffer_length, GFP_KERNEL);
- if (!transfer_buffer)
- return -ENOMEM;
-
if (set & TIOCM_RTS)
rts = 1;
if (set & TIOCM_DTR)
@@ -469,7 +462,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
KOBIL_TIMEOUT);
}
dev_dbg(dev, "%s - Send set_status_line URB returns: %i\n", __func__, result);
- kfree(transfer_buffer);
return (result < 0) ? result : 0;
}
@@ -530,8 +522,6 @@ static int kobil_ioctl(struct tty_struct *tty,
{
struct usb_serial_port *port = tty->driver_data;
struct kobil_private *priv = usb_get_serial_port_data(port);
- unsigned char *transfer_buffer;
- int transfer_buffer_length = 8;
int result;
if (priv->device_type == KOBIL_USBTWIN_PRODUCT_ID ||
@@ -541,10 +531,6 @@ static int kobil_ioctl(struct tty_struct *tty,
switch (cmd) {
- transfer_buffer = kmalloc(transfer_buffer_length, GFP_KERNEL);
- if (!transfer_buffer)
- return -ENOBUFS;
-
result = usb_control_msg(port->serial->dev,
usb_sndctrlpipe(port->serial->dev, 0),
SUSBCRequest_Misc,
@@ -559,7 +545,6 @@ static int kobil_ioctl(struct tty_struct *tty,
dev_dbg(&port->dev,
"%s - Send reset_all_queues (FLUSH) URB returns: %i\n",
__func__, result);
- kfree(transfer_buffer);
return (result < 0) ? -EIO: 0;

^^^
Returning 0 is almost certainly wrong; no further processing for
TCFLSH is performed.

Only this driver returns 0 (of all the tty drivers in mainline).

Returning -ENOIOCTLCMD allows further processing to continue;
especially the line discipline's input flushing, if TCIFLUSH/TCIOFLUSH.

Is it trying to avoid the tty_driver_flush_buffer() because it doesn't
have an output buffer?

Post by Peter Hurley
return -ENOIOCTLCMD;

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johan Hovold

2014-10-19 17:12:52 UTC

[ +CC: Jiri, Alan, linux-serial ]

^^^
Returning 0 is almost certainly wrong; no further processing for
TCFLSH is performed.

Indeed.

Post by Peter Hurley
Only this driver returns 0 (of all the tty drivers in mainline).
Returning -ENOIOCTLCMD allows further processing to continue;
especially the line discipline's input flushing, if TCIFLUSH/TCIOFLUSH.

That doesn't seem like a very good idea, and only two *staging* drivers
try to play such games (i.e. pretending not to implement the ioctl) as
far as I can see.

The only non-staging tty driver which appears to implement TCFLSH,
ipwireless, calls tty_perform_flush directly to flush the ldisc buffers.
That doesn't seem right either.

Shouldn't this be fixed by removing TCFLSH from these tty drivers'
ioctl callbacks and implementing flush_buffer()?

The staging drivers also flush a device input buffer, which could be
done in a new callback if at all needed.

Post by Peter Hurley
Is it trying to avoid the tty_driver_flush_buffer() because it doesn't
have an output buffer?

I don't think so. The author probably just assumed returning 0 for a
"handled" ioctl was the right thing to do.

I'll add flush_buffer support to usb-serial and fix up kobil_sct
meanwhile.

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

Peter Hurley

2014-10-22 11:40:20 UTC

Post by Johan Hovold
[ +CC: Jiri, Alan, linux-serial ]

^^^
Returning 0 is almost certainly wrong; no further processing for
TCFLSH is performed.

Indeed.

That doesn't seem like a very good idea, and only two *staging* drivers
try to play such games (i.e. pretending not to implement the ioctl) as
far as I can see.

Well, returning EIONOCTLCMD is the standard method of ioctl passthrough
from driver to line discipline.

Since driver 'input buffer' flushing is not currently supported by the
core, this seems the only available workaround.

Post by Johan Hovold
The only non-staging tty driver which appears to implement TCFLSH,
ipwireless, calls tty_perform_flush directly to flush the ldisc buffers.
That doesn't seem right either.

I'm not sure why ipwireless does this; I can only guess that it's a
workaround for some line discipline that doesn't use n_tty_ioctl_helper().

Post by Johan Hovold
Shouldn't this be fixed by removing TCFLSH from these tty drivers'
ioctl callbacks and implementing flush_buffer()?
The staging drivers also flush a device input buffer, which could be
done in a new callback if at all needed.

Yeah, that's why the Digi staging drivers are trapping TCFLSH; so they
can clear input buffers on TCIFLUSH/TCIOFLUSH.

I'd like to better understand the hardware and driver before extending
the core interface; this driver may not even run.

For example, this driver clears its 'input buffer' for
tcsetattr(TCSADRAIN or TCSAFLUSH). But that doesn't make sense considering
that the flip buffers could have data in them that isn't flushed; the tty
core doesn't dump the flip buffers because 'input processing' has not
happened on that data.

I think when/if these drivers are promoted is when/if the core interface
should address this. Just my opinion, though :)

Post by Peter Hurley
Is it trying to avoid the tty_driver_flush_buffer() because it doesn't
have an output buffer?

I don't think so. The author probably just assumed returning 0 for a
"handled" ioctl was the right thing to do.
I'll add flush_buffer support to usb-serial and fix up kobil_sct
meanwhile.

Thanks.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johan Hovold

2014-10-19 15:37:54 UTC

The second buffer has actually been allocated but unused since pre-git
times. Should have removed both nonetheless.

Thanks for fixing this, I'll queue it up once rc1 is out.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johan Hovold

2014-10-22 08:20:34 UTC

Post by Peter Hurley
Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
"USB: kobil_sct: fix control requests without data stage", removed

checkpatch.pl complains on your commit-reference style so you know.

Post by Peter Hurley
the bogus data buffer arguments, but still allocate transfer
buffers which are not used.

Applied now.

Thanks,
Johan

Post by Peter Hurley
---
drivers/usb/serial/kobil_sct.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
index 078f9ed..3d2bd65 100644
--- a/drivers/usb/serial/kobil_sct.c
+++ b/drivers/usb/serial/kobil_sct.c
@@ -414,8 +414,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
int result;
int dtr = 0;
int rts = 0;
- unsigned char *transfer_buffer;
- int transfer_buffer_length = 8;
/* FIXME: locking ? */
priv = usb_get_serial_port_data(port);
@@ -425,11 +423,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
return -EINVAL;
}
- /* allocate memory for transfer buffer */
- transfer_buffer = kzalloc(transfer_buffer_length, GFP_KERNEL);
- if (!transfer_buffer)
- return -ENOMEM;
-
if (set & TIOCM_RTS)
rts = 1;
if (set & TIOCM_DTR)
@@ -469,7 +462,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
KOBIL_TIMEOUT);
}
dev_dbg(dev, "%s - Send set_status_line URB returns: %i\n", __func__, result);
- kfree(transfer_buffer);
return (result < 0) ? result : 0;
}
@@ -530,8 +522,6 @@ static int kobil_ioctl(struct tty_struct *tty,
{
struct usb_serial_port *port = tty->driver_data;
struct kobil_private *priv = usb_get_serial_port_data(port);
- unsigned char *transfer_buffer;
- int transfer_buffer_length = 8;
int result;
if (priv->device_type == KOBIL_USBTWIN_PRODUCT_ID ||
@@ -541,10 +531,6 @@ static int kobil_ioctl(struct tty_struct *tty,
switch (cmd) {
- transfer_buffer = kmalloc(transfer_buffer_length, GFP_KERNEL);
- if (!transfer_buffer)
- return -ENOBUFS;
-
result = usb_control_msg(port->serial->dev,
usb_sndctrlpipe(port->serial->dev, 0),
SUSBCRequest_Misc,
@@ -559,7 +545,6 @@ static int kobil_ioctl(struct tty_struct *tty,
dev_dbg(&port->dev,
"%s - Send reset_all_queues (FLUSH) URB returns: %i\n",
__func__, result);
- kfree(transfer_buffer);
return (result < 0) ? -EIO: 0;
return -ENOIOCTLCMD;

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Peter Hurley

2014-10-22 10:26:42 UTC

Post by Peter Hurley
Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
"USB: kobil_sct: fix control requests without data stage", removed

checkpatch.pl complains on your commit-reference style so you know.

Thanks for letting me know.

Joe,

Why is checkpatch complaining about the commit reference above?

$ ./scripts/checkpatch.pl ../patches/fixes-3.18/0001-USB-kobil_sct-Remove-unused-transfer-buffer-allocs.patch
ERROR: Please use 12 or more chars for the git commit ID like: 'Commit 90419cfcb5d9 ("USB: kobil_sct: fix control requests without data stage")'
#6:
Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,

total: 1 errors, 0 warnings, 51 lines checked

Post by Peter Hurley
the bogus data buffer arguments, but still allocate transfer
buffers which are not used.

Applied now.
Thanks,
Johan

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Joe Perches

2014-10-22 12:07:13 UTC

Post by Peter Hurley
Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
"USB: kobil_sct: fix control requests without data stage", removed

checkpatch.pl complains on your commit-reference style so you know.

Thanks for letting me know.
Joe,
Why is checkpatch complaining about the commit reference above?
$ ./scripts/checkpatch.pl ../patches/fixes-3.18/0001-USB-kobil_sct-Remove-unused-transfer-buffer-allocs.patch
ERROR: Please use 12 or more chars for the git commit ID like: 'Commit 90419cfcb5d9 ("USB: kobil_sct: fix control requests without data stage")'
Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,

It wants parentheses around the commit description.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Peter Hurley

2014-10-22 12:09:53 UTC

Post by Joe Perches