Re: [PATCH] usb_wwan: some improvement on write and resume
On Sun, Apr 27, 2014 at 10:15:45AM +0800, Xiao Jin wrote: > > They are however two distinct bugs and should be fixed separately. Could > > you split the fixes into two patches and resubmit? > > > > Please also include a more descriptive subject line for each patch, for > > example: > > > > "USB: usb_wwan: fix urb leak in write error path" > > "USB: usb_wwan: fix race between write and resume" > > > > You should probably point out that the write-resume race could also lead > > to writes being reordered in the description of that patch. > > > > Thanks. I will resubmit two patches. Thanks for resending. I'll include them in the autosuspend series I'll hopefully be able to post shortly. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb_wwan: some improvement on write and resume
They are however two distinct bugs and should be fixed separately. Could you split the fixes into two patches and resubmit? Please also include a more descriptive subject line for each patch, for example: "USB: usb_wwan: fix urb leak in write error path" "USB: usb_wwan: fix race between write and resume" You should probably point out that the write-resume race could also lead to writes being reordered in the description of that patch. Thanks. I will resubmit two patches. Jin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb_wwan: some improvement on write and resume
On Fri, Apr 25, 2014 at 08:57:15AM +0800, xiao jin wrote: > When enable usb serial for modem data, sometimes the tty is blocked > in tty_wait_until_sent because portdata->out_busy always is set and > have no chance to be cleared. > > We have found two scenarios lead to portdata->out_busy problem. > 1. usb_wwan_write set portdata->out_busy firstly, then try autopm > async with error. No out urb submit and no usb_wwan_outdat_callback > to this write, portdata->out_busy can't be cleared. > 2. usb_wwan_resume run play_delayed() and spin_unlock, but > intfdata->suspended still is not set to zero. At this time > usb_wwan_write is called and anchor the urb to delay list. Then resume > keep running but the delayed urb have no chance to be commit until next > resume. If the time of next resume is far away, tty will be blocked > in tty_wait_until_sent during time. First of all, good catch of both of these issues. They are however two distinct bugs and should be fixed separately. Could you split the fixes into two patches and resubmit? Please also include a more descriptive subject line for each patch, for example: "USB: usb_wwan: fix urb leak in write error path" "USB: usb_wwan: fix race between write and resume" You should probably point out that the write-resume race could also lead to writes being reordered in the description of that patch. > The patch make some enhancement on write and resume. > 1. clear portdata->out_busy if usb_wwan_write try autopm async with > error. > 2. put play_Delayed and intfdata->suspended together in the spinlock, > it's to avoid the write race during resume. As I mentioned in the cdc-acm PM thread, there are a few more issues with autosuspend in usb-serial so I'll include your patches in a series that I'm preparing. Some other stuff got in the way this week, but hopefully I'll find the time to finish it this week. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb_wwan: some improvement on write and resume
When enable usb serial for modem data, sometimes the tty is blocked in tty_wait_until_sent because portdata->out_busy always is set and have no chance to be cleared. We have found two scenarios lead to portdata->out_busy problem. 1. usb_wwan_write set portdata->out_busy firstly, then try autopm async with error. No out urb submit and no usb_wwan_outdat_callback to this write, portdata->out_busy can't be cleared. 2. usb_wwan_resume run play_delayed() and spin_unlock, but intfdata->suspended still is not set to zero. At this time usb_wwan_write is called and anchor the urb to delay list. Then resume keep running but the delayed urb have no chance to be commit until next resume. If the time of next resume is far away, tty will be blocked in tty_wait_until_sent during time. The patch make some enhancement on write and resume. 1. clear portdata->out_busy if usb_wwan_write try autopm async with error. 2. put play_Delayed and intfdata->suspended together in the spinlock, it's to avoid the write race during resume. Signed-off-by: xiao jin Signed-off-by: Zhang, Qi1 Reviewed-by: David Cohen --- drivers/usb/serial/usb_wwan.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c index 640fe01..8e06afc 100644 --- a/drivers/usb/serial/usb_wwan.c +++ b/drivers/usb/serial/usb_wwan.c @@ -228,8 +228,10 @@ int usb_wwan_write(struct tty_struct *tty, struct usb_serial_port *port, usb_pipeendpoint(this_urb->pipe), i); err = usb_autopm_get_interface_async(port->serial->interface); - if (err < 0) + if (err < 0) { + clear_bit(i, &portdata->out_busy); break; + } /* send the data */ memcpy(this_urb->transfer_buffer, buf, todo); @@ -661,17 +663,15 @@ int usb_wwan_resume(struct usb_serial *serial) } } + spin_lock_irq(&intfdata->susp_lock); for (i = 0; i < serial->num_ports; i++) { /* walk all ports */ port = serial->port[i]; portdata = usb_get_serial_port_data(port); /* skip closed ports */ - spin_lock_irq(&intfdata->susp_lock); - if (!portdata || !portdata->opened) { - spin_unlock_irq(&intfdata->susp_lock); + if (!portdata || !portdata->opened) continue; - } for (j = 0; j < N_IN_URB; j++) { urb = portdata->in_urbs[j]; @@ -684,9 +684,7 @@ int usb_wwan_resume(struct usb_serial *serial) } } play_delayed(port); - spin_unlock_irq(&intfdata->susp_lock); } - spin_lock_irq(&intfdata->susp_lock); intfdata->suspended = 0; spin_unlock_irq(&intfdata->susp_lock); err_out: -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/