On Fri, 2009-09-04 at 05:55 -0700, Oliver Neukum wrote:
> Am Friday 04 September 2009 03:32:48 schrieben Sie:
> > On Wed, 2009-08-26 at 10:08 -0700, Oliver Neukum wrote:
> > > Hi,
> > >
> > > Please disregard the last version as it was still buggy. To get rid
> > > of te races we simply need to be merciless to the VM and use GFP_ATOMIC.
> > > This version should fix it. Could you test?
> > >
> > > Regards
> > > Oliver
> >
> > Hi Oliver,
> > Verified the patch. Works well for sierra composite and non-composite
> > devices. Tested with multiple devices as well.
> >
> > Please consider 2 comments below marked (EP).
> > Otherwise looks great and a 'go ahead' from me.
>
> Hi Elina,
>
> yes, you found two bugs. Unfortunately the first one of them invalidates
> the test's results, unless you had fixed it.
> Here's the fixed version.
>
> Regards
> Oliver
Hi Oliver,
I did test with ".support_autosuspend = 1" and with the fix in
sierra_write() yesterday. Sorry I did not mention that.
I also verified the patch you sent me with the fixes.
Looks good.
Regards,
Elina
> --
>
> commit e297b63468dc58e9d3636a65b916ab00c0dae7a2
> Author: Oliver Neukum <oliver@xxxxxxxxxx>
> Date: Fri Sep 4 14:53:42 2009 +0200
>
> usb:sierra: full autosuspend while online
>
> diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
> index f48d05e..f6aceb3 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,13 +273,18 @@ 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,
> + .supports_autosuspend = 1,
> };
>
> 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 +296,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 +409,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 +437,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 +450,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 +466,14 @@ 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) {
> + spin_lock_irqsave(&portdata->lock, flags);
> + portdata->outstanding_urbs--;
> + spin_unlock_irqrestore(&portdata->lock, flags);
> + goto error_simple;
> + }
> +
> buffer = kmalloc(writesize, GFP_ATOMIC);
> if (!buffer) {
> dev_err(&port->dev, "out of memory\n");
> @@ -468,14 +500,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 +538,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 +579,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 +641,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 +762,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 +776,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 +788,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 +798,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 +832,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 +885,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 +913,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;
> + int b;
> +
> + if (serial->dev->auto_pm) {
> + intfdata = serial->private;
> + spin_lock_irq(&intfdata->susp_lock);
> + b = intfdata->in_flight;
> +
> + if (b) {
> + spin_unlock_irq(&intfdata->susp_lock);
> + return -EBUSY;
> + } else {
> + 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;
> +
> + spin_lock_irq(&intfdata->susp_lock);
> + 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);
> + intfdata->in_flight++;
> + err = usb_submit_urb(urb, GFP_ATOMIC);
> + if (err < 0) {
> + intfdata->in_flight--;
> + usb_unanchor_urb(urb);
> + usb_scuttle_anchored_urbs(&portdata->delayed);
> + break;
> + }
> + }
> +
> + if (portdata->opened) {
> + err = sierra_submit_rx_urbs(port, GFP_ATOMIC);
> + if (err)
> + ec++;
> + }
> + }
> + 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 +1010,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
