Re: [PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
On 05/03/2014 11:20 PM, Alan Stern wrote: On Sat, 3 May 2014, xiao jin wrote: We use usb ehci to connect with modem and run stress test on ehci remote wake. Sometimes usb disconnect. We add more debug ftrace (Kernel version: 3.10) and list the key log to show how problem happened. idle-0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD idle-0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd-state[132] pstatus[0][238014c5] suspended_ports[1] reset_done[0] = kernel receive a remote wakeup irq from controller ...-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status 238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT = PORTSC = 238014c5 (line status = K-state, suspend = 1, force port resume = 1, J-to-K transition is detected) ...-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] ...-12873 [000] d..1 26879.393553: ehci_hub_control: [ehci_hub_control]line[891] port[0] hostpc_reg [44000202]-[44000202] idle-0 [001] ..s. 26879.403122: ehci_hub_status_data: wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1] ...-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] = kernel write 238014c5 to PORTSC ...-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status 23801885 17 ERR POWER sig=j SUSPEND PE CONNECT = PORTSC = 23801885 (line status = J-state, suspend = 1), is the status weird? ...-12873 [000] 26879.473158: check_port_resume_type: port 1 status .0507 after resume, -19 ...-12873 [000] 26879.473160: usb_port_resume: status = -19 after check_port_resume_type ...-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19 ...-12873 [000] 26879.473162: hub_port_logical_disconnect: logical disconnect on port 1 This is a little difficult to understand... We add some debug log manually, please let me explain a little more. See above =. There is a in-band remote wakeup and controller run in k-state. Then kernel What do you mean by in-band? We use EHCI as host and have two kinds of mechanism to remote wakeup event, in-band is ehci controller self wakeup, sorry to make you misunderstanding. driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit into controller. Why did it do that? Did the kernel try to resume the port at the same time as the device issued a remote wakeup request? In other words, was there a race between resume and remote wakeup? Yes, I mean a race between kernel driver resume and controller remote wakeup. It makes controller status weird. Why? Your log shows that the RESUME bit was already turned on, so writing a 1 to it shouldn't make any difference. (And the LS(k-state) bit is RO, so writing a 1 to it shouldn't matter.) Here the problem is, after remote wakeup, the controller still is in SUSPEND and kernel report disconnect finally. Could kernel write SUSPEND or Force Port Resume bit be related to the problem we meet with? It's defined in EHCI controller spec(Revision 1.0), If it has enabled remote wake-up, a K-state on the bus will turn the transceiver clock and generate an interrupt. The software will then have to wait 20 ms for the resume to complete and the port to go back to an active state. Where in the spec did you find that quote? It's not present in my copy of the EHCI Rev 1.0 spec. I am sorry to make a mistake, I quote it from controller reference manual. I can find the similar definition in EHCI Rev 1.0, 4.3.1 Port Suspend/Resume. When Force Port Resume bit is a one, the host controller sends resume signaling down the port. System software times the duration of the resume (nominally 20 milliseconds) then sets the Force Port Resume bit to a zero. In this case Kernel should wait for the wakeup finished, then judge what should do next step. We have some thought and give a patch. This patch is to wait for controller RESUME finished when hub try to clear port SUSPEND feature. This is definitely wrong. For one thing, you mustn't have a 20-ms delay with interrupts disabled. For another, the spec states (Table 2-16, the entry for bits 11:10) that the Line Status value is valid only when the port enable bit is 0, so you shouldn't rely on it. Lastly, there really is no need to do anything, because the remote wakeup will finish all by itself. I agree disable irq for maximum 20-ms is not good, but I can find another case when ehci_hub_control() deal with GetPortStatus. I have no idea how controller run, from both EHCI spec and reference manual, I can only deduce that it's better kernel driver don't touch PORTSC until resume finished. Otherwise how to explain the problem we meet with? (After remote wakeup, the controller still is in SUSPEND and kernel report disconnect finally.) When we try the original change in the mail, we never see the same problem until
Re: [PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
On 05/04/2014 08:26 AM, Peter Chen wrote: On Sat, May 03, 2014 at 11:52:25AM +0800, xiao jin wrote: We use usb ehci to connect with modem and run stress test on ehci remote wake. Sometimes usb disconnect. We add more debug ftrace (Kernel version: 3.10) and list the key log to show how problem happened. idle-0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD idle-0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd-state[132] pstatus[0][238014c5] suspended_ports[1] reset_done[0] ...-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status 238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT ...-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] ...-12873 [000] d..1 26879.393553: ehci_hub_control: [ehci_hub_control]line[891] port[0] hostpc_reg [44000202]-[44000202] idle-0 [001] ..s. 26879.403122: ehci_hub_status_data: wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1] ...-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] ...-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status 23801885 17 ERR POWER sig=j SUSPEND PE CONNECT ...-12873 [000] 26879.473158: check_port_resume_type: port 1 status .0507 after resume, -19 ...-12873 [000] 26879.473160: usb_port_resume: status = -19 after check_port_resume_type ...-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19 ...-12873 [000] 26879.473162: hub_port_logical_disconnect: logical disconnect on port 1 There is a in-band remote wakeup and controller run in k-state. Then kernel driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit into controller. It makes controller status weird. It's defined in EHCI Are you sure you are at this path? If there is a remote wakeup, the sending resume signal from host controller will be skipped (USB_PORT_FEAT_SUSPEND), see usb_port_resume at drivers/usb/core/hub.c. Yes, I abstract more debug log to explain. ...-12873 [000] 26879.393496: usb_port_resume: wgq[usb_port_resume] ...-12873 [000] 26879.393544: usb_port_resume: status = 0 after hub_port_status ...-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] ...-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] ...-12873 [000] 26879.413401: usb_port_resume: status = 0 after clear_port_feature controller spec(Revision 1.0), If it has enabled remote wake-up, a K-state on the bus will turn the transceiver clock and generate an interrupt. The software will then have to wait 20 ms for the resume to complete and the port to go back to an active state. In this case Kernel should wait for the wakeup finished, then judge what should do next step. Do you use a chipidea core? Try to do not clear run/stop to see if this problem is fixed or not. I have explained more about the problem in another mail. Please have a look if there still need more info. Jin -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
We use usb ehci to connect with modem and run stress test on ehci remote wake. Sometimes usb disconnect. We add more debug ftrace (Kernel version: 3.10) and list the key log to show how problem happened. idle-0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD idle-0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd-state[132] pstatus[0][238014c5] suspended_ports[1] reset_done[0] ...-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status 238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT ...-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] ...-12873 [000] d..1 26879.393553: ehci_hub_control: [ehci_hub_control]line[891] port[0] hostpc_reg [44000202]-[44000202] idle-0 [001] ..s. 26879.403122: ehci_hub_status_data: wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1] ...-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] ...-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status 23801885 17 ERR POWER sig=j SUSPEND PE CONNECT ...-12873 [000] 26879.473158: check_port_resume_type: port 1 status .0507 after resume, -19 ...-12873 [000] 26879.473160: usb_port_resume: status = -19 after check_port_resume_type ...-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19 ...-12873 [000] 26879.473162: hub_port_logical_disconnect: logical disconnect on port 1 There is a in-band remote wakeup and controller run in k-state. Then kernel driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit into controller. It makes controller status weird. It's defined in EHCI controller spec(Revision 1.0), If it has enabled remote wake-up, a K-state on the bus will turn the transceiver clock and generate an interrupt. The software will then have to wait 20 ms for the resume to complete and the port to go back to an active state. In this case Kernel should wait for the wakeup finished, then judge what should do next step. We have some thought and give a patch. This patch is to wait for controller RESUME finished when hub try to clear port SUSPEND feature. Signed-off-by: xiao jin jin.x...@intel.com Reviewed-by: David Cohen david.a.co...@linux.intel.com --- drivers/usb/host/ehci-hub.c |7 +++ include/linux/usb/ehci_def.h |5 + 2 files changed, 12 insertions(+) diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c index 7ae0c4d..09a8b6b 100644 --- a/drivers/usb/host/ehci-hub.c +++ b/drivers/usb/host/ehci-hub.c @@ -935,6 +935,13 @@ static int ehci_hub_control ( break; } #endif + if ((temp PORT_RESUME) +((temp PORT_LS_MASK) == PORT_K_STATE)) { + ehci_handshake(ehci, status_reg, + PORT_RESUME, 0, 2 /* 20msec */); + temp = ehci_readl(ehci, status_reg); + temp = ~PORT_RWC_BITS; + } if (!(temp PORT_SUSPEND)) break; if ((temp PORT_PE) == 0) diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h index daec99a..0f0f919 100644 --- a/include/linux/usb/ehci_def.h +++ b/include/linux/usb/ehci_def.h @@ -149,6 +149,11 @@ struct ehci_regs { #define PORT_POWER (112) /* true: has power (see PPC) */ #define PORT_USB11(x) (((x)(310)) == (110)) /* USB 1.1 device */ /* 11:10 for detecting lowspeed devices (reset vs release ownership) */ +#define PORT_LS_MASK (0x310) /* line status */ +#define PORT_SE0_STATE (010) +#define PORT_K_STATE (110) +#define PORT_J_STATE (210) +#define PORT_UNDEFINED_STATE (310) /* 9 reserved */ #define PORT_LPM (19) /* LPM transaction */ #define PORT_RESET (18) /* reset port */ -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] USB: usb_wwan: fix race between write and resume
We find a race between write and resume. 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 race also can lead to writes being reordered. This patch put play_Delayed and intfdata-suspended together in the spinlock, it's to avoid the write race during resume. Signed-off-by: xiao jin jin.x...@intel.com Signed-off-by: Zhang, Qi1 qi1.zh...@intel.com Reviewed-by: David Cohen david.a.co...@linux.intel.com --- drivers/usb/serial/usb_wwan.c |8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c index bd30d70..8e06afc 100644 --- a/drivers/usb/serial/usb_wwan.c +++ b/drivers/usb/serial/usb_wwan.c @@ -663,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]; @@ -686,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-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 jin.x...@intel.com Signed-off-by: Zhang, Qi1 qi1.zh...@intel.com Reviewed-by: David Cohen david.a.co...@linux.intel.com --- 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-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: cdc-acm: fix broken runtime suspend
Hi, Johan, On 04/15/2014 03:58 AM, Johan Hovold wrote: The current ACM runtime-suspend implementation is broken in several ways: Firstly, it buffers only the first write request being made while suspended -- any further writes are silently dropped. Secondly, writes being dropped also leak write urbs, which are never reclaimed (until the device is unbound). Thirdly, even the single buffered write is not cleared at shutdown (which may happen before the device is resumed), something which can lead to another urb leak as well as a PM usage-counter leak. Fix this by implementing a delayed-write queue using urb anchors and making sure to discard the queue properly at shutdown. Reported-by: Xiao Jin jin.x...@intel.com Signed-off-by: Johan Hovold jhov...@gmail.com --- Let's fix the current runtime-suspend implementation before considering adding a write fifo. Since this has never really worked, I'll let someone else decide whether the fix should be back-ported to stable or not. Jin, did you check what closing_wait setting your application is using? I check the closing_wait is 30s by default. Below is the trace we get when reproduced problem. ...-1360 [003] d.s5 1843.061418: acm_tty_write: acm_tty_write - write 65 ...-1360 [003] d.s5 1843.061425: acm_write_start: acm_write_start - susp_count 2 ...-2535 [002] 1843.180687: acm_tty_close: acm_tty_close ...-2535 [002] 1843.181217: acm_wb_is_avail: avail n=15 ...-2535 [002] 1843.181238: acm_port_shutdown: acm_port_shutdown ...-438 [003] 1843.182803: acm_wb_is_avail: avail n=16 ...-438 [003] d..1 1843.182817: acm_tty_write: acm_tty_write - write 11 ...-438 [003] d..1 1843.182826: acm_write_start: acm_write_start - susp_count 2 ...-37[003] 1843.202884: acm_resume: wgq[acm_resume] ...-37[003] 1843.202892: acm_resume: wgq[acm_resume] ...-37[003] d..1 1843.203195: acm_resume: send d_wb-1046297992 ...-37[003] 1843.203199: acm_start_wb: acm_start_wb, acm-transmitting=0 idle-0 [000] d.h2 1843.203343: acm_write_done.isra.11: acm_write_done, acm-transmitting=1 ...-1989 [001] 1843.207197: acm_tty_cleanup: acm_tty_cleanup There are two acms in the case, acm0 and acm3. acm0 have delayed 65 bytes before close. When acm0 close, ASYNCB_INITIALIZED flag is cleared, that lead to acm0 have no chance to start delayed wb during acm resume. acm3 delayed 11 bytes send out because it still is opened. It looks closing_wait didn't take effect at that time. I am not sure the reason why because we have no more debug log. Now We are checking the issue again. Could you give this patch a try as well? I try the write and resume part of this patch, anchor urb works well. Thanks, Johan drivers/usb/class/cdc-acm.c | 36 +++- drivers/usb/class/cdc-acm.h | 2 +- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index 900f7ff805ee..ebbcc7a6a7c8 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -571,6 +571,8 @@ static void acm_port_destruct(struct tty_port *port) static void acm_port_shutdown(struct tty_port *port) { struct acm *acm = container_of(port, struct acm, port); + struct urb *urb; + struct acm_wb *wb; int i; dev_dbg(acm-control-dev, %s\n, __func__); @@ -578,6 +580,16 @@ static void acm_port_shutdown(struct tty_port *port) mutex_lock(acm-mutex); if (!acm-disconnected) { usb_autopm_get_interface(acm-control); + spin_lock_irq(acm-write_lock); + for (;;) { + urb = usb_get_from_anchor(acm-delayed); + if (!urb) + break; + wb = urb-context; + wb-use = 0; + usb_autopm_put_interface_async(acm-control); + } + spin_unlock_irq(acm-write_lock); acm_set_control(acm, acm-ctrlout = 0); usb_kill_urb(acm-ctrlurb); for (i = 0; i ACM_NW; i++) @@ -646,12 +658,9 @@ static int acm_tty_write(struct tty_struct *tty, usb_autopm_get_interface_async(acm-control); if (acm-susp_count) { - if (!acm-delayed_wb) - acm-delayed_wb = wb; - else - usb_autopm_put_interface_async(acm-control); + usb_anchor_urb(wb-urb, acm-delayed); spin_unlock_irqrestore(acm-write_lock, flags); - return count; /* A white lie */ + return count; } usb_mark_last_busy(acm-dev); @@ -1267,6 +1276,7 @@ made_compressed_probe: acm-bInterval = epread
Re: [PATCH] cdc-acm: some enhancement on acm delayed write
Hi, Oliver, On 04/10/2014 04:02 PM, Oliver Neukum wrote: On Wed, 2014-04-09 at 22:57 +0800, Xiao Jin wrote: Thanks all for the review. We meet with the problems when developing product. I would like to explain my understanding. On 04/08/2014 11:05 AM, Xiao Jin wrote: We find two problems on acm tty write delayed mechanism. (1) When acm resume, the delayed wb will be started. But now only one write can be saved during acm suspend. More acm write may be abandoned. The scenario usually happened when user space write series AT after acm suspend. If acm accept the first AT, what's the reason for acm to refuse the second AT? If write return 0, user space will try repeatedly until resume. It looks simpler that acm accept all the data and sent out urb when resume. No. We cannot accept an arbitrary amount of data. It would let any user OOM the system. There will have to be an arbitrary limit. The simplest limit is 1 urb. And that is because we said that we are ready to accept data. We apply cdc-acm for modem AT data. I can find other usb modem driver usb_wwan_write use list to accept more data when suspend, maybe usbnet is the same. Do you have any more reason for me to understand why cdc-acm accept only one? (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when close. If acm resume callback run after ASYNCB_INITIALIZED flag cleared, there will have no chance for delayed write to start. That lead to acm_wb.use can't be cleared. If user space open acm tty again and try to setd, tty will be blocked in tty_wait_until_sent for ever. We see tty write and close concurrently after acm suspend in this case. It looks no method to avoid it from tty layer. acm_tty_write and There is a delay user space can set. acm_resume call after acm_port_shutdown. It looks any action in acm_port_shutdown can't solve the problem. As acm has accepted the user space data, we can only find a way to send out urb. I feel anyway to discard the data looks like a lie to user space. In my understanding acm should accept data as much as possible, and send out urb as soon as possible. What do you think of? There's certainly no problem with sending out the data. Yet simply resuming the device in shutdown() should do the job. We see tty write and close concurrently, we have debug log to show that acm_tty_write and acm_resume is called after acm_port_shutdown, I don't understand resuming the device in shutdown() should do the job. Regards Oliver My understanding may be superficial, please correct me if anything wrong. Thanks. Br, Jin -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cdc-acm: some enhancement on acm delayed write
Thanks all for the review. We meet with the problems when developing product. I would like to explain my understanding. On 04/08/2014 11:05 AM, Xiao Jin wrote: We find two problems on acm tty write delayed mechanism. (1) When acm resume, the delayed wb will be started. But now only one write can be saved during acm suspend. More acm write may be abandoned. The scenario usually happened when user space write series AT after acm suspend. If acm accept the first AT, what's the reason for acm to refuse the second AT? If write return 0, user space will try repeatedly until resume. It looks simpler that acm accept all the data and sent out urb when resume. (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when close. If acm resume callback run after ASYNCB_INITIALIZED flag cleared, there will have no chance for delayed write to start. That lead to acm_wb.use can't be cleared. If user space open acm tty again and try to setd, tty will be blocked in tty_wait_until_sent for ever. We see tty write and close concurrently after acm suspend in this case. It looks no method to avoid it from tty layer. acm_tty_write and acm_resume call after acm_port_shutdown. It looks any action in acm_port_shutdown can't solve the problem. As acm has accepted the user space data, we can only find a way to send out urb. I feel anyway to discard the data looks like a lie to user space. In my understanding acm should accept data as much as possible, and send out urb as soon as possible. What do you think of? Br, Jin -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cdc-acm: some enhancement on acm delayed write
From: xiao jin jin.x...@intel.com Date: Tue, 25 Mar 2014 15:54:36 +0800 Subject: [PATCH] cdc-acm: some enhancement on acm delayed write We find two problems on acm tty write delayed mechanism. (1) When acm resume, the delayed wb will be started. But now only one write can be saved during acm suspend. More acm write may be abandoned. (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when close. If acm resume callback run after ASYNCB_INITIALIZED flag cleared, there will have no chance for delayed write to start. That lead to acm_wb.use can't be cleared. If user space open acm tty again and try to setd, tty will be blocked in tty_wait_until_sent for ever. This patch have two modification. (1) use list_head to save the write acm_wb during acm suspend. It can ensure no acm write abandoned. (2) enable flush buffer callback when acm tty close. acm delayed wb will start before acm port shutdown callback, it make sure the delayed acm_wb.use to be cleared. The patch also clear acm_wb.use and acm.transmitting when port shutdown. Signed-off-by: xiao jin jin.x...@intel.com Reviewed-by: David Cohen david.a.co...@linux.intel.com --- drivers/usb/class/cdc-acm.c | 56 ++- drivers/usb/class/cdc-acm.h |3 ++- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index 900f7ff..cfe00b2 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -217,6 +217,24 @@ static int acm_start_wb(struct acm *acm, struct acm_wb *wb) } /* + * Delayed write. + */ +static int acm_start_delayed_wb(struct acm *acm) +{ + struct acm_wb *wb, *n_wb; + LIST_HEAD(delay_wb_list); + + spin_lock_irq(acm-write_lock); + list_replace_init(acm-delayed_wb_list, delay_wb_list); + spin_unlock_irq(acm-write_lock); + list_for_each_entry_safe(wb, n_wb, + delay_wb_list, delay) { + list_del(wb-delay); + acm_start_wb(acm, wb); + } +} + +/* * attributes exported through sysfs */ static ssize_t show_caps @@ -580,8 +598,11 @@ static void acm_port_shutdown(struct tty_port *port) usb_autopm_get_interface(acm-control); acm_set_control(acm, acm-ctrlout = 0); usb_kill_urb(acm-ctrlurb); - for (i = 0; i ACM_NW; i++) + acm-transmitting = 0; + for (i = 0; i ACM_NW; i++) { usb_kill_urb(acm-wb[i].urb); + acm-wb[i].use = 0; + } for (i = 0; i acm-rx_buflimit; i++) usb_kill_urb(acm-read_urbs[i]); acm-control-needs_remote_wakeup = 0; @@ -608,6 +629,8 @@ static void acm_tty_close(struct tty_struct *tty, struct file *filp) { struct acm *acm = tty-driver_data; dev_dbg(acm-control-dev, %s\n, __func__); + /* Set flow_stopped to enable flush buffer*/ + tty-flow_stopped = 1; tty_port_close(acm-port, tty, filp); } @@ -646,10 +669,7 @@ static int acm_tty_write(struct tty_struct *tty, usb_autopm_get_interface_async(acm-control); if (acm-susp_count) { - if (!acm-delayed_wb) - acm-delayed_wb = wb; - else - usb_autopm_put_interface_async(acm-control); + list_add_tail(wb-delay, acm-delayed_wb_list); spin_unlock_irqrestore(acm-write_lock, flags); return count; /* A white lie */ } @@ -688,6 +708,18 @@ static int acm_tty_chars_in_buffer(struct tty_struct *tty) return (ACM_NW - acm_wb_is_avail(acm)) * acm-writesize; } +static void acm_tty_flush_buffer(struct tty_struct *tty) +{ + struct acm *acm = tty-driver_data; + + /* flush delayed write buffer */ + if (!acm-disconnected) { + usb_autopm_get_interface(acm-control); + acm_start_delayed_wb(acm); + usb_autopm_put_interface(acm-control); + } +} + static void acm_tty_throttle(struct tty_struct *tty) { struct acm *acm = tty-driver_data; @@ -1346,6 +1378,7 @@ made_compressed_probe: snd-urb-transfer_flags |= URB_NO_TRANSFER_DMA_MAP; snd-instance = acm; } + INIT_LIST_HEAD(acm-delayed_wb_list); usb_set_intfdata(intf, acm); @@ -1540,7 +1573,6 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message) static int acm_resume(struct usb_interface *intf) { struct acm *acm = usb_get_intfdata(intf); - struct acm_wb *wb; int rv = 0; int cnt; @@ -1555,16 +1587,7 @@ static int acm_resume(struct usb_interface *intf) if (test_bit(ASYNCB_INITIALIZED, acm-port.flags)) { rv = usb_submit_urb(acm-ctrlurb, GFP_NOIO); - spin_lock_irq(acm-write_lock); - if (acm-delayed_wb
[PATCH] xhci-ring: process bulk set actual_length only when not COMP_STOP_INVAL
When suspend, some urb is put into cancelled_td_list. process_bulk_intr_td may process the last trb as below: xhci_transfer_event.transfer_len = 0 xhci_transfer_event.trb_comp_code = 27 urb.transfer_buffer_length = 1024 trb_comp_code is COMP_STOP_INVAL, it should be taken as invalid urb and discarded, but urb actual_length still is calculated as transfer_buffer_length. When handling Stop Endpoint Command completion, the urb in cancelled_td_list is transferred to upper layer(cdc-acm for example). The content of urb transfer_buffer is the overlay of previous two read transfer_buffer. It makes upper layer process wrong buffer. This patch is to set actual_length as transfer_buffer_length only if trb_comp_code is not COMP_STOP_INVAL when process_bulk_intr_td. Signed-off-by: xiao jin jin.x...@intel.com --- drivers/usb/host/xhci-ring.c |7 +-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 6bfbd80..c9a8863 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -2319,8 +2319,11 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td, *status = 0; } } else { - td-urb-actual_length = - td-urb-transfer_buffer_length; + if (trb_comp_code != COMP_STOP_INVAL) + td-urb-actual_length = + td-urb-transfer_buffer_length; + else + td-urb-actual_length = 0; /* Ignore a short packet completion if the * untransferred length was zero. */ -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: cdc-acm: put delayed_wb to list
Hi, Greg, I am sorry for the inconvenience again. I will do as what you point to make sure the thing won't happen again in future. On Wed, 2013-10-16 at 13:44 -0700, Greg KH wrote: On Tue, Oct 15, 2013 at 09:01:35AM +0800, xiao jin wrote: If acm_write_start during acm suspend, write acm_wb is backuped to delayed_wb. When acm resume, the delayed_wb will be started. This mechanism can only record one write during acm suspend. More acm write will be abandoned. This patch is to use list_head to record the write acm_wb during acm suspend. It can ensure no acm write abandoned. Signed-off-by: xiao jin jin.x...@intel.com You obviously did not test this patch at all, as it breaks the build. Please get a senior Intel kernel developer to sign-off on your future patches so this does not happen again. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: cdc-acm: put delayed_wb to list
Hi, Greg, I am sorry for the inconvenience. I will the patch format with the title v2. Thanks for the review. On Mon, 2013-10-14 at 11:10 -0700, Greg KH wrote: On Tue, Oct 08, 2013 at 05:08:48PM +0800, Xiao Jin wrote: From: xiao jin jin.x...@intel.com If acm_write_start during acm suspend, write acm_wb is backuped to delayed_wb. When acm resume, the delayed_wb will be started. This mechanism can only record one write during acm suspend. More acm write will be abandoned. This patch is to use list_head to record the write acm_wb during acm suspend. It can ensure no acm write abandoned. This patch is line-wrapped, making it impossible to apply. Please check your email client and fix up to not do this in the future and resend this patch. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] cdc-acm: put delayed_wb to list
If acm_write_start during acm suspend, write acm_wb is backuped to delayed_wb. When acm resume, the delayed_wb will be started. This mechanism can only record one write during acm suspend. More acm write will be abandoned. This patch is to use list_head to record the write acm_wb during acm suspend. It can ensure no acm write abandoned. Signed-off-by: xiao jin jin.x...@intel.com --- drivers/usb/class/cdc-acm.c | 30 -- drivers/usb/class/cdc-acm.h |7 ++- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index 9f49bfe..be679be 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -607,6 +607,7 @@ static int acm_tty_write(struct tty_struct *tty, unsigned long flags; int wbn; struct acm_wb *wb; + struct delayed_wb *d_wb; if (!count) return 0; @@ -634,12 +635,17 @@ static int acm_tty_write(struct tty_struct *tty, usb_autopm_get_interface_async(acm-control); if (acm-susp_count) { - if (!acm-delayed_wb) - acm-delayed_wb = wb; - else + d_wb = kmalloc(sizeof(struct delayed_wb), GFP_ATOMIC); + if (d_wb == NULL) { usb_autopm_put_interface_async(acm-control); - spin_unlock_irqrestore(acm-write_lock, flags); - return count; /* A white lie */ + spin_unlock_irqrestore(acm-write_lock, flags); + return -ENOMEM; + } else { + d_wb-wb = wb; + list_add_tail(d_wb-list, acm-delayed_wb_list); + spin_unlock_irqrestore(acm-write_lock, flags); + return count; /* A white lie */ + } } usb_mark_last_busy(acm-dev); @@ -1255,6 +1261,7 @@ made_compressed_probe: snd-urb-transfer_flags |= URB_NO_TRANSFER_DMA_MAP; snd-instance = acm; } + INIT_LIST_HEAD(acm-delayed_wb_list); usb_set_intfdata(intf, acm); @@ -1449,6 +1456,7 @@ static int acm_resume(struct usb_interface *intf) { struct acm *acm = usb_get_intfdata(intf); struct acm_wb *wb; + struct delayed_wb *d_wb, *nd_wb; int rv = 0; int cnt; @@ -1464,14 +1472,16 @@ static int acm_resume(struct usb_interface *intf) rv = usb_submit_urb(acm-ctrlurb, GFP_NOIO); spin_lock_irq(acm-write_lock); - if (acm-delayed_wb) { - wb = acm-delayed_wb; - acm-delayed_wb = NULL; + list_for_each_entry_safe(d_wb, nd_wb, + acm-delayed_wb_list, list) { + wb = d_wb-wb; + list_del(d_wb-list); + kfree(d_wb); spin_unlock_irq(acm-write_lock); acm_start_wb(acm, wb); - } else { - spin_unlock_irq(acm-write_lock); + spin_lock_irq(acm-write_lock); } + spin_unlock_irq(acm-write_lock); /* * delayed error checking because we must diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h index 0f76e4a..5eed93f 100644 --- a/drivers/usb/class/cdc-acm.h +++ b/drivers/usb/class/cdc-acm.h @@ -79,6 +79,11 @@ struct acm_rb { struct acm *instance; }; +struct delayed_wb { + struct list_headlist; + struct acm_wb *wb; +} + struct acm { struct usb_device *dev; /* the corresponding usb device */ struct usb_interface *control; /* control interface */ @@ -117,7 +122,7 @@ struct acm { unsigned int throttled:1; /* actually throttled */ unsigned int throttle_req:1;/* throttle requested */ u8 bInterval; - struct acm_wb *delayed_wb; /* write queued for a device about to be woken */ + struct list_head delayed_wb_list; /* delayed wb list */ }; #define CDC_DATA_INTERFACE_TYPE0x0a -- 1.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xhci: Ensure a command structure points to the correct trb on the command ring
Sarah, As you said, I make a mistake and send wrong patch. I am sorry for it. On Fri, 2013-10-11 at 10:28 -0700, Sarah Sharp wrote: On Fri, Oct 11, 2013 at 10:25:23AM -0700, Sarah Sharp wrote: Hi Xiao, I think you did something odd when you tried to send me the latest revision of your patch xhci: correct the usage of USB_CTRL_SET_TIMEOUT. You sent this patch instead. On the plus side, it looks like git-send-email works. Can you try again? Ah, nevermind, I saw the v2 patch you sent later. Sarah On Fri, Oct 11, 2013 at 08:47:54AM +0800, xiao jin wrote: From: Mathias Nyman mathias.ny...@linux.intel.com If a command on the command ring needs to be cancelled before it is handled it can be turned to a no-op operation when the ring is stopped. We want to store the command ring enqueue pointer in the command structure when the command in enqueued for the cancellation case. Some commands used to store the command ring dequeue pointers instead of enqueue (these often worked because enqueue happends to equal dequeue quite often) Other commands correctly used the enqueue pointer but did not check if it pointed to a valid trb or a link trb, this caused for example stop endpoint command to timeout in xhci_stop_device() in about 2% of suspend/resume cases. This should also solve some weird behavior happening in command cancellation cases. This patch is based on a patch submitted by Sarah Sharp to linux-usb, but then forgotten: http://marc.info/?l=linux-usbm=136269803207465w=2 This patch should be backported to kernels as old as 3.7, that contain the commit b92cc66c047ff7cf587b318fe377061a353c120f xHCI: add aborting command ring function Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com Signed-off-by: Sarah Sharp sarah.a.sh...@linux.intel.com Cc: sta...@vger.kernel.org --- drivers/usb/host/xhci-hub.c |2 +- drivers/usb/host/xhci-ring.c | 10 ++ drivers/usb/host/xhci.c | 25 + drivers/usb/host/xhci.h |1 + 4 files changed, 17 insertions(+), 21 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index fae697e..ccf0a06 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -287,7 +287,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) if (virt_dev-eps[i].ring virt_dev-eps[i].ring-dequeue) xhci_queue_stop_endpoint(xhci, slot_id, i, suspend); } - cmd-command_trb = xhci-cmd_ring-enqueue; + cmd-command_trb = xhci_find_next_enqueue(xhci-cmd_ring); list_add_tail(cmd-cmd_list, virt_dev-cmd_list); xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend); xhci_ring_cmd_db(xhci); diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index aaa2906..9ac9672 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -123,6 +123,16 @@ static int enqueue_is_link_trb(struct xhci_ring *ring) return TRB_TYPE_LINK_LE32(link-control); } +union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring) +{ + /* Enqueue pointer can be left pointing to the link TRB, + * we must handle that + */ + if (TRB_TYPE_LINK_LE32(ring-enqueue-link.control)) + return ring-enq_seg-next-trbs; + return ring-enqueue; +} + /* Updates trb to point to the next TRB in the ring, and updates seg if the next * TRB is in a new segment. This does not skip over link TRBs, and it does not * effect the ring dequeue or enqueue pointers. diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 49b6edb..1e36dbb 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -2598,15 +2598,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci, if (command) { cmd_completion = command-completion; cmd_status = command-status; - command-command_trb = xhci-cmd_ring-enqueue; - - /* Enqueue pointer can be left pointing to the link TRB, - * we must handle that - */ - if (TRB_TYPE_LINK_LE32(command-command_trb-link.control)) - command-command_trb = - xhci-cmd_ring-enq_seg-next-trbs; - + command-command_trb = xhci_find_next_enqueue(xhci-cmd_ring); list_add_tail(command-cmd_list, virt_dev-cmd_list); } else { cmd_completion = virt_dev-cmd_completion; @@ -2614,7 +2606,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci, } init_completion(cmd_completion); - cmd_trb = xhci-cmd_ring-dequeue; + cmd_trb = xhci_find_next_enqueue(xhci-cmd_ring); if (!ctx_change) ret = xhci_queue_configure_endpoint(xhci
[PATCH] xhci: Ensure a command structure points to the correct trb on the command ring
From: Mathias Nyman mathias.ny...@linux.intel.com If a command on the command ring needs to be cancelled before it is handled it can be turned to a no-op operation when the ring is stopped. We want to store the command ring enqueue pointer in the command structure when the command in enqueued for the cancellation case. Some commands used to store the command ring dequeue pointers instead of enqueue (these often worked because enqueue happends to equal dequeue quite often) Other commands correctly used the enqueue pointer but did not check if it pointed to a valid trb or a link trb, this caused for example stop endpoint command to timeout in xhci_stop_device() in about 2% of suspend/resume cases. This should also solve some weird behavior happening in command cancellation cases. This patch is based on a patch submitted by Sarah Sharp to linux-usb, but then forgotten: http://marc.info/?l=linux-usbm=136269803207465w=2 This patch should be backported to kernels as old as 3.7, that contain the commit b92cc66c047ff7cf587b318fe377061a353c120f xHCI: add aborting command ring function Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com Signed-off-by: Sarah Sharp sarah.a.sh...@linux.intel.com Cc: sta...@vger.kernel.org --- drivers/usb/host/xhci-hub.c |2 +- drivers/usb/host/xhci-ring.c | 10 ++ drivers/usb/host/xhci.c | 25 + drivers/usb/host/xhci.h |1 + 4 files changed, 17 insertions(+), 21 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index fae697e..ccf0a06 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -287,7 +287,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) if (virt_dev-eps[i].ring virt_dev-eps[i].ring-dequeue) xhci_queue_stop_endpoint(xhci, slot_id, i, suspend); } - cmd-command_trb = xhci-cmd_ring-enqueue; + cmd-command_trb = xhci_find_next_enqueue(xhci-cmd_ring); list_add_tail(cmd-cmd_list, virt_dev-cmd_list); xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend); xhci_ring_cmd_db(xhci); diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index aaa2906..9ac9672 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -123,6 +123,16 @@ static int enqueue_is_link_trb(struct xhci_ring *ring) return TRB_TYPE_LINK_LE32(link-control); } +union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring) +{ + /* Enqueue pointer can be left pointing to the link TRB, +* we must handle that +*/ + if (TRB_TYPE_LINK_LE32(ring-enqueue-link.control)) + return ring-enq_seg-next-trbs; + return ring-enqueue; +} + /* Updates trb to point to the next TRB in the ring, and updates seg if the next * TRB is in a new segment. This does not skip over link TRBs, and it does not * effect the ring dequeue or enqueue pointers. diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 49b6edb..1e36dbb 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -2598,15 +2598,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci, if (command) { cmd_completion = command-completion; cmd_status = command-status; - command-command_trb = xhci-cmd_ring-enqueue; - - /* Enqueue pointer can be left pointing to the link TRB, -* we must handle that -*/ - if (TRB_TYPE_LINK_LE32(command-command_trb-link.control)) - command-command_trb = - xhci-cmd_ring-enq_seg-next-trbs; - + command-command_trb = xhci_find_next_enqueue(xhci-cmd_ring); list_add_tail(command-cmd_list, virt_dev-cmd_list); } else { cmd_completion = virt_dev-cmd_completion; @@ -2614,7 +2606,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci, } init_completion(cmd_completion); - cmd_trb = xhci-cmd_ring-dequeue; + cmd_trb = xhci_find_next_enqueue(xhci-cmd_ring); if (!ctx_change) ret = xhci_queue_configure_endpoint(xhci, in_ctx-dma, udev-slot_id, must_succeed); @@ -3439,14 +3431,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev) /* Attempt to submit the Reset Device command to the command ring */ spin_lock_irqsave(xhci-lock, flags); - reset_device_cmd-command_trb = xhci-cmd_ring-enqueue; - - /* Enqueue pointer can be left pointing to the link TRB, -* we must handle that -*/ - if (TRB_TYPE_LINK_LE32(reset_device_cmd-command_trb-link.control)) - reset_device_cmd-command_trb = - xhci-cmd_ring-enq_seg-next-trbs; + reset_device_cmd-command_trb =
[PATCH v2] xhci: correct the usage of USB_CTRL_SET_TIMEOUT
The usage of USB_CTRL_SET_TIMEOUT in xhci is incorrect. The definition of USB_CTRL_SET_TIMEOUT is 5000ms. The input timeout to wait_for_completion_interruptible_timeout is jiffies. That makes the timeout be longer than what we want, such as 50s in some platform. The patch is to use XHCI_CMD_DEFAULT_TIMEOUT instead of USB_CTRL_SET_TIMEOUT as command completion event timeout. Signed-off-by: xiao jin jin.x...@intel.com --- drivers/usb/host/xhci-hub.c |2 +- drivers/usb/host/xhci.c |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 1d35459..8dbaa81 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -295,7 +295,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) /* Wait for last stop endpoint command to finish */ timeleft = wait_for_completion_interruptible_timeout( cmd-completion, - USB_CTRL_SET_TIMEOUT); + XHCI_CMD_DEFAULT_TIMEOUT); if (timeleft = 0) { xhci_warn(xhci, %s while waiting for stop endpoint command\n, timeleft == 0 ? Timeout : Signal); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 9478caa..5dc6903 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3486,7 +3486,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev) /* Wait for the Reset Device command to finish */ timeleft = wait_for_completion_interruptible_timeout( reset_device_cmd-completion, - USB_CTRL_SET_TIMEOUT); + XHCI_CMD_DEFAULT_TIMEOUT); if (timeleft = 0) { xhci_warn(xhci, %s while waiting for reset device command\n, timeleft == 0 ? Timeout : Signal); -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xhci-hub.c: handle command_trb that may be link TRB
Sarah, Thanks for the reminding. The kernel base of the patch is k3.10, I didn't notice k3.12 commit ec7e43e2d98173483866fe2e4e690143626b659c at that time. I will backport the patch from k3.12 for use. As you mentioned in another email, we meet with dpm timeout when xhci suspend, the root cause are what the patch aimed to solve. FYI. On Wed, 2013-10-09 at 13:18 -0700, Sarah Sharp wrote: Hi Xiao, Thanks for taking the time to submit this patch. Comments below. On Wed, Oct 09, 2013 at 09:42:36AM +0800, Xiao Jin wrote: From: xiao jin jin.x...@intel.com Date: Wed, 9 Oct 2013 09:38:45 +0800 Subject: [PATCH] xhci-hub.c: handle command_trb that may be link TRB I won't be able to apply your patch because it has these extra lines in the body ^^^. I suspect you used `git format-patch` to produce this, and then tried to copy-paste it into your mail client? You can't do that, because your mail client will probably word-wrap your patch, and possibly turn tabs into spaces. You need to use `git send-email` instead to send your patches. When xhci stop device, it's possible cmd_ring enqueue point to link TRB after queue the last but one stop endpoint. We must handle the command_trb point to the next segment trb. Otherwise xhci stop devie will timeout because command_trb can't match with cmd_ring dequeue. The patch is to let command_trb point to the next segment trb if cmd_ring enqueue point to link TRB. Signed-off-by: xiao jin jin.x...@intel.com --- drivers/usb/host/xhci-hub.c |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 1d35459..4872640 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -287,6 +287,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) xhci_queue_stop_endpoint(xhci, slot_id, i, suspend); } cmd-command_trb = xhci-cmd_ring-enqueue; + /* Enqueue pointer can be left pointing to the link TRB, +* we must handle that +*/ + if (TRB_TYPE_LINK_LE32(cmd-command_trb-link.control)) + cmd-command_trb = + xhci-cmd_ring-enq_seg-next-trbs; + list_add_tail(cmd-cmd_list, virt_dev-cmd_list); xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend); xhci_ring_cmd_db(xhci); What kernel version or tree is it against? I ask because there's already a fix in the 3.12-rc4 kernel for this: static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) { ... spin_lock_irqsave(xhci-lock, flags); for (i = LAST_EP_INDEX; i 0; i--) { if (virt_dev-eps[i].ring virt_dev-eps[i].ring-dequeue) xhci_queue_stop_endpoint(xhci, slot_id, i, suspend); } cmd-command_trb = xhci_find_next_enqueue(xhci-cmd_ring); ... Where xhci_find_next_enqueue is defined as: union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring) { /* Enqueue pointer can be left pointing to the link TRB, * we must handle that */ if (TRB_TYPE_LINK_LE32(ring-enqueue-link.control)) return ring-enq_seg-next-trbs; return ring-enqueue; } That was added in commit ec7e43e2d98173483866fe2e4e690143626b659c xhci: Ensure a command structure points to the correct trb on the command ring It's in (or should be in soon) the stable kernels as well. Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] xhci: correct the usage of USB_CTRL_SET_TIMEOUT
From: xiao jin jin.x...@intel.com Date: Wed, 9 Oct 2013 09:09:46 +0800 Subject: [PATCH] xhci: correct the usage of USB_CTRL_SET_TIMEOUT The usage of USB_CTRL_SET_TIMEOUT is incorrect. The definition of USB_CTRL_SET_TIMEOUT is 5000ms. The input timeout to wait_for_completion_interruptible_timeout is jiffies. That makes the timeout be longer than what we want, such as 50s in some platform. The patch is to convert USB_CTRL_SET_TIMEOUT to jiffies as command completion event timeout. Signed-off-by: xiao jin jin.x...@intel.com --- drivers/usb/host/xhci-hub.c |2 +- drivers/usb/host/xhci.c |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 1d35459..78cf294 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -295,7 +295,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) /* Wait for last stop endpoint command to finish */ timeleft = wait_for_completion_interruptible_timeout( cmd-completion, - USB_CTRL_SET_TIMEOUT); + msecs_to_jiffies(USB_CTRL_SET_TIMEOUT)); if (timeleft = 0) { xhci_warn(xhci, %s while waiting for stop endpoint command\n, timeleft == 0 ? Timeout : Signal); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 9478caa..f9ebc72 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3486,7 +3486,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev) /* Wait for the Reset Device command to finish */ timeleft = wait_for_completion_interruptible_timeout( reset_device_cmd-completion, - USB_CTRL_SET_TIMEOUT); + msecs_to_jiffies(USB_CTRL_SET_TIMEOUT)); if (timeleft = 0) { xhci_warn(xhci, %s while waiting for reset device command\n, timeleft == 0 ? Timeout : Signal); -- 1.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html