diff mbox series

[3/5] USB: serial: ir-usb: fix IrLAP framing

Message ID 20200122101530.29176-4-johan@kernel.org (mailing list archive)
State Mainlined
Commit 38c0d5bdf4973f9f5a888166e9d3e9ed0d32057a
Headers show
Series USB: serial: ir-usb fixes and cleanups | expand

Commit Message

Johan Hovold Jan. 22, 2020, 10:15 a.m. UTC
Commit f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
switched to using the generic write implementation which may combine
multiple write requests into larger transfers. This can break the IrLAP
protocol where end-of-frame is determined using the USB short packet
mechanism, for example, if multiple frames are sent in rapid succession.

Fixes: f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
Cc: stable <stable@vger.kernel.org>     # 2.6.35
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ir-usb.c | 113 +++++++++++++++++++++++++++++-------
 1 file changed, 91 insertions(+), 22 deletions(-)

Comments

Greg KH Jan. 22, 2020, 1:25 p.m. UTC | #1
On Wed, Jan 22, 2020 at 11:15:28AM +0100, Johan Hovold wrote:
> Commit f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> switched to using the generic write implementation which may combine
> multiple write requests into larger transfers. This can break the IrLAP
> protocol where end-of-frame is determined using the USB short packet
> mechanism, for example, if multiple frames are sent in rapid succession.
> 
> Fixes: f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> Cc: stable <stable@vger.kernel.org>     # 2.6.35
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/ir-usb.c | 113 +++++++++++++++++++++++++++++-------
>  1 file changed, 91 insertions(+), 22 deletions(-)

Ah, nice fix, sorry about that :(

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Johan Hovold Jan. 22, 2020, 1:37 p.m. UTC | #2
On Wed, Jan 22, 2020 at 02:25:42PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 22, 2020 at 11:15:28AM +0100, Johan Hovold wrote:
> > Commit f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> > switched to using the generic write implementation which may combine
> > multiple write requests into larger transfers. This can break the IrLAP
> > protocol where end-of-frame is determined using the USB short packet
> > mechanism, for example, if multiple frames are sent in rapid succession.
> > 
> > Fixes: f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> > Cc: stable <stable@vger.kernel.org>     # 2.6.35
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/serial/ir-usb.c | 113 +++++++++++++++++++++++++++++-------
> >  1 file changed, 91 insertions(+), 22 deletions(-)
> 
> Ah, nice fix, sorry about that :(

The offending commit was mine so same to you. ;)

> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Johan
diff mbox series

Patch

diff --git a/drivers/usb/serial/ir-usb.c b/drivers/usb/serial/ir-usb.c
index 26eab1307165..627bea7e6cfb 100644
--- a/drivers/usb/serial/ir-usb.c
+++ b/drivers/usb/serial/ir-usb.c
@@ -45,9 +45,10 @@  static int buffer_size;
 static int xbof = -1;
 
 static int  ir_startup (struct usb_serial *serial);
-static int  ir_open(struct tty_struct *tty, struct usb_serial_port *port);
-static int ir_prepare_write_buffer(struct usb_serial_port *port,
-						void *dest, size_t size);
+static int ir_write(struct tty_struct *tty, struct usb_serial_port *port,
+		const unsigned char *buf, int count);
+static int ir_write_room(struct tty_struct *tty);
+static void ir_write_bulk_callback(struct urb *urb);
 static void ir_process_read_urb(struct urb *urb);
 static void ir_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios);
@@ -77,8 +78,9 @@  static struct usb_serial_driver ir_device = {
 	.num_ports		= 1,
 	.set_termios		= ir_set_termios,
 	.attach			= ir_startup,
-	.open			= ir_open,
-	.prepare_write_buffer	= ir_prepare_write_buffer,
+	.write			= ir_write,
+	.write_room		= ir_write_room,
+	.write_bulk_callback	= ir_write_bulk_callback,
 	.process_read_urb	= ir_process_read_urb,
 };
 
@@ -254,35 +256,102 @@  static int ir_startup(struct usb_serial *serial)
 	return 0;
 }
 
-static int ir_open(struct tty_struct *tty, struct usb_serial_port *port)
+static int ir_write(struct tty_struct *tty, struct usb_serial_port *port,
+		const unsigned char *buf, int count)
 {
-	int i;
+	struct urb *urb = NULL;
+	unsigned long flags;
+	int ret;
 
-	for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i)
-		port->write_urbs[i]->transfer_flags = URB_ZERO_PACKET;
+	if (port->bulk_out_size == 0)
+		return -EINVAL;
 
-	/* Start reading from the device */
-	return usb_serial_generic_open(tty, port);
-}
+	if (count == 0)
+		return 0;
 
-static int ir_prepare_write_buffer(struct usb_serial_port *port,
-						void *dest, size_t size)
-{
-	unsigned char *buf = dest;
-	int count;
+	count = min(count, port->bulk_out_size - 1);
+
+	spin_lock_irqsave(&port->lock, flags);
+	if (__test_and_clear_bit(0, &port->write_urbs_free)) {
+		urb = port->write_urbs[0];
+		port->tx_bytes += count;
+	}
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	if (!urb)
+		return 0;
 
 	/*
 	 * The first byte of the packet we send to the device contains an
-	 * inbound header which indicates an additional number of BOFs and
+	 * outbound header which indicates an additional number of BOFs and
 	 * a baud rate change.
 	 *
 	 * See section 5.4.2.2 of the USB IrDA spec.
 	 */
-	*buf = ir_xbof | ir_baud;
+	*(u8 *)urb->transfer_buffer = ir_xbof | ir_baud;
+
+	memcpy(urb->transfer_buffer + 1, buf, count);
+
+	urb->transfer_buffer_length = count + 1;
+	urb->transfer_flags = URB_ZERO_PACKET;
+
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret) {
+		dev_err(&port->dev, "failed to submit write urb: %d\n", ret);
+
+		spin_lock_irqsave(&port->lock, flags);
+		__set_bit(0, &port->write_urbs_free);
+		port->tx_bytes -= count;
+		spin_unlock_irqrestore(&port->lock, flags);
+
+		return ret;
+	}
+
+	return count;
+}
+
+static void ir_write_bulk_callback(struct urb *urb)
+{
+	struct usb_serial_port *port = urb->context;
+	int status = urb->status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	__set_bit(0, &port->write_urbs_free);
+	port->tx_bytes -= urb->transfer_buffer_length - 1;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	switch (status) {
+	case 0:
+		break;
+	case -ENOENT:
+	case -ECONNRESET:
+	case -ESHUTDOWN:
+		dev_dbg(&port->dev, "write urb stopped: %d\n", status);
+		return;
+	case -EPIPE:
+		dev_err(&port->dev, "write urb stopped: %d\n", status);
+		return;
+	default:
+		dev_err(&port->dev, "nonzero write-urb status: %d\n", status);
+		break;
+	}
+
+	usb_serial_port_softint(port);
+}
+
+static int ir_write_room(struct tty_struct *tty)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	int count = 0;
+
+	if (port->bulk_out_size == 0)
+		return 0;
+
+	if (test_bit(0, &port->write_urbs_free))
+		count = port->bulk_out_size - 1;
 
-	count = kfifo_out_locked(&port->write_fifo, buf + 1, size - 1,
-								&port->lock);
-	return count + 1;
+	return count;
 }
 
 static void ir_process_read_urb(struct urb *urb)