On Thu, Sep 24, 2009 at 03:21:25PM +0200, Oliver Neukum wrote:
> OK, I concur we can't do it with a single bit. We at least need a flag
> for the shutdown state. But do we really need an initializing state?
> It seems to me that we need to solve the initilization problem with
> mutual exclusion and no flag can solve the problem.
Speaking of that, isn't there a race in the current usb serial_open:
/* Do the device-specific open only if the hardware isn't
* already initialized.
*/
if (!test_bit(ASYNCB_INITIALIZED, &port->port.flags)) {
if (mutex_lock_interruptible(&port->mutex))
return -ERESTARTSYS;
mutex_lock(&serial->disc_mutex);
if (serial->disconnected)
retval = -ENODEV;
else
retval = port->serial->type->open(tty, port);
mutex_unlock(&serial->disc_mutex);
mutex_unlock(&port->mutex);
if (retval)
return retval;
set_bit(ASYNCB_INITIALIZED, &port->port.flags);
}
Shouldn't it be something more along the lines of:
if (mutex_lock_interruptible(&port->mutex))
return -ERESTARTSYS;
if (!test_bit(ASYNCB_INITIALIZED, &port->port.flags)) {
mutex_lock(&serial->disc_mutex);
if (serial->disconnected)
retval = -ENODEV;
else
retval = port->serial->type->open(tty, port);
mutex_unlock(&serial->disc_mutex);
if (retval) {
mutex_unlock(&port->mutex);
return retval;
}
set_bit(ASYNCB_INITIALIZED, &port->port.flags);
}
mutex_unlock(&port->mutex);
> The completion handlers care only about an open or closing adapter
> as we have no running URBs unless they have been submitted, but
> drivers use tasklets which need a flag to safely cancel.
Agree on open, but is such a flag for cancelling the tasklets always necessary?
It seems to me that in the ftdi_close case at least, if usb_kill_urb would be
called before cancel_delayed_work_sync (instead of the other way round) then
that would be sufficient and we could then simply have the completion handler
always resubmit the urb until it fails with -EPERM. It solves the stalled read
(port count) issue nicely.
Attaching a patch.
Regards,
Johan
>From 75c078ce9e07759a8a91f2b9ed567e36d6022074 Mon Sep 17 00:00:00 2001
From: Johan Hovold <jhovold@xxxxxxxxx>
Date: Thu, 24 Sep 2009 15:26:25 +0200
Subject: [PATCH] USB: ftdi_sio: Always try to resubmit read urb in completion callback.
Use usb_submit_urb return status rather than port count to determine
when to stop reading from the device.
This fixes a regression in 2.6.31 where read would stall as port count
is no longer necessarily non-zero when the completion handler is called
(even when the port has not been closed).
---
drivers/usb/serial/ftdi_sio.c | 36 ++++++++++++------------------------
1 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 4f883b1..ac65115 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1787,12 +1787,12 @@ static void ftdi_close(struct usb_serial_port *port)
dbg("%s", __func__);
+ /* shutdown our bulk read */
+ usb_kill_urb(port->read_urb);
/* cancel any scheduled reading */
cancel_delayed_work_sync(&priv->rx_work);
- /* shutdown our bulk read */
- usb_kill_urb(port->read_urb);
kref_put(&priv->kref, ftdi_sio_priv_release);
} /* ftdi_close */
@@ -2033,9 +2033,6 @@ static void ftdi_read_bulk_callback(struct urb *urb)
dbg("%s - port %d", __func__, port->number);
- if (port->port.count <= 0)
- return;
-
tty = tty_port_tty_get(&port->port);
if (!tty) {
dbg("%s - bad tty pointer - exiting", __func__);
@@ -2089,9 +2086,6 @@ static void ftdi_process_read(struct work_struct *work)
dbg("%s - port %d", __func__, port->number);
- if (port->port.count <= 0)
- return;
-
tty = tty_port_tty_get(&port->port);
if (!tty) {
dbg("%s - bad tty pointer - exiting", __func__);
@@ -2246,33 +2240,27 @@ static void ftdi_process_read(struct work_struct *work)
goto out;
}
spin_unlock_irqrestore(&priv->rx_lock, flags);
- /* if the port is closed stop trying to read */
- if (port->port.count > 0)
- /* delay processing of remainder */
- schedule_delayed_work(&priv->rx_work, 1);
- else
- dbg("%s - port is closed", __func__);
+ /* delay processing of remainder */
+ schedule_delayed_work(&priv->rx_work, 1);
goto out;
}
/* urb is completely processed */
priv->rx_processed = 0;
- /* if the port is closed stop trying to read */
- if (port->port.count > 0) {
- /* Continue trying to always read */
- usb_fill_bulk_urb(port->read_urb, port->serial->dev,
+ usb_fill_bulk_urb(port->read_urb, port->serial->dev,
usb_rcvbulkpipe(port->serial->dev,
port->bulk_in_endpointAddress),
port->read_urb->transfer_buffer,
port->read_urb->transfer_buffer_length,
ftdi_read_bulk_callback, port);
-
- result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
- if (result)
- dev_err(&port->dev,
- "%s - failed resubmitting read urb, error %d\n",
- __func__, result);
+ result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
+ if (result == -EPERM)
+ dbg("%s: port is closed", __func__);
+ else if (result) {
+ dev_err(&port->dev,
+ "%s - failed resubmitting read urb, error %d\n",
+ __func__, result);
}
out:
tty_kref_put(tty);
--
1.6.4.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
