On Sat, 2009-08-08 at 02:50 -0700, Oliver Neukum wrote:
> Am Freitag, 7. August 2009 22:50:16 schrieb Elina Pasheva:
>
> > There might be a potential race condition in sierra_write() when it
> > checks 'suspended' flag and sierra-resume() being executed at the
same
> > time, a bit more synchronization is needed in that area.
>
> Can you elaborate? Where exactly does it race?
>
Hi Oliver,
First, please find a copy of your patch below which is 'marked' to
illustrate the point.
Secondly, please note that I have added the settings of "suspended"
flag (missing in the original patch) in the copy of the patch below and
I did it the same way it is in the option driver. Without it the patch
does not work at all (cannot resume).
Thirdly, giving credit to Matthew Safar who discovered this race
condition initially in the option.c driver and now in the sierra.c
patch:
If you look at the sierra_resume() function in the 'while' loop where it
processes the delayed queue and further down right after the 'for' loop:
Between the marks "----->" (i.e. between spin_unlock_irq() and the
next spin_lock_irg() setting the "suspend" flag, there is a window of
opportunity for a sierra_write() to be called and because the suspend
flag still indicates "suspended" (see ******* in sierra_write()) these
urbs will be kept on the delayed queue. But right after that the
"suspend" status is set to 0 by the resume() function so in reality the
urbs in the delayed queue will not be processed until the next
(auto)suspend condition occurs or may be lost if the port is closed.
This window of opportunity may be used for instance, in a dual-core
architecture by the other CPU to call write() function, etc.
This condition exist for the option driver as well.
Regards,
Elina
On Sat, 2009-07-25 at 05:00 -0700, Oliver Neukum wrote:
> Hi,
>
> this is a port from option to sierra of the aggressive support for autosuspend.
> This is practically the best you can do under USB 2.0. If possible you can
> use this as a base for autosuspend support. That would save trouble if
> support for this would need yet another firmware revision.
>
> --
>
> commit add5d88d3c241121ff7924838e8d3711f7e32b19
> Author: Oliver Neukum <oliver@xxxxxxxxxx>
> Date: Sat Jul 25 13:32:41 2009 +0200
>
> usb: aggressive autosuspend for sierra ported from option
>
> diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
> index f48d05e..8bae5dd 100644
> --- a/drivers/usb/serial/sierra.c
> +++ b/drivers/usb/serial/sierra.c
> @@ -51,6 +51,12 @@ struct sierra_iface_info {
> const u8 *ifaceinfo; /* pointer to the array holding the numbers */
> };
>
> +struct sierra_intf_private {
> + spinlock_t susp_lock;
> + unsigned int suspended:1;
> + int in_flight;
> +};
> +
> static int sierra_set_power_state(struct usb_device *udev, __u16 swiState)
> {
> int result;
> @@ -144,6 +150,7 @@ static int sierra_probe(struct usb_serial *serial,
> {
> int result = 0;
> struct usb_device *udev;
> + struct sierra_intf_private *data;
> u8 ifnum;
>
> udev = serial->dev;
> @@ -171,6 +178,11 @@ static int sierra_probe(struct usb_serial *serial,
> return -ENODEV;
> }
>
> + data = serial->private = kzalloc(sizeof(struct sierra_intf_private), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + spin_lock_init(&data->susp_lock);
> +
> return result;
> }
>
> @@ -261,6 +273,8 @@ static struct usb_driver sierra_driver = {
> .name = "sierra",
> .probe = usb_serial_probe,
> .disconnect = usb_serial_disconnect,
> + .suspend = usb_serial_suspend,
> + .resume = usb_serial_resume,
> .id_table = id_table,
> .no_dynamic_id = 1,
> };
> @@ -268,6 +282,8 @@ static struct usb_driver sierra_driver = {
> struct sierra_port_private {
> spinlock_t lock; /* lock the structure */
> int outstanding_urbs; /* number of out urbs in flight */
> + struct usb_anchor active;
> + struct usb_anchor delayed;
>
> /* Input endpoints and buffers for this port */
> struct urb *in_urbs[N_IN_URB];
> @@ -279,6 +295,8 @@ struct sierra_port_private {
> int dsr_state;
> int dcd_state;
> int ri_state;
> +
> + unsigned int opened:1;
> };
>
> static int sierra_send_setup(struct usb_serial_port *port)
> @@ -390,21 +408,25 @@ static void sierra_outdat_callback(struct urb *urb)
> {
> struct usb_serial_port *port = urb->context;
> struct sierra_port_private *portdata = usb_get_serial_port_data(port);
> + struct sierra_intf_private *intfdata;
> int status = urb->status;
> - unsigned long flags;
>
> dev_dbg(&port->dev, "%s - port %d\n", __func__, port->number);
> + intfdata = port->serial->private;
>
> /* free up the transfer buffer, as usb_free_urb() does not do this */
> kfree(urb->transfer_buffer);
> -
> + usb_autopm_put_interface_async(port->serial->interface);
> if (status)
> dev_dbg(&port->dev, "%s - nonzero write bulk status "
> "received: %d\n", __func__, status);
>
> - spin_lock_irqsave(&portdata->lock, flags);
> + spin_lock(&portdata->lock);
> --portdata->outstanding_urbs;
> - spin_unlock_irqrestore(&portdata->lock, flags);
> + spin_unlock(&portdata->lock);
> + spin_lock(&intfdata->susp_lock);
> + --intfdata->in_flight;
> + spin_unlock(&intfdata->susp_lock);
>
> usb_serial_port_softint(port);
> }
> @@ -414,6 +436,7 @@ static int sierra_write(struct tty_struct *tty, struct usb_serial_port *port,
> const unsigned char *buf, int count)
> {
> struct sierra_port_private *portdata = usb_get_serial_port_data(port);
> + struct sierra_intf_private *intfdata;
> struct usb_serial *serial = port->serial;
> unsigned long flags;
> unsigned char *buffer;
> @@ -426,9 +449,9 @@ static int sierra_write(struct tty_struct *tty, struct usb_serial_port *port,
> return 0;
>
> portdata = usb_get_serial_port_data(port);
> + intfdata = serial->private;
>
> dev_dbg(&port->dev, "%s: write (%zd bytes)\n", __func__, writesize);
> -
> spin_lock_irqsave(&portdata->lock, flags);
> dev_dbg(&port->dev, "%s - outstanding_urbs: %d\n", __func__,
> portdata->outstanding_urbs);
> @@ -442,6 +465,10 @@ static int sierra_write(struct tty_struct *tty, struct usb_serial_port *port,
> portdata->outstanding_urbs);
> spin_unlock_irqrestore(&portdata->lock, flags);
>
> + retval = usb_autopm_get_interface_async(serial->interface);
> + if (retval < 0)
> + goto error_simple;
> +
> buffer = kmalloc(writesize, GFP_ATOMIC);
> if (!buffer) {
> dev_err(&port->dev, "out of memory\n");
> @@ -468,14 +495,29 @@ static int sierra_write(struct tty_struct *tty, struct usb_serial_port *port,
> /* Handle the need to send a zero length packet */
> urb->transfer_flags |= URB_ZERO_PACKET;
>
> + spin_lock_irqsave(&intfdata->susp_lock, flags);
> +
> *******+ if (intfdata->suspended) {
> + usb_anchor_urb(urb, &portdata->delayed);
> + spin_unlock_irqrestore(&intfdata->susp_lock, flags);
> + goto skip_power;
> + } else {
> + usb_anchor_urb(urb, &portdata->active);
> + }
> /* send it down the pipe */
> retval = usb_submit_urb(urb, GFP_ATOMIC);
> if (retval) {
> + usb_unanchor_urb(urb);
> + spin_unlock_irqrestore(&intfdata->susp_lock, flags);
> dev_err(&port->dev, "%s - usb_submit_urb(write bulk) failed "
> "with status = %d\n", __func__, retval);
> goto error;
> + } else {
> + intfdata->in_flight++;
> + spin_unlock_irqrestore(&intfdata->susp_lock, flags);
> }
>
> +skip_power:
> /* we are done with this urb, so let the host driver
> * really free it when it is finished with it */
> usb_free_urb(urb);
> @@ -491,6 +533,8 @@ error_no_buffer:
> dev_dbg(&port->dev, "%s - 2. outstanding_urbs: %d\n", __func__,
> portdata->outstanding_urbs);
> spin_unlock_irqrestore(&portdata->lock, flags);
> + usb_autopm_put_interface_async(serial->interface);
> +error_simple:
> return retval;
> }
>
> @@ -530,6 +574,7 @@ static void sierra_indat_callback(struct urb *urb)
>
> /* Resubmit urb so we continue receiving */
> if (port->port.count && status != -ESHUTDOWN && status != -EPERM) {
> + usb_mark_last_busy(port->serial->dev);
> err = usb_submit_urb(urb, GFP_ATOMIC);
> if (err)
> dev_err(&port->dev, "resubmit read urb failed."
> @@ -591,6 +636,7 @@ static void sierra_instat_callback(struct urb *urb)
>
> /* Resubmit urb so we continue receiving IRQ data */
> if (port->port.count && status != -ESHUTDOWN && status != -ENOENT) {
> + usb_mark_last_busy(serial->dev);
> urb->dev = serial->dev;
> err = usb_submit_urb(urb, GFP_ATOMIC);
> if (err)
> @@ -711,6 +757,8 @@ static void sierra_close(struct usb_serial_port *port)
> int i;
> struct usb_serial *serial = port->serial;
> struct sierra_port_private *portdata;
> + struct sierra_intf_private *intfdata = port->serial->private;
> +
>
> dev_dbg(&port->dev, "%s\n", __func__);
> portdata = usb_get_serial_port_data(port);
> @@ -723,6 +771,10 @@ static void sierra_close(struct usb_serial_port *port)
> if (!serial->disconnected)
> sierra_send_setup(port);
> mutex_unlock(&serial->disc_mutex);
> + spin_lock_irq(&intfdata->susp_lock);
> + portdata->opened = 0;
> + spin_unlock_irq(&intfdata->susp_lock);
> +
>
> /* Stop reading urbs */
> sierra_stop_rx_urbs(port);
> @@ -731,6 +783,8 @@ static void sierra_close(struct usb_serial_port *port)
> sierra_release_urb(portdata->in_urbs[i]);
> portdata->in_urbs[i] = NULL;
> }
> + usb_autopm_get_interface(serial->interface);
> + serial->interface->needs_remote_wakeup = 0;
> }
> }
>
> @@ -739,6 +793,7 @@ static int sierra_open(struct tty_struct *tty,
> {
> struct sierra_port_private *portdata;
> struct usb_serial *serial = port->serial;
> + struct sierra_intf_private *intfdata = serial->private;
> int i;
> int err;
> int endpoint;
> @@ -772,6 +827,12 @@ static int sierra_open(struct tty_struct *tty,
> }
> sierra_send_setup(port);
>
> + serial->interface->needs_remote_wakeup = 1;
> + spin_lock_irq(&intfdata->susp_lock);
> + portdata->opened = 1;
> + spin_unlock_irq(&intfdata->susp_lock);
> + usb_autopm_put_interface(serial->interface);
> +
> return 0;
> }
>
> @@ -819,6 +880,8 @@ static int sierra_startup(struct usb_serial *serial)
> return -ENOMEM;
> }
> spin_lock_init(&portdata->lock);
> + init_usb_anchor(&portdata->active);
> + init_usb_anchor(&portdata->delayed);
> /* Set the port private data pointer */
> usb_set_serial_port_data(port, portdata);
> }
> @@ -845,6 +908,83 @@ static void sierra_release(struct usb_serial *serial)
> }
> }
>
> +static void stop_read_write_urbs(struct usb_serial *serial)
> +{
> + int i, j;
> + struct usb_serial_port *port;
> + struct sierra_port_private *portdata;
> +
> + /* Stop reading/writing urbs */
> + for (i = 0; i < serial->num_ports; ++i) {
> + port = serial->port[i];
> + portdata = usb_get_serial_port_data(port);
> + for (j = 0; j < N_IN_URB; j++)
> + usb_kill_urb(portdata->in_urbs[j]);
> + usb_kill_anchored_urbs(&portdata->active);
> + }
> +}
> +
> +static int sierra_suspend(struct usb_serial *serial, pm_message_t message)
> +{
> - struct sierra_intf_private *intfdata = serial->private;
struct sierra_intf_private *intfdata = serial->private;
> + int b;
> +
> + if (serial->dev->auto_pm) {
> + intfdata = serial->private;
> + spin_lock_irq(&intfdata->susp_lock);
> + b = intfdata->in_flight;
> + spin_unlock_irq(&intfdata->susp_lock);
> +
> + if (b)
> + return -EBUSY;
> + }
spin_lock_irq(&intfdata->susp_lock);
intfdata->suspended = 1;
spin_unlock_irq(&intfdata->susp_lock);
> + stop_read_write_urbs(serial);
> +
> + return 0;
> +}
> +
> +static int sierra_resume(struct usb_serial *serial)
> +{
> + struct usb_serial_port *port;
> + struct sierra_intf_private *intfdata = serial->private;
> + struct sierra_port_private *portdata;
> + struct urb *urb;
> + int ec = 0;
> + int i, err;
> +
> + for (i = 0; i < serial->num_ports; i++) {
> + port = serial->port[i];
> + portdata = usb_get_serial_port_data(port);
> +
> + while ((urb = usb_get_from_anchor(&portdata->delayed))) {
> + usb_anchor_urb(urb, &portdata->active);
> + spin_lock_irq(&intfdata->susp_lock);
> + intfdata->in_flight++;
> + spin_unlock_irq(&intfdata->susp_lock);
> + err = usb_submit_urb(urb, GFP_NOIO);
> + if (err < 0) {
> + spin_lock_irq(&intfdata->susp_lock);
> + intfdata->in_flight--;
> + spin_unlock_irq(&intfdata->susp_lock);
> + usb_unanchor_urb(urb);
> + usb_scuttle_anchored_urbs(&portdata->delayed);
> + break;
> + }
> + }
> +
> + spin_lock_irq(&intfdata->susp_lock);
> + if (portdata->opened) {
> + err = sierra_submit_rx_urbs(port, GFP_ATOMIC);
> + if (err)
> + ec++;
> + }
> +------> spin_unlock_irq(&intfdata->susp_lock);
> + }
> +
-----> spin_lock_irq(&intfdata->susp_lock);
intfdata->suspended = 0;
spin_unlock_irq(&intfdata->susp_lock);
> +
> + return ec ? -EIO : 0;
> +}
> +
> static struct usb_serial_driver sierra_device = {
> .driver = {
> .owner = THIS_MODULE,
> @@ -865,6 +1005,8 @@ static struct usb_serial_driver sierra_device = {
> .tiocmset = sierra_tiocmset,
> .attach = sierra_startup,
> .release = sierra_release,
> + .suspend = sierra_suspend,
> + .resume = sierra_resume,
> .read_int_callback = sierra_instat_callback,
> };
>
>
>
--
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
