Re: [RFC PATCH 0/2] usb: dwc2: Enable URB giveback in a tasklet context
On Mon, Sep 15, 2014 at 11:22 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Hi Ming-Lei, Thanks for your patch to enable the URB giveback in a tasklet context for the EHCI driver. I found your patch to fix a USB webcam timeout/stutter issue on the DWC2 HCD in the SOCFPGA platform. However, I need your help trying to figure out why I need the 2nd patch to get your URB giveback patch to fully work. Without your second patch, local interrupt is still disabled even the completion handler is run from tasklet context. Generally speaking, what we want is to decrease the interrupt disable period from USB. But this change can make the same host controller interrupt handled on other CPUs at the same time too. I enable tracepoints in the dwc2 dwc2_handle_hcd_intr for irq:irq_handler_entry and irq:irq_handler_exit. However, I did not see any measurable differences between enabling HCD_BH and no enabling HCD_BH. But I am able to get a full Which platform are you running? There should have been big difference between the two setting. Per my experiences, some ARM platform uses a low frequency clock source(such as 32K on pandaboard, or omap5), so the time stamp in trace isn't accurate enough, but high resolution timer can be passed from kernel parameter too. image from a webcam with the following 2 patches, and timeouts/green screen without the 2 patches. I am using a Logitech HD C270 webcam. Opening video decoder: [raw] RAW Uncompressed Video Movie-Aspect is undefined - no prescaling applied. VO: [sdl] 640x480 = 640x480 Packed YUY2 Selected video codec: [rawyuy2] vfm: raw (RAW YUY2) Can you provide any comments on the following issues: 1) Am I placing the tracepoints in the right place for this non-ehci hcd? I think it is right if URB completion is run from the only interrupt handler of DWC2. Maybe you needn't put it by yourself, and the built-in irq_handler_entry and irq_handler_exit events should be enough for your test. 2) I don't quite understand why removing local_irq_save/local_irq_restore around the the complete makes the webcam work? The two helpers around the -complete() should have been removed but it isn't mature since we need to make all current drivers safe with the conversion. See below link: http://marc.info/?w=2r=1s=enabling+irq+completeq=t If you plan to use the feature, the above changes have to be merged first. Thanks, -- 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: [RFC PATCH 0/2] usb: dwc2: Enable URB giveback in a tasklet context
On Tue, Sep 16, 2014 at 7:16 AM, Paul Zimmerman paul.zimmer...@synopsys.com wrote: From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com] Sent: Monday, September 15, 2014 8:23 AM Hi Ming-Lei, Thanks for your patch to enable the URB giveback in a tasklet context for the EHCI driver. I found your patch to fix a USB webcam timeout/stutter issue on the DWC2 HCD in the SOCFPGA platform. However, I need your help trying to figure out why I need the 2nd patch to get your URB giveback patch to fully work. I enable tracepoints in the dwc2 dwc2_handle_hcd_intr for irq:irq_handler_entry and irq:irq_handler_exit. However, I did not see any measurable differences between enabling HCD_BH and no enabling HCD_BH. But I am able to get a full image from a webcam with the following 2 patches, and timeouts/green screen without the 2 patches. I am using a Logitech HD C270 webcam. Opening video decoder: [raw] RAW Uncompressed Video Movie-Aspect is undefined - no prescaling applied. VO: [sdl] 640x480 = 640x480 Packed YUY2 Selected video codec: [rawyuy2] vfm: raw (RAW YUY2) Can you provide any comments on the following issues: 1) Am I placing the tracepoints in the right place for this non-ehci hcd? 2) I don't quite understand why removing local_irq_save/local_irq_restore around the the complete makes the webcam work? Thanks in advance for your comments. Hi Dinh, I'm not Ming, but I can hazard a guess why you are seeing this issue. The webcam driver is likely doing a lot of processing in the -complete callback. With local interrupts disabled, this probably causes too long of a delay in handling other interrupts. Since the SOCFPGA platform is a uniprocessor (I think), there is no other CPU to handle the other interrupts. Per my previous observation, on some ARM platforms it is extremely slow to memcpy to DMA coherent buffer, which is used in uvcvideo at default. I remember Pandaboard should be one such platform. You can check if it is your case. I think the only solution is to look at whatever driver is handling the webcam, and see if it can re-enable interrupts earlier. I believe this is an OK thing for it to do when the interrupt handler is threaded. You need to persuade all usb camera drivers' maintainera to agree on this change to these drivers, :-) We discussed to take threaded-irq too, looks which may decrease usb-storage performance a bit. Ah, wait. I just looked at the specs, and I see the SOCFPGA is dual-core. So the second core should be available for handling the other interrupts. I wonder if something is wrong there? Is the 2nd CPU actually running? Are interrupts getting disabled on both CPUs somehow? The same interrupt can't be handled on two CPU cores at the same time. Thanks, -- 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: questions about give back urb in tasklet
On Wed, Mar 19, 2014 at 11:05 PM, vichy vichy@gmail.com wrote: hi Ming: One approach I like to use is trace event. Would you please let us how to to use trace event for calculate max/average usb irq time? You can enable irq_handler_entry and irq_handler_exit event, then write a script easily to figure out the time consumed in ehci irq handler, see Documentation/trace/events.txt of linux kernel for detailed steps. I find it ^^ But actually the local interrupt is still disabled during completion because we need to convert spin_lock in all current usb drivers to spin_lock_irqsave() first. Would you please let me know the function or source you mentioned? The local interrupt is disabled during completion handler in __usb_hcd_giveback_urb(). BTW, why I cannot see xhci add HCD_BH? There might be some reasons: - in previous following up discussion, Alan thought it should be easier to just split the hcd interrupt handler into two parts - as I said, spin_lock() isn't converted to spin_lock_irqsave() in drivers' completion handler yet Do you mean , take uvc/uac for example, if we put completion what they register previously in BH, the spin_lock() in their completions should be changed to spin_lock_irqsave()? Is it due to original completion is called in irq context? Some drivers may assume that the completion handler is called from hardirq handler, so call spin_lock(), and they should have called spin_lock_irqsave() if there are shared resources between its completion handler and other irq handler. So the simplest approach for the conversion is to replace all spin_lock() inside urb completion path into spin_lock_irqsave(). - maybe no one really care the time in which local interrupt is disabled by USB completion handler.(That might be true because USB devices are commonly used in personal computer, not in big server product) is there some reason xhci cannot put completion in BH? IMO, the completion handler should be put into tasklet, and can be done. If you like, you can take time to do that. Below is so far kernel implementation. Isn't it put completion handler into tasklet? Yes, it is run in tasklet context with local irq disabled. As you know we need to enable irq during URB completion. ... .. spin_lock(bh-lock); list_add_tail(urb-urb_list, bh-head); running = bh-running; spin_unlock(bh-lock); if (running) ; else if (high_prio_bh) tasklet_hi_schedule(bh-bh); else tasklet_schedule(bh-bh); Thanks for your help, -- 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: questions about give back urb in tasklet
On Wed, Mar 19, 2014 at 5:57 PM, vichy vichy@gmail.com wrote: hi Ming.lei: 2014-03-17 22:01 GMT+08:00 Alan Stern st...@rowland.harvard.edu: On Sun, 16 Mar 2014, vichy wrote: hi all: recently we bump to system performance issue when usb irq take quite long. I found below link for discussing how to short in http://permalink.gmane.org/gmane.linux.usb.general/89363 I have some questions about this patch. 1. is there patch or kernel config I can use to measure man/avage usb irq time consuming like the above link show One approach I like to use is trace event. 2. I see this patch is roll back in commit c04ee4b1136e462722567cf6e76bb35a181574a7 and intend to be ready in 3.13-rc1 Is there special reason why we need to roll back? Yes, the revert revert can enable to run completion handler in BH, isn't that what you need? But actually the local interrupt is still disabled during completion because we need to convert spin_lock in all current usb drivers to spin_lock_irqsave() first. I posted lot of patches to do the conversion, but unfortunately most of them aren't merged. Thanks, -- Ming Lei -- 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: questions about give back urb in tasklet
On Wed, Mar 19, 2014 at 9:34 PM, vichy vichy@gmail.com wrote: hi Ming in http://permalink.gmane.org/gmane.linux.usb.general/89363 I have some questions about this patch. 1. is there patch or kernel config I can use to measure man/avage usb irq time consuming like the above link show One approach I like to use is trace event. Would you please let us how to to use trace event for calculate max/average usb irq time? You can enable irq_handler_entry and irq_handler_exit event, then write a script easily to figure out the time consumed in ehci irq handler, see Documentation/trace/events.txt of linux kernel for detailed steps. 2. I see this patch is roll back in commit c04ee4b1136e462722567cf6e76bb35a181574a7 and intend to be ready in 3.13-rc1 Is there special reason why we need to roll back? Yes, the revert revert can enable to run completion handler in BH, isn't that what you need? YES, what I need is try to see whether my problem will be solved if running completion handlers in BH. At beginning, the revert let me think there are something wrong after we put completion in BH. And we need to revert it back to put completion in irq so the revert mean put completion in BH, right? Yes, it is a revert revert. But actually the local interrupt is still disabled during completion because we need to convert spin_lock in all current usb drivers to spin_lock_irqsave() first. Would you please let me know the function or source you mentioned? The local interrupt is disabled during completion handler in __usb_hcd_giveback_urb(). BTW, why I cannot see xhci add HCD_BH? There might be some reasons: - in previous following up discussion, Alan thought it should be easier to just split the hcd interrupt handler into two parts - as I said, spin_lock() isn't converted to spin_lock_irqsave() in drivers' completion handler yet - maybe no one really care the time in which local interrupt is disabled by USB completion handler.(That might be true because USB devices are commonly used in personal computer, not in big server product) is there some reason xhci cannot put completion in BH? IMO, the completion handler should be put into tasklet, and can be done. If you like, you can take time to do that. Thanks, -- Ming Lei -- 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: 3.13-rc1 regression: Scatter-gather list issues at SuperSpeed only
On Tue, Mar 4, 2014 at 10:56 PM, David Laight david.lai...@aculab.com wrote: From: Bjørn Mork If xHCI won't plan to support arbitrary-length scatter-gather any more, that is fine to revert the commit forever. Otherwise, it should be better to just clear no_sg_constraint in xhcd, shouldn't it? No, that's what's currently causing bugs with the storage driver. IIUC, the bug was added by commit 10e232c597ac (USB: check sg buffer size in usb_submit_urb) which introduced an additional restriction on SG URBs not present before: + for_each_sg(urb-sg, sg, urb-num_sgs - 1, i) + if (sg-length % max) + return -EINVAL; where max is usb_endpoint_maxp(ep-desc). As has been shown by numerous bug reports lately, the storage driver will submit SG lists with 512 byte elements on superspeed endpoints with max == 1024. That has never been 'fine'. So when this SS storage device(maxp isn't 512) comes and block layer may send scatter-gather list with an non-tail entry that isn't aligned to the max packet size, looks xHCD has to enable no_sg_constraint and has to deal with it correctly, and I am wondering if there is other solutions for the case. Basically this case is very similar with current usbnet(axis 88179/ 178a). Thanks, -- Ming Lei -- 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: 3.13-rc1 regression: Scatter-gather list issues at SuperSpeed only
On Tue, Mar 4, 2014 at 11:21 PM, David Laight david.lai...@aculab.com wrote: Actually most of the block layer code could be taught to split requests into multiple URBs. Or even resubmit bounced requests split in two. Although splitting requests won't help the network code. It might not help storage too since the single URB in the middle of transfer produces short packet. Thanks, -- Ming Lei -- 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: 3.13-rc1 regression: Scatter-gather list issues at SuperSpeed only
On Tue, Mar 4, 2014 at 5:47 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: Greg, Dave, Freddy, question about cross-subsystem reverts below: On Fri, Feb 28, 2014 at 04:15:12PM -0500, Alan Stern wrote: On Fri, 28 Feb 2014, Sarah Sharp wrote: When testing 3.14-rc1 with a USB 3.0 Lexar flash drive, the drive fails to be mounted. I added a bit of debugging to the USB core: That revealed the SCSI request fails because the USB core is rejecting a scatter-gather list with an entry that isn't aligned to the max packet size: Feb 28 09:45:30 xanatos kernel: [ 376.449316] usb 2-2: URB sg entry 0 of 17, size 1536 not a multiple of ep1in max packet 1024 Notice the request length: 1536. That's three 512-byte sectors. A little unusual, since most I/O is done in units of pages, which are 4096 bytes. It's failing because of commit 247bf557273d xhci 1.0: Limit arbitrarily-aligned scatter gather. That commit clears the hcd-self.no_sg_constraint flag if the host is a 1.0 host (which my Panther Point host is). It was put in to avoid TD fragment issues on 1.0 hosts with ethernet devices. (Note, this also means that David Laight's potential work-around patch [1] wouldn't help if arbitrary-length scatter gather bigger than a ring segment was submitted.) The behavior for reproducing this is odd. I can only reproduce this on my Ubuntu 13.10 laptop with Intel Panther Point xHCI, when the device is running at SuperSpeed. If I plug the device into an EHCI port, or behind a USB 2.0 hub plugged into an xHCI port, I never see these arbitrary-length scatter-gather list entries. Dan can't reproduce this on his Intel Haswell machine running Fedora at all. Some of this behavior is to be expected. 1536 isn't a multiple of 1024 (the maxpacket size when running at SuperSpeed), but it _is_ a multiple of 512 (the maxpacket size when running at high speed). Therefore the failure won't occur when the drive is attached to an EHCI controller or is behind a USB-2 hub. Ah, that makes sense now. Does the same thing happen if you prevent the system from automatically trying to mount the volume? If the system doesn't automatically mount the volume, the error doesn't occur. If we can't figure out how to get max-packet sized scatter-gather list entries from the mass storage driver, Mathias is going to need to: The SG entries don't come from usb-storage; they come from the block layer. As far as I know, there is no way to tell the block layer that each element in an SG list (except the last) must be a multiple of some specific size. revert commit 3804fad45411 USBNET: ax88179_178a: enable tso if usb host supports sg dma revert commit 247bf557273d xhci 1.0: Limit arbitrarily-aligned scatter gather. And we'll need to focus on getting the TD fragments supported in 3.16. So far we've gotten away with this at high speed or below, because no USB mass-storage devices have a block size smaller than 512 (at least, none that I've ever heard of). But when the maxpacket size is 1024, a request for an odd number of blocks can cause trouble. Ok, we can't have SuperSpeed mass storage devices broken, so it looks like we'll have to revert the last patch to add scatter-gather to the ASIX driver to avoid that breakage. That means Mathias is going to need to revert those two commits then, since he's taking over pushing xHCI driver bug fixes this kernel. Greg, Dave, Freddy, how do you want to handle reverting commit 3804fad45411? Should that come through Dave's networking tree or Greg's USB tree? Sorry, could you explain why you want to revert commit 3804fad45411 (USBNET: ax88179_178a)? If xHCI won't plan to support arbitrary-length scatter-gather any more, that is fine to revert the commit forever. Otherwise, it should be better to just clear no_sg_constraint in xhcd, shouldn't it? Thanks, -- Ming Lei -- 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 RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 1, 2014 at 9:30 PM, Mark Lord ml...@pobox.com wrote: On 14-02-01 02:54 AM, Ming Lei wrote: .. With SG enabled, for the iperf client test case, the average urb size for transmission will be increased from ~1500 to ~20K bytes in my test case: iperf -c $SRV -t 30 -P 4 -w 128K So I am wondering you guys do not care the improvement .. No, that's not it. Simply, the recent changes killed the driver I just want to clarify the sg approach does improve performance, instead of no improvement mentioned by your guys. for some users, something Linus calls a regression, and does not permit. Even real regressions are easily/often introduced, and we are discussing how to fix that. I suggest to unset the flag only for the known buggy controllers. Thanks, -- Ming Lei -- 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 RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 1, 2014 at 3:00 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Fri, Jan 31, 2014 at 08:17:58AM +0800, Ming Lei wrote: On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: FWIW, the plan looks fine to me. Just adding a couple of hints to simplify the implementation. Sarah Sharp sarah.a.sh...@linux.intel.com writes: Let's do this fix the right way, instead of wall papering over the issue. Here's what we should do: 1. Disable scatter-gather for the ax88179_178a driver when it's under an xHCI host. No need to make this conditional. SG is only enabled in the ax88179_178a driver if udev-bus-no_sg_constraint is true, so it applies only to xHCI hosts in the first place. Ah, so you're suggesting just reverting commit 3804fad45411b48233b48003e33a78f290d227c8 USBNET: ax88179_178a: enable tso if usb host supports sg dma? If I understand the problem correctly, the current issue is that xhci driver doesn't support the arbitrary dma length not well, but per XHCI spec, it should be supported, right? If the above is correct, reverting the commit isn't correct since there isn't any issue about the commit, so I suggest to disable the flag in xhci for the buggy devices, and it may be enabled again if the problem is fixed. Ok, I like that plan, since it means I don't have to touch any networking code to fix this. :) I believe that means we'll have to disable the flag for all 1.0 xHCI hosts, since those are the ones that need TD fragments. 2. Revert the following commits: f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes. d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs 35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst 3. Dan and Mathias can work together to come up with an overall plan to change the xHCI driver architecture to be fully compliant with the TD fragment rules. That can be done over the next few kernel releases. The end result is that we don't destabilize storage or break userspace USB drivers, we don't break people's xHCI host controllers, the ax88179_178a USB ethernet devices still work under xHCI (a bit with worse performance), and other USB ethernet devices still get the performance improvement introduced in 3.12. No other usbnet drivers has enabled SG... Which is why you have only seen this problem with the ax88179_178a devices. So there is no performance improvement to keep. In my test environment, the patch does improve both throughput and cpu utilization, if you search the previous email for the patch, you can see the data. With SG enabled, for the iperf client test case, the average urb size for transmission will be increased from ~1500 to ~20K bytes in my test case: iperf -c $SRV -t 30 -P 4 -w 128K So I am wondering you guys do not care the improvement, maybe the CPU is powerful enough to not degrade throughoutcpu utilization not much, but there is still the potential CPU wakeup issue, which means extra CPU power consumption might be introduced after disabling SG for usbnet. Right, I did see the performance improvement note in that commit. Do you know if the ARM A15 dual core board was using a 0.96 xHCI host, or a 1.0 host? You can find out by reloading the xHCI driver with dynamic debugging turned on: # sudo modprobe xhci_hcd dyndbg Looks I can't find the parameter 'dyndbg' for xhci_hcd. and then look for lines like: [25296.765767] xhci_hcd :00:14.0: HCIVERSION: 0x100 I change xhci_dbg.c manually with below: diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c index b016d38..1ae1966 100644 --- a/drivers/usb/host/xhci-dbg.c +++ b/drivers/usb/host/xhci-dbg.c @@ -66,7 +66,7 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci) (unsigned int) temp); xhci_dbg(xhci, CAPLENGTH: 0x%x\n, (unsigned int) HC_LENGTH(temp)); - xhci_dbg(xhci, HCIVERSION: 0x%x\n, + dev_info(xhci_to_hcd(xhci)-self.controller, HCIVERSION: 0x%x\n, (unsigned int) HC_VERSION(temp)); and got the below output: [tom@ming ~]$ dmesg | grep HCIVERSION xhci-hcd xhci-hcd.2.auto: HCIVERSION: 0x100 Thanks, -- Ming Lei -- 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 RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: FWIW, the plan looks fine to me. Just adding a couple of hints to simplify the implementation. Sarah Sharp sarah.a.sh...@linux.intel.com writes: Let's do this fix the right way, instead of wall papering over the issue. Here's what we should do: 1. Disable scatter-gather for the ax88179_178a driver when it's under an xHCI host. No need to make this conditional. SG is only enabled in the ax88179_178a driver if udev-bus-no_sg_constraint is true, so it applies only to xHCI hosts in the first place. Ah, so you're suggesting just reverting commit 3804fad45411b48233b48003e33a78f290d227c8 USBNET: ax88179_178a: enable tso if usb host supports sg dma? If I understand the problem correctly, the current issue is that xhci driver doesn't support the arbitrary dma length not well, but per XHCI spec, it should be supported, right? If the above is correct, reverting the commit isn't correct since there isn't any issue about the commit, so I suggest to disable the flag in xhci for the buggy devices, and it may be enabled again if the problem is fixed. 2. Revert the following commits: f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes. d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs 35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst 3. Dan and Mathias can work together to come up with an overall plan to change the xHCI driver architecture to be fully compliant with the TD fragment rules. That can be done over the next few kernel releases. The end result is that we don't destabilize storage or break userspace USB drivers, we don't break people's xHCI host controllers, the ax88179_178a USB ethernet devices still work under xHCI (a bit with worse performance), and other USB ethernet devices still get the performance improvement introduced in 3.12. No other usbnet drivers has enabled SG... Which is why you have only seen this problem with the ax88179_178a devices. So there is no performance improvement to keep. In my test environment, the patch does improve both throughput and cpu utilization, if you search the previous email for the patch, you can see the data. Thanks, -- Ming Lei -- 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: PROBLEM: usbnet / ax88179_178a: Panic in usb_hcd_map_urb_for_dma
On Mon, Jan 13, 2014 at 9:26 PM, David Laight david.lai...@aculab.com wrote: I believe all processing use the urb-num_sgs field to limit the number of entries. Common interfaces like dma_map_sg() and for_each_sg() limit their processing to nents entries, and the USB code use the value of urb-num_sgs for this parameter. Which mostly means that the sg_xxx functions are doing a whole load of unnecessary instructions and memory accesses... This probably has a lot to do with the significant difference in the cpu use for the usb3 and 'normal' ethernet interfaces. While each bit doesn't seem significant, they soon add up. If you plan to remove the 'nents' parameter, I am wondering if it is a good idea, because sg_nents() should be more heavy. Not mention sometimes the callers just want to map/unmap part of entries. Thanks, -- Ming Lei -- 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 net,stable] net: usbnet: fix SG initialisation
On Sat, Jan 11, 2014 at 6:10 AM, Bjørn Mork bj...@mork.no wrote: Commit 60e453a940ac (USBNET: fix handling padding packet) added an extra SG entry in case padding is necessary, but failed to update the initialisation of the list. This can cause list traversal to fall off the end of the list, resulting in an oops. Fixes: 60e453a940ac (USBNET: fix handling padding packet) Reported-by: Thomas Kear tho...@kear.co.nz Cc: Ming Lei ming@canonical.com Signed-off-by: Bjørn Mork bj...@mork.no --- I don't have the hardware to verify this fix. It would be good if someone could test it before it goes to stable... But in case this works, it should go into v3.12 stable. Yes, the problem can only be triggered when the zlp padding packet is needed, I remember I have a quick approach to reproduce and test the case, and I will do it when I return home tonight. Looks the fix is correct, and sorry for introducing the issue. Bjørn drivers/net/usb/usbnet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 8494bb53ebdc..aba04f561760 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1245,7 +1245,7 @@ static int build_dma_sg(const struct sk_buff *skb, struct urb *urb) return -ENOMEM; urb-num_sgs = num_sgs; - sg_init_table(urb-sg, urb-num_sgs); + sg_init_table(urb-sg, urb-num_sgs + 1); sg_set_buf(urb-sg[s++], skb-data, skb_headlen(skb)); total_len += skb_headlen(skb); Thanks, -- Ming Lei -- 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 net,stable] net: usbnet: fix SG initialisation
On Sat, Jan 11, 2014 at 5:16 PM, Ming Lei ming@canonical.com wrote: On Sat, Jan 11, 2014 at 6:10 AM, Bjørn Mork bj...@mork.no wrote: Commit 60e453a940ac (USBNET: fix handling padding packet) added an extra SG entry in case padding is necessary, but failed to update the initialisation of the list. This can cause list traversal to fall off the end of the list, resulting in an oops. Fixes: 60e453a940ac (USBNET: fix handling padding packet) Reported-by: Thomas Kear tho...@kear.co.nz Cc: Ming Lei ming@canonical.com Signed-off-by: Bjørn Mork bj...@mork.no Tested-by: Ming Lei ming@canonical.com On one of my arm box, I can reproduce the problem with below command every time: $iperf -c $SRV -l 2398 -n 1 #the 2nd tcp packet length is 1016 and the oops is same with Thomas's. After applying Oliver's patch, I still can see zlp padding packet is appended, but no oops any more. Thanks Oliver for fixing the problem. Thanks, -- Ming Lei -- 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: storage: fix compile warning
This patch should fix the below compile warning:
drivers/usb/storage/protocol.c: In function 'usb_stor_access_xfer_buf':
drivers/usb/storage/protocol.c:155:22: warning: comparison of distinct
pointer types lacks a cast [enabled by default]
Reported-by: kbuild test robot fengguang...@intel.com
Reported-by: Stephen Rothwell s...@canb.auug.org.au
---
drivers/usb/storage/protocol.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index f54e5fe..599d88f 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -152,7 +152,8 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
return cnt;
while (sg_miter_next(miter) cnt buflen) {
- unsigned int len = min(miter.length, buflen - cnt);
+ unsigned int len = min_t(unsigned, miter.length,
+ buflen - cnt);
if (dir == FROM_XFER_BUF)
memcpy(buffer + cnt, miter.addr, len);
--
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: storage: fix compile warning
On Tue, Dec 10, 2013 at 1:05 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:
On Tue, Dec 10, 2013 at 12:37:24PM +0800, Ming Lei wrote:
This patch should fix the below compile warning:
drivers/usb/storage/protocol.c: In function 'usb_stor_access_xfer_buf':
drivers/usb/storage/protocol.c:155:22: warning: comparison of distinct
pointer types lacks a cast [enabled by default]
Reported-by: kbuild test robot fengguang...@intel.com
Reported-by: Stephen Rothwell s...@canb.auug.org.au
---
drivers/usb/storage/protocol.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index f54e5fe..599d88f 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -152,7 +152,8 @@ unsigned int usb_stor_access_xfer_buf(unsigned char
*buffer,
return cnt;
while (sg_miter_next(miter) cnt buflen) {
- unsigned int len = min(miter.length, buflen - cnt);
+ unsigned int len = min_t(unsigned, miter.length,
s/unsigned,/unsigned int/
Actually, the two are same, and I may change to 'unsigned int' if
you require that, also signed-off-by is missed for the patch.
Thanks,
--
Ming Lei
--
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 v1] USB: storage: fix compile warning
This patch should fix the below compile warning:
drivers/usb/storage/protocol.c: In function 'usb_stor_access_xfer_buf':
drivers/usb/storage/protocol.c:155:22: warning: comparison of distinct
pointer types lacks a cast [enabled by default]
Reported-by: kbuild test robot fengguang...@intel.com
Reported-by: Stephen Rothwell s...@canb.auug.org.au
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/storage/protocol.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index f54e5fe..12e3c2f 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -152,7 +152,8 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
return cnt;
while (sg_miter_next(miter) cnt buflen) {
- unsigned int len = min(miter.length, buflen - cnt);
+ unsigned int len = min_t(unsigned int, miter.length,
+ buflen - cnt);
if (dir == FROM_XFER_BUF)
memcpy(buffer + cnt, miter.addr, len);
--
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 v1 1/2] lib/scatterlist: export sg_miter_skip()
On Sat, Nov 30, 2013 at 6:23 AM, Tejun Heo t...@kernel.org wrote: On Tue, Nov 26, 2013 at 12:43:37PM +0800, Ming Lei wrote: sg_copy_buffer() can't meet demand for some drrivers(such usb mass storage), so we have to use the sg_miter_* APIs to access sg buffer, then need export sg_miter_skip() for these drivers. The API is needed for converting to sg_miter_* APIs in USB storage driver for accessing sg buffer. Acked-by: Andrew Morton a...@linux-foundation.org Cc: FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp Cc: Tejun Heo t...@kernel.org Cc: Jens Axboe ax...@kernel.dk Signed-off-by: Ming Lei ming@canonical.com Reviewed-by: Tejun Heo t...@kernel.org Thanks. I suppose this should go through -mm? Last round, Andrew said it can be though whatever tree, so given dependency reason, it is better to merge via greg's tree. Greg, could you merge these two patches via your usb-next tree? Thanks, -- Ming Lei -- 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 v1 1/2] lib/scatterlist: export sg_miter_skip()
sg_copy_buffer() can't meet demand for some drrivers(such usb
mass storage), so we have to use the sg_miter_* APIs to access
sg buffer, then need export sg_miter_skip() for these drivers.
The API is needed for converting to sg_miter_* APIs in USB storage
driver for accessing sg buffer.
Acked-by: Andrew Morton a...@linux-foundation.org
Cc: FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp
Cc: Tejun Heo t...@kernel.org
Cc: Jens Axboe ax...@kernel.dk
Signed-off-by: Ming Lei ming@canonical.com
---
include/linux/scatterlist.h |1 +
lib/scatterlist.c |3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index adae88f..a964f72 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -345,6 +345,7 @@ struct sg_mapping_iter {
void sg_miter_start(struct sg_mapping_iter *miter, struct scatterlist *sgl,
unsigned int nents, unsigned int flags);
+bool sg_miter_skip(struct sg_mapping_iter *miter, off_t offset);
bool sg_miter_next(struct sg_mapping_iter *miter);
void sg_miter_stop(struct sg_mapping_iter *miter);
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index d16fa29..3a8e8e8 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -495,7 +495,7 @@ static bool sg_miter_get_next_page(struct sg_mapping_iter
*miter)
* true if @miter contains the valid mapping. false if end of sg
* list is reached.
*/
-static bool sg_miter_skip(struct sg_mapping_iter *miter, off_t offset)
+bool sg_miter_skip(struct sg_mapping_iter *miter, off_t offset)
{
sg_miter_stop(miter);
@@ -513,6 +513,7 @@ static bool sg_miter_skip(struct sg_mapping_iter *miter,
off_t offset)
return true;
}
+EXPORT_SYMBOL(sg_miter_skip);
/**
* sg_miter_next - proceed mapping iterator to the next mapping
--
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 2/2] USB: storage: use sg_miter_* APIs to access scsi buffer
On Fri, Nov 1, 2013 at 10:54 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Wed, 30 Oct 2013, Ming Lei wrote:
We have sg_miter_* APIs for accessing scsi sg buffer, so
use them to make code clean and bug free.
Hmmm. You could simply call sg_copy_buffer, if you didn't mind the
quadratic penalty for the sg_miter_skip operations.
I don't want to change all current callers now.
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -135,69 +135,43 @@ unsigned int usb_stor_access_xfer_buf(unsigned char
*buffer,
unsigned int buflen, struct scsi_cmnd *srb, struct scatterlist **sgptr,
unsigned int *offset, enum xfer_buf_dir dir)
{
- unsigned int cnt;
+ unsigned int cnt = 0;
struct scatterlist *sg = *sgptr;
+ struct sg_mapping_iter miter;
+ unsigned int nents = scsi_sg_count(srb);
- /* We have to go through the list one entry
- * at a time. Each s-g entry contains some number of pages, and
- * each page has to be kmap()'ed separately. If the page is already
- * in kernel-addressable memory then kmap() will return its address.
- * If the page is not directly accessible -- such as a user buffer
- * located in high memory -- then kmap() will map it to a temporary
- * position in the kernel's virtual address space.
- */
-
- if (!sg)
+ if (sg)
+ nents -= sg - scsi_sglist(srb);
This is definitely wrong. Scatterlist entries are not always stored in
a linear array. To do this properly, you would have to make the caller
keep track of the current value of nents.
Or better yet, have the caller store and pass the sg_mapping_iter
structure rather than sgptr and offset.
You are right, but looks we can use sg_nents(), which is easier, :-)
+ else
sg = scsi_sglist(srb);
- /* This loop handles a single s-g list entry, which may
- * include multiple pages. Find the initial page structure
- * and the starting offset within the page, and update
- * the *offset and **sgptr values for the next loop.
- */
- cnt = 0;
- while (cnt buflen sg) {
- struct page *page = sg_page(sg) +
- ((sg-offset + *offset) PAGE_SHIFT);
- unsigned int poff = (sg-offset + *offset) (PAGE_SIZE-1);
- unsigned int sglen = sg-length - *offset;
-
- if (sglen buflen - cnt) {
-
- /* Transfer ends within this s-g entry */
- sglen = buflen - cnt;
- *offset += sglen;
- } else {
+ if (dir == FROM_XFER_BUF)
+ sg_miter_start(miter, sg, nents, SG_MITER_FROM_SG);
+ else
+ sg_miter_start(miter, sg, nents, SG_MITER_TO_SG);
I find this style somewhat annoying. Maybe the compiler is smart
enough to optimize it, maybe not. In any case, I would prefer to see
if (dir == FROM_XFER_BUF)
sgdir = SG_MITER_FROM_SG;
else
sgdir = SG_MITER_TO_SG;
sg_miter_start(miter, nents, sgdir);
Or even:
sg_miter_start(miter, nents, (dir == FROM_XFER_BUF ?
SG_MITER_FROM_SG : SG_MITER_TO_SG));
Looks the above is fine.
- /* Transfer continues to next s-g entry */
- *offset = 0;
- sg = sg_next(sg);
- }
+ if (!sg_miter_skip(miter, *offset))
+ return cnt;
+
+ while (sg_miter_next(miter) cnt buflen) {
+ unsigned int len = min(miter.length, buflen - cnt);
+
+ if (dir == FROM_XFER_BUF)
+ memcpy(buffer + cnt, miter.addr, len);
+ else
+ memcpy(miter.addr, buffer + cnt, len);
- /* Transfer the data for all the pages in this
- * s-g entry. For each page: call kmap(), do the
- * transfer, and call kunmap() immediately after. */
- while (sglen 0) {
- unsigned int plen = min(sglen, (unsigned int)
- PAGE_SIZE - poff);
- unsigned char *ptr = kmap(page);
-
- if (dir == TO_XFER_BUF)
- memcpy(ptr + poff, buffer + cnt, plen);
- else
- memcpy(buffer + cnt, ptr + poff, plen);
- kunmap(page);
-
- /* Start at the beginning of the next page */
- poff = 0;
- ++page;
- cnt += plen;
- sglen -= plen;
+ if (*offset + len miter.piter.sg-length) {
+ *offset += len;
+ *sgptr = miter.piter.sg;
+ } else {
+ *offset = 0;
+ *sgptr = sg_next
Re: [PATCH 1/2] lib/scatterlist: export sg_miter_skip()
Hi Andrew, Thank you for the comment. On Fri, Nov 1, 2013 at 5:47 AM, Andrew Morton a...@linux-foundation.org wrote: Looks OK to me. Please include this in whatever tree is used to merge [PATCH 2/2] USB: storage: use sg_miter_* APIs to access scsi buffer. Greg and Alan, would you mind reviewing commenting on 2/2 and merge the two finally to usb-next tree if you are OK with it? Thanks, -- Ming Lei -- 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: storage: use sg_miter_* APIs to access scsi buffer
We have sg_miter_* APIs for accessing scsi sg buffer, so
use them to make code clean and bug free.
Cc: Matthew Dharm mdharm-...@one-eyed-alien.net
Cc: Alan Stern st...@rowland.harvard.edu
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/storage/protocol.c | 82 ++--
1 file changed, 28 insertions(+), 54 deletions(-)
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index 5dfb4c3..01697e5 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -135,69 +135,43 @@ unsigned int usb_stor_access_xfer_buf(unsigned char
*buffer,
unsigned int buflen, struct scsi_cmnd *srb, struct scatterlist **sgptr,
unsigned int *offset, enum xfer_buf_dir dir)
{
- unsigned int cnt;
+ unsigned int cnt = 0;
struct scatterlist *sg = *sgptr;
+ struct sg_mapping_iter miter;
+ unsigned int nents = scsi_sg_count(srb);
- /* We have to go through the list one entry
-* at a time. Each s-g entry contains some number of pages, and
-* each page has to be kmap()'ed separately. If the page is already
-* in kernel-addressable memory then kmap() will return its address.
-* If the page is not directly accessible -- such as a user buffer
-* located in high memory -- then kmap() will map it to a temporary
-* position in the kernel's virtual address space.
-*/
-
- if (!sg)
+ if (sg)
+ nents -= sg - scsi_sglist(srb);
+ else
sg = scsi_sglist(srb);
- /* This loop handles a single s-g list entry, which may
-* include multiple pages. Find the initial page structure
-* and the starting offset within the page, and update
-* the *offset and **sgptr values for the next loop.
-*/
- cnt = 0;
- while (cnt buflen sg) {
- struct page *page = sg_page(sg) +
- ((sg-offset + *offset) PAGE_SHIFT);
- unsigned int poff = (sg-offset + *offset) (PAGE_SIZE-1);
- unsigned int sglen = sg-length - *offset;
-
- if (sglen buflen - cnt) {
-
- /* Transfer ends within this s-g entry */
- sglen = buflen - cnt;
- *offset += sglen;
- } else {
+ if (dir == FROM_XFER_BUF)
+ sg_miter_start(miter, sg, nents, SG_MITER_FROM_SG);
+ else
+ sg_miter_start(miter, sg, nents, SG_MITER_TO_SG);
- /* Transfer continues to next s-g entry */
- *offset = 0;
- sg = sg_next(sg);
- }
+ if (!sg_miter_skip(miter, *offset))
+ return cnt;
+
+ while (sg_miter_next(miter) cnt buflen) {
+ unsigned int len = min(miter.length, buflen - cnt);
+
+ if (dir == FROM_XFER_BUF)
+ memcpy(buffer + cnt, miter.addr, len);
+ else
+ memcpy(miter.addr, buffer + cnt, len);
- /* Transfer the data for all the pages in this
- * s-g entry. For each page: call kmap(), do the
- * transfer, and call kunmap() immediately after. */
- while (sglen 0) {
- unsigned int plen = min(sglen, (unsigned int)
- PAGE_SIZE - poff);
- unsigned char *ptr = kmap(page);
-
- if (dir == TO_XFER_BUF)
- memcpy(ptr + poff, buffer + cnt, plen);
- else
- memcpy(buffer + cnt, ptr + poff, plen);
- kunmap(page);
-
- /* Start at the beginning of the next page */
- poff = 0;
- ++page;
- cnt += plen;
- sglen -= plen;
+ if (*offset + len miter.piter.sg-length) {
+ *offset += len;
+ *sgptr = miter.piter.sg;
+ } else {
+ *offset = 0;
+ *sgptr = sg_next(miter.piter.sg);
}
+ cnt += len;
}
- *sgptr = sg;
+ sg_miter_stop(miter);
- /* Return the amount actually transferred */
return cnt;
}
EXPORT_SYMBOL_GPL(usb_stor_access_xfer_buf);
--
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 1/2] lib/scatterlist: export sg_miter_skip()
sg_copy_buffer() can't meet demand for some drrivers(such usb
mass storage), so we have to use the sg_miter_* APIs to access
sg buffer, then need export sg_miter_skip() for these drivers.
The API is needed for converting to sg_miter_* APIs in USB storage
driver for accessing sg buffer.
Cc: Andrew Morton a...@linux-foundation.org,
Cc: FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp,
Cc: Tejun Heo t...@kernel.org,
Cc: Jens Axboe ax...@kernel.dk
Signed-off-by: Ming Lei ming@canonical.com
---
include/linux/scatterlist.h |1 +
lib/scatterlist.c |3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index adae88f..a964f72 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -345,6 +345,7 @@ struct sg_mapping_iter {
void sg_miter_start(struct sg_mapping_iter *miter, struct scatterlist *sgl,
unsigned int nents, unsigned int flags);
+bool sg_miter_skip(struct sg_mapping_iter *miter, off_t offset);
bool sg_miter_next(struct sg_mapping_iter *miter);
void sg_miter_stop(struct sg_mapping_iter *miter);
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index d16fa29..3a8e8e8 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -495,7 +495,7 @@ static bool sg_miter_get_next_page(struct sg_mapping_iter
*miter)
* true if @miter contains the valid mapping. false if end of sg
* list is reached.
*/
-static bool sg_miter_skip(struct sg_mapping_iter *miter, off_t offset)
+bool sg_miter_skip(struct sg_mapping_iter *miter, off_t offset)
{
sg_miter_stop(miter);
@@ -513,6 +513,7 @@ static bool sg_miter_skip(struct sg_mapping_iter *miter,
off_t offset)
return true;
}
+EXPORT_SYMBOL(sg_miter_skip);
/**
* sg_miter_next - proceed mapping iterator to the next mapping
--
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-storage: scsiglue: Changing the command result
On Thu, Sep 26, 2013 at 7:35 PM, Vishal Annapurve vannapu...@nvidia.com wrote: Hi, There was a recent commit in mainline for the scsi devices which do not respond properly to medium access command: commit18a4d0a22ed6c54b67af7718c305cd010f09ddf8 [SCSI] Handle disk devices which can not process medium access commands We have experienced several devices which fail in a fashion we do not currently handle gracefully in SCSI. After a failure these devices will respond to the SCSI primary command set (INQUIRY, TEST UNIT READY, etc.) but any command accessing the storage medium will time out. I came across a USB drive which showed similar problem and what I see is usb storage is still not able to cope with such devices properly. The control flow downwards is like: scsi_times_out -- Setting cmd-result as DID_TIME_OUT scsi_error_handler scsi_unjam_host scsi_eh_abort_cmds command_abort(sets US_FLIDX_TIMED_OUT for us-dflags calls stop_transport, and waits for)usb_stor_control_thread (which is waiting for transport call to return inside usb_stor_invoke_transport) both usb_stor_control_thread and usb_stor_invoke_transport check for us-dflags timed_out bit and set the result as DID_ABORT and signal completion for command_abort to complete .. sd_eh_action checks for cmd-result and finds out that it's DID_ABORT rather than DID_TIME_OUT. This patch updates the command result to be TIME_OUT explicitly before returning from command_abort in scsiglue.c. I would like to know if this patch can work out for such USB Storage devices? What would be the better way to do the same? Looks your diagnose is correct, and patch should be doable, the only side effect is that previous returned DID_ABORT in srb-result is replaced with DID_TIME_OUT now. Another way is to implement .eh_timed_out callback to return different error for the two failure, but looks not a big deal. Thanks, -- Ming Lei -- 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-storage: scsiglue: Changing the command result
On Wed, Oct 16, 2013 at 4:22 AM, Alan Stern st...@rowland.harvard.edu wrote: On Tue, 15 Oct 2013, Vishal Annapurve wrote: Hi Alan, USB storage maybe just has to say that the abort occurred. By setting the US_FLIDX_TIMED_OUT bit USB storage is getting signaled that the reason was time out and the command is being aborted. No. By setting the US_FLIDX_TIMED_OUT bit, usb-storage indicates that the command was aborted. This doesn't indicate anything about the reason for the abort. (Maybe this bit's name wasn't chosen very well.) Now, it's arguable whether to change the implication of US_FLIDX_TIMED_OUT bit for scsi - USB storage bridge or for entire usb storage I don't understand this. What's the difference between scsi - USB storage bridge and entire usb storage? Aren't they the same thing? Or maybe scsi has decided to abort so it should override the result. Of course the SCSI midlayer has decided to abort. That's the only way this bit can get set. But usb-storage doesn't know why SCSI decided to abort. usb-storage may know if it is caused by timeout via .eh_timed_out callback if it wants to know. Thanks, -- Ming Lei -- 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] usbnet: smsc95xx: Add device tree input for MAC address
On Thu, Oct 10, 2013 at 8:08 PM, Dan Murphy dmur...@ti.com wrote: You are correct I don't expect a match for PnP devices only devices that are hard wired. After thinking of it I should move the OF code below the EEPROM code as the EEPROM should take preference over the DT code. I will need to post V2 for that. Your patch still doesn't deal with two smsc95xx devices built in one board. The problem is a generic one, looks it is better to do it when OF supports discoverable bus. Thanks, -- Ming Lei -- 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] usbnet: smsc95xx: Add device tree input for MAC address
On Thu, Oct 10, 2013 at 8:47 PM, Dan Murphy dmur...@ti.com wrote: Is there a board that has 2 built in smsc devices? I don't know, maybe there isn't, but driver should be generic enough, and as I said it is a generic problem, and people are discussing it, so suggest to read previous discussions first before post your V2. Below link is one I remembered, but I think you can find more: https://lkml.org/lkml/2013/8/11/100 This is all dependent on what the dev.id is of udev. I am not sure why it depends on udev, and udev can't see the device util it is added to system. Thanks, -- Ming Lei -- 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] usbnet: smsc95xx: Add device tree input for MAC address
On Mon, Oct 7, 2013 at 1:31 AM, Dan Murphy dmur...@ti.com wrote: On 10/06/2013 10:05 AM, Ming Lei wrote: On Sat, Oct 5, 2013 at 2:25 AM, Dan Murphy dmur...@ti.com wrote: If the smsc95xx does not have a valid MAC address stored within the eeprom then a random number is generated. The MAC can also be set by uBoot but the smsc95xx does not have a way to read this. Create the binding for the smsc95xx so that uBoot can set the MAC and the code can retrieve the MAC from the modified DTB file. Suppose there are two smsc95xx usbnet devices connected to usb bus, and one is built-in, another is hotplug device, can your patch handle the situation correctly? Look at this line in the patch below sprintf(of_name, %s%i, SMSC95XX_OF_NAME, dev-udev-dev.id); I am appending the dev ID of the device to the of_name here. As long as init_mac_address is called, the dev.id and the uBoot entry match then yes. Currently, non-root-hub usb device is created and added into usb bus without any platform(device tree) knowledge, so you can't expect the match here. Also not mention the two smsc95xx devices may attach to two different usb host controllers(buses). Thanks, -- Ming Lei -- 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] usbnet: smsc95xx: Add device tree input for MAC address
On Sat, Oct 5, 2013 at 2:25 AM, Dan Murphy dmur...@ti.com wrote:
If the smsc95xx does not have a valid MAC address stored within
the eeprom then a random number is generated. The MAC can also
be set by uBoot but the smsc95xx does not have a way to read this.
Create the binding for the smsc95xx so that uBoot can set the MAC
and the code can retrieve the MAC from the modified DTB file.
Suppose there are two smsc95xx usbnet devices connected to usb bus, and
one is built-in, another is hotplug device, can your patch handle the situation
correctly?
Signed-off-by: Dan Murphy dmur...@ti.com
---
Documentation/devicetree/bindings/net/smsc95xx.txt | 17 ++
drivers/net/usb/smsc95xx.c | 24
2 files changed, 41 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/smsc95xx.txt
diff --git a/Documentation/devicetree/bindings/net/smsc95xx.txt
b/Documentation/devicetree/bindings/net/smsc95xx.txt
new file mode 100644
index 000..4c37280
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/smsc95xx.txt
@@ -0,0 +1,17 @@
+* Smart Mixed-Signal Connectivity (SMSC) 95xx Controller
+
+Required properties:
+None
+
+Optional properties:
+- mac-address - Read the mac address that was stored by uBoot
+- local-address - Read the mac address that was stored by uBoot
+- address - Read the mac address that was stored by uBoot
+
+Examples:
+
+smsc0: smsc95xx@0 {
+ /* Filled in by U-Boot */
+ mac-address = [ 00 00 00 00 00 00 ];
+};
+
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 3f38ba8..baee0bd 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -31,6 +31,9 @@
#include linux/crc32.h
#include linux/usb/usbnet.h
#include linux/slab.h
+#include linux/of.h
+#include linux/of_net.h
+
#include smsc95xx.h
#define SMSC_CHIPNAME smsc95xx
@@ -62,6 +65,8 @@
#define SUSPEND_ALLMODES (SUSPEND_SUSPEND0 | SUSPEND_SUSPEND1
| \
SUSPEND_SUSPEND2 | SUSPEND_SUSPEND3)
+#define SMSC95XX_OF_NAME /smsc95xx@
+
struct smsc95xx_priv {
u32 mac_cr;
u32 hash_hi;
@@ -767,6 +772,25 @@ static int smsc95xx_ioctl(struct net_device *netdev,
struct ifreq *rq, int cmd)
static void smsc95xx_init_mac_address(struct usbnet *dev)
{
+
+#ifdef CONFIG_OF
+ struct device_node *ap;
+ const char *mac = NULL;
+ char *of_name = SMSC95XX_OF_NAME;
+
+ sprintf(of_name, %s%i, SMSC95XX_OF_NAME, dev-udev-dev.id);
+ ap = of_find_node_by_path(of_name);
+ if (ap) {
+ mac = of_get_mac_address(ap);
+ if (is_valid_ether_addr(mac)) {
+ /* Device tree has a mac for this so use that */
+ memcpy(dev-net-dev_addr, mac, ETH_ALEN);
+ netif_dbg(dev, ifup, dev-net, MAC address read from
DTB\n);
+ return;
+ }
+ }
+#endif
+
/* try reading mac address from EEPROM */
if (smsc95xx_read_eeprom(dev, EEPROM_MAC_OFFSET, ETH_ALEN,
dev-net-dev_addr) == 0) {
--
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/
Thanks,
--
Ming Lei
--
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] memory mapping for usbfs (v0.4)
On Sat, Oct 5, 2013 at 4:34 AM, Alan Stern st...@rowland.harvard.edu wrote: On Fri, 4 Oct 2013, Markus Rechberger wrote: I was only testing reading the data so I didn't see any caching effects since I don't have a device or driver which I can send a lot data out. As far as I understand pgprot_noncached would only be required when sending data (writing data to the DMA'able buffer). No, applciation still may read obsolete data from the mapped buffer even after transfer from device is complete without any synchronization. This question is still open. The buffer should be cached. The userspace program will have to make sure that it doesn't try to access the buffer while DMA is in progress. As long as that restriction is obeyed, the USB core will take care of mapping the buffer for DMA (which flushes the cache). No, HCD mapping/unmapping can't deal with the problem since they use the kernel direct-mapped virtual address of the buffer to flush cache, but applications use the mapped virtual address, and CPU can use both the two virtual addresse to cache data, so it is probable that the transfer buffer can be cached in more than one locations by CPU for VIVT or VIPT-alias cache. So Markus takes the simplest way which uses nocahced mapping, but it may degrade performance on some ARCHs since it is reported that it is extremely slow to access non-cached memory on some ARMv7 SoCs. As I understand it, you wrote this in order to solve problems where memory couldn't be allocated for USB transfers. If the system is running short on memory, won't your USBDEVFS_ALLOC_MEMORY ioctl also fail? No, the buffers will be kept in a list and re-used (aside of that they are also kept in a list in devio.c and re-used). When the buffers are allocated initially there's no chance to fail this process during run-time. Why not? What if the memory is already almost full? The call to alloc_pages_exact() could fail. I agree this is less likely than a failure with the current system, but it is still possible. With CONFIG_COMPACTION enabled, it is hard to trigger the allocation failure, and actually on some ARCHs, size of kernel stack or page directory table for creating each process is 16KB. Thanks, -- Ming Lei -- 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] memory mapping for usbfs (v0.4)
On Sat, Oct 5, 2013 at 11:10 PM, Alan Stern st...@rowland.harvard.edu wrote: On Sat, 5 Oct 2013, Ming Lei wrote: The buffer should be cached. The userspace program will have to make sure that it doesn't try to access the buffer while DMA is in progress. As long as that restriction is obeyed, the USB core will take care of mapping the buffer for DMA (which flushes the cache). No, HCD mapping/unmapping can't deal with the problem since they use the kernel direct-mapped virtual address of the buffer to flush cache, but applications use the mapped virtual address, and CPU can use both the two virtual addresse to cache data, so it is probable that the transfer buffer can be cached in more than one locations by CPU for VIVT or VIPT-alias cache. So Markus takes the simplest way which uses nocahced mapping, but it may degrade performance on some ARCHs since it is reported that it is extremely slow to access non-cached memory on some ARMv7 SoCs. Then how do you suggest the cache be flushed? flush_cache_range() may be OK, and suggest to refer to Documentation/cachetlb.txt first. Thanks, -- Ming Lei -- 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] Memory mapping for USBFS
On Sun, Sep 29, 2013 at 7:51 PM, Markus Rechberger mrechber...@gmail.com wrote: I agree, but here not only small buffers are the problem, also latency. I will send another patch version which overrides the SG transfer once a preallocated buffer is submitted. The current patch will only use the pre-allocated buffers if the buffer size is smaller than 15-16k. If you put much attention to performance latency, I am wondering why you don't write a kernel mode usb driver to fix your problem? With usbfs and user mode driver, both performance and latency won't be very good: - URB can only be submitted in process context with context switch cost - URB complete is notified by signal, which might introduce much latency With one kernel mode driver, you can get better performance/latency much easily. Thanks, -- Ming Lei -- 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] Memory mapping for USBFS
On Sun, Sep 29, 2013 at 8:18 PM, Markus Rechberger mrechber...@gmail.com wrote: On Sun, Sep 29, 2013 at 2:07 PM, Ming Lei tom.leim...@gmail.com wrote: On Sun, Sep 29, 2013 at 7:51 PM, Markus Rechberger mrechber...@gmail.com wrote: I agree, but here not only small buffers are the problem, also latency. I will send another patch version which overrides the SG transfer once a preallocated buffer is submitted. The current patch will only use the pre-allocated buffers if the buffer size is smaller than 15-16k. If you put much attention to performance latency, I am wondering why you don't write a kernel mode usb driver to fix your problem? With usbfs and user mode driver, both performance and latency won't be very good: Userspace drivers work with all systems from Linux 2.6.15 on in our case. One driver for all. USBFS has been designed to write such drivers, and we are using it. If it does work from 2.6.15 on, why do you post the patch now? Is there any recent change which breaks your driver? - URB can only be submitted in process context with context switch cost - URB complete is notified by signal, which might introduce much latency With one kernel mode driver, you can get better performance/latency much easily. nearly all embedded systems are usually shipped without third party kernel modules and in some cases they don't even provide the kernel source (additionally said I do not want to provide kernel drivers for each system it's a big waste of time, not even thinking about the time trying to get the kernel sources for a system). You can submit patch and ask to merge the driver into kernel tree. We have our glue module which we could ship but once a manufacturer decides to update their system the game starts again the driver would not load (they do not ship any modules which do not belong to their system). The current problems are maybe less than 0.1% on our side but we also want to have them fixed. I mean with usbfs and user space driver, you can't expect very good performance and latency. I didn't review your patch in detail, but looks your patch doesn't handle cache problem, for example, user space may write data to mmap area, but the data may stay in cache and don't reach memory, so it might be be transferred to device mistakenly. Thanks, -- Ming Lei -- 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] memory mapping for usbfs (v0.4)
On Sun, Sep 29, 2013 at 10:02 PM, Markus Rechberger mrechber...@gmail.com wrote: This patch adds memory mapping support to USBFS for isochronous and bulk data transfers, it allows to pre-allocate usb transfer buffers. The CPU usage decreases 1-2% on my 1.3ghz U7300 notebook The CPU usage decreases 6-8% on an Intel Atom n270 when transferring 20mbyte/sec (isochronous), it should be more interesting to see those statistics on embedded systems where copying data is more expensive. X86 platform should be memory coherent, which means uncached mapping might not affect memory access performance from CPU, so you might need provide performance data on other non-coherent ARCH. Usage from userspace: allocation: rv = ioctl(priv-usbfd, USBDEVFS_ALLOC_MEMORY, mem); if (rv == 0) buffer = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, priv-usbfd, mem.offset); use the mapped buffer with urb-buffer. release: rv = munmap(buffer, size); memset(mem, 0x0, sizeof(struct usbdevfs_memory)); mem.buffer = buffer; rv = ioctl(priv-usbfd, USBDEVFS_RELEASE_MEMORY, mem); non freed memory buffers are collected and will be released when closing the node. I tested this patch with Bulk and Isochronous, with and without memory mapping (applications which don't use mmap will just fall back to the legacy mechanism). Version 0.3: * Removed comment * Renamed USBDEVFS_FREE_MEMORY to USBDEVFS_RELEASE_MEMORY * Clearing allocated memory Version 0.4: * overriding SG transfers once memory mapped buffers are allocated for BULK Why do you want to override SG transfer? * adding pgprot_noncached to ensure that the IO memory is not cached Accessing non-cached mapping on some ARCHs is extremely slow, so I an wondering if it is good way to map the area as non-cached. Thanks, -- Ming Lei -- 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] Memory mapping for USBFS
On Sat, Sep 28, 2013 at 9:38 PM, Alan Stern st...@rowland.harvard.edu wrote: Very few non-xHCI controllers can do DMA above the 4 GB limit. Yes, but I am wondering non-xHCI need this kind of zero copy optimization, since very few user space drivers complain or care performance or cpu utilization when devices attach to non xHCI. make sure this will happen? actually That can't be guaranteed but we can handle it with page bounce, just like block device. Obviously. But if we have to bounce the pages, it isn't zero-copy any more. Suppose the optimization is mainly for xHCI, there should be no such problem. The problem only exists when non-xHCI is used and system has more than 4GB memory, which looks not a mainstream configuration. I propose the idea only for comparing the two approaches, and each one has its own advantage and disadvantage, maybe the two can coexist. mmap approach: - interface is a bit complicated, each URB need usbfs to allocate one buffer - not easy to scale well if the buffer need to be very big for obtaining good performance direct i/o approach: - interface is simple, maybe passing O_DIRECT to open() should be enough - if HCD can't DMA to 4GB above memory, part of 4GB above pages need to be bounced. Actually I observed both throughput and cpu utilization can be improved with the 4GB of DMA limit on either 32bit arch or 64bit arch, wrt. direct I/O over usb mass storage block device. This may depend more on the host controller capabilities than on the CPU architecture. Yes, but for most cases, more than 4GB ram is seldom used in 32bit CPU. Thanks, -- Ming Lei -- 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] Memory mapping for USBFS
On Wed, Sep 25, 2013 at 3:12 AM, Markus Rechberger mrechber...@gmail.com wrote: This patch adds memory mapping support to USBFS for isochronous and bulk data transfers, it allows to pre-allocate usb transfer buffers. The CPU usage decreases 1-2% on my 1.3ghz U7300 notebook when transferring 20mbyte/sec, it should be more interesting to see those statistics on embedded systems where copying data is more expensive. Given USB3 is becoming popular and throughput is increased much, zero copy should be charming. And another approach is to use direct I/O method(SG DMA to pages allocated to user space directly), which should be more flexible, and user don't need to use mmap/munmap, so should be easier to use. At least, wrt. usb mass storage test, both CPU utilization and throughput can be improved with direct I/O. Thanks, -- Ming Lei -- 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] Memory mapping for USBFS
On Sat, Sep 28, 2013 at 10:29 AM, Alan Stern st...@rowland.harvard.edu wrote: On Sat, 28 Sep 2013, Ming Lei wrote: On Wed, Sep 25, 2013 at 3:12 AM, Markus Rechberger mrechber...@gmail.com wrote: This patch adds memory mapping support to USBFS for isochronous and bulk data transfers, it allows to pre-allocate usb transfer buffers. The CPU usage decreases 1-2% on my 1.3ghz U7300 notebook when transferring 20mbyte/sec, it should be more interesting to see those statistics on embedded systems where copying data is more expensive. Given USB3 is becoming popular and throughput is increased much, zero copy should be charming. And another approach is to use direct I/O method(SG DMA to pages allocated to user space directly), which should be more flexible, and user don't need to use mmap/munmap, so should be easier to use. At least, wrt. usb mass storage test, both CPU utilization and throughput can be improved with direct I/O. For zero-copy to work, on many systems the pages have to be allocated in the first 4 GB of physical memory. How can the userspace program It depends if device can DMA to/from 4GB above physical memory. make sure this will happen? That can't be guaranteed but we can handle it with page bounce, just like block device. Actually I observed both throughput and cpu utilization can be improved with the 4GB of DMA limit on either 32bit arch or 64bit arch, wrt. direct I/O over usb mass storage block device. Thanks, -- Ming Lei -- 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 v1] USBNET: fix handling padding packet
Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
if the usb host controller is capable of building packet from
discontinuous buffers, but missed handling padding packet when
building DMA SG.
This patch attachs the pre-allocated padding packet at the
end of the sg list, so padding packet can be sent to device
if drivers require that.
Reported-by: David Laight david.lai...@aculab.com
Acked-by: Oliver Neukum oli...@neukum.org
Signed-off-by: Ming Lei ming@canonical.com
---
v1:
- fix bug in case of dev-can_dma_sg and !urb-num_sgs
---
drivers/net/usb/usbnet.c | 27 +--
include/linux/usb/usbnet.h |1 +
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 7b331e6..bf94e10 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb, struct
urb *urb)
if (num_sgs == 1)
return 0;
- urb-sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
+ /* reserve one for zero packet */
+ urb-sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
+ GFP_ATOMIC);
if (!urb-sg)
return -ENOMEM;
@@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (build_dma_sg(skb, urb) 0)
goto drop;
}
- entry-length = length = urb-transfer_buffer_length;
+ length = urb-transfer_buffer_length;
/* don't assume the hardware handles USB_ZERO_PACKET
* NOTE: strictly conforming cdc-ether devices should expect
@@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (length % dev-maxpacket == 0) {
if (!(info-flags FLAG_SEND_ZLP)) {
if (!(info-flags FLAG_MULTI_PACKET)) {
- urb-transfer_buffer_length++;
- if (skb_tailroom(skb)) {
+ length++;
+ if (skb_tailroom(skb) !urb-num_sgs) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
- }
+ } else if (urb-num_sgs)
+ sg_set_buf(urb-sg[urb-num_sgs++],
+ dev-padding_pkt, 1);
}
} else
urb-transfer_flags |= URB_ZERO_PACKET;
}
+ entry-length = urb-transfer_buffer_length = length;
spin_lock_irqsave(dev-txq.lock, flags);
retval = usb_autopm_get_interface_async(dev-intf);
@@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
usb_kill_urb(dev-interrupt);
usb_free_urb(dev-interrupt);
+ kfree(dev-padding_pkt);
free_netdev(net);
}
@@ -1679,9 +1685,16 @@ usbnet_probe (struct usb_interface *udev, const struct
usb_device_id *prod)
/* initialize max rx_qlen and tx_qlen */
usbnet_update_max_qlen(dev);
+ if (dev-can_dma_sg !(info-flags FLAG_SEND_ZLP)
+ !(info-flags FLAG_MULTI_PACKET)) {
+ dev-padding_pkt = kzalloc(1, GFP_KERNEL);
+ if (!dev-padding_pkt)
+ goto out4;
+ }
+
status = register_netdev (net);
if (status)
- goto out4;
+ goto out5;
netif_info(dev, probe, dev-net,
register '%s' at usb-%s-%s, %s, %pM\n,
udev-dev.driver-name,
@@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct
usb_device_id *prod)
return 0;
+out5:
+ kfree(dev-padding_pkt);
out4:
usb_free_urb(dev-interrupt);
out3:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 9cb2fe8..e303eef 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -42,6 +42,7 @@ struct usbnet {
struct usb_host_endpoint *status;
unsignedmaxpacket;
struct timer_list delay;
+ const char *padding_pkt;
/* protocol/interface state */
struct net_device *net;
--
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: Cannot shutdown power use from built in webcam in thinkpad T530 questions]
On Thu, Sep 19, 2013 at 12:35 PM, Marc MERLIN m...@merlins.org wrote: gandalfthegreat:/sys/bus/usb/devices/3-1.6/power# grep . * active_duration:61227648 async:enabled autosuspend:2 autosuspend_delay_ms:2000 connected_duration:66830880 control:auto level:auto persist:1 runtime_active_kids:0 runtime_active_time:18870052 runtime_enabled:enabled runtime_status:active runtime_suspended_time:5324088 runtime_usage:0 The uvc device should have been put in auto-suspend state if no application is using the device. You can check if there is application which is using the device by 'lsof | grep /dev/video'. Also looks power usage estimation of 'power top' should be used when battery provides power, and the data isn't correct if power is provided from power adapter, if I remember correctly. Thanks, -- Ming Lei -- 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] USBNET: fix handling padding packet
On Thu, Sep 19, 2013 at 3:18 PM, Bjørn Mork bj...@mork.no wrote: Oliver Neukum oneu...@suse.de writes: On Wed, 2013-09-18 at 20:56 +0200, Bjørn Mork wrote: Oliver Neukum oneu...@suse.de wrote: No, USB 3.0 uses no companion controllers, so you can have devices of any speed connected to it. Ah, right. I don't own such modern hardware, but I should have known this anyway. This still doesn't change the fact that the driver is brand new for brand new devices. I believe we should assume such devices will support ZLPs unless we have documentation stating anything else. For such devices we might assume it. But how does that matter for generic code? The code isn't generic yet. Most of it is placed inside the ax88179_178a minidriver. No, the patch doesn't touch code of ax99179_178a. And it is really generic to fix the padding in case of dma sg. But I do agree that adding this padding support can be seen as one step towards making the code generic. So if you really anticipate this being enabled for e.g. the ECM class driver, then yes, padding must be supported. 1byte padding frame is already supported by usbnet before, isn't it? I would have had less trouble understanding the value if this patch was part of a generalisation series, though... As my below test on ax99179_178a, I believe the patch should fix padding for dma sg, but need a little update, and I will send out v1 later: $ping -s 974 another_machine #from host with ax99179_178a attached If FLAG_SEND_ZLP is set for ax99179_178a, the above ping won't work any more either on USB3.0 or USB 2.0 host controller. So don't assume that these brand new devices can support ZLP well. As any kind of device may be connected to XHCI, the sg code is relevant for every driver. And I certainly don't want trouble for older devices' drivers converted to sg. I wonder what the gain of that really is? Yes, I can see the advantage of making the class drivers more effective. But padding is only relevant for the ECM class, isn't it? And are there any ECM class devices where SG support matters? Why is padding only relevant for the ECM? Of course, other devices need it too, such as asix devices. Thanks, -- Ming Lei -- 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] USBNET: fix handling padding packet
On Wed, Sep 18, 2013 at 9:59 PM, Bjørn Mork bj...@mork.no wrote:
Ming Lei ming@canonical.com writes:
Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
if the usb host controller is capable of building packet from
discontinuous buffers, but missed handling padding packet when
building DMA SG.
This patch attachs the pre-allocated padding packet at the
end of the sg list, so padding packet can be sent to device
if drivers require that.
Reported-by: David Laight david.lai...@aculab.com
Cc: Oliver Neukum oli...@neukum.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/usb/usbnet.c | 27 +--
include/linux/usb/usbnet.h |1 +
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 7b331e6..4a9bed3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb,
struct urb *urb)
if (num_sgs == 1)
return 0;
- urb-sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
+ /* reserve one for zero packet */
+ urb-sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
+ GFP_ATOMIC);
if (!urb-sg)
return -ENOMEM;
@@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (build_dma_sg(skb, urb) 0)
goto drop;
}
- entry-length = length = urb-transfer_buffer_length;
+ length = urb-transfer_buffer_length;
/* don't assume the hardware handles USB_ZERO_PACKET
* NOTE: strictly conforming cdc-ether devices should expect
@@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (length % dev-maxpacket == 0) {
if (!(info-flags FLAG_SEND_ZLP)) {
if (!(info-flags FLAG_MULTI_PACKET)) {
- urb-transfer_buffer_length++;
- if (skb_tailroom(skb)) {
+ length++;
+ if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
- }
+ } else if (dev-can_dma_sg)
+ sg_set_buf(urb-sg[urb-num_sgs++],
+ dev-padding_pkt, 1);
}
} else
urb-transfer_flags |= URB_ZERO_PACKET;
}
+ entry-length = urb-transfer_buffer_length = length;
spin_lock_irqsave(dev-txq.lock, flags);
retval = usb_autopm_get_interface_async(dev-intf);
@@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
usb_kill_urb(dev-interrupt);
usb_free_urb(dev-interrupt);
+ kfree(dev-padding_pkt);
free_netdev(net);
}
@@ -1679,9 +1685,16 @@ usbnet_probe (struct usb_interface *udev, const
struct usb_device_id *prod)
/* initialize max rx_qlen and tx_qlen */
usbnet_update_max_qlen(dev);
+ if (dev-can_dma_sg !(info-flags FLAG_SEND_ZLP)
+ !(info-flags FLAG_MULTI_PACKET)) {
+ dev-padding_pkt = kzalloc(1, GFP_KERNEL);
+ if (!dev-padding_pkt)
+ goto out4;
+ }
+
status = register_netdev (net);
if (status)
- goto out4;
+ goto out5;
netif_info(dev, probe, dev-net,
register '%s' at usb-%s-%s, %s, %pM\n,
udev-dev.driver-name,
@@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct
usb_device_id *prod)
return 0;
+out5:
+ kfree(dev-padding_pkt);
out4:
usb_free_urb(dev-interrupt);
out3:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 9cb2fe8..e303eef 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -42,6 +42,7 @@ struct usbnet {
struct usb_host_endpoint *status;
unsignedmaxpacket;
struct timer_list delay;
+ const char *padding_pkt;
/* protocol/interface state */
struct net_device *net;
The code handling the frame padding was already unecessarily complex
IMHO, and this does not improve it...
Are you really sure that the one driver/device using this really need
the padding byte? If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary. As the comment in front of the padding code says:
At least for the effected driver of ax88179, the padding packet is needed
since the driver does set the padding flag in the header, see
ax88179_tx_fixup().
strictly conforming cdc-ether devices should expect
Re: [PATCH] USBNET: fix handling padding packet
On Wed, Sep 18, 2013 at 11:52 PM, Bjørn Mork bj...@mork.no wrote:
Ming Lei ming@canonical.com writes:
Are you really sure that the one driver/device using this really need
the padding byte? If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary. As the comment in front of the padding code says:
At least for the effected driver of ax88179, the padding packet is needed
since the driver does set the padding flag in the header, see
ax88179_tx_fixup().
Yes, I noticed that the driver doesn't set the flag. I just didn't put
too much into that. I don't think that necessarily means that the
padding is needed. It probably just means that the driver worked with
the default padding enabled, so the author left it there.
This flag should really have been inverted. ZLP should be the default
for all new usbnet drivers.
In theory, it is, but who knows the reality.
Why don't you test it on the device you tested the SG patch with? I am
pretty sure it works just fine using proper ZLP transfer termination.
I should have planned to test it, but didn't know how to build TCP TSO
to make the whole frame length plus 8 dividable by 1024.
Could you or other guys share how to build such packet so that I can
do the test?
Once we can confirm the padding isn't needed, we can remove the
padding flag. But if the padding flag is still there, this patch should be
dropped.
strictly conforming cdc-ether devices should expect the ZLP here
There shouldn't be any problems requiring this conformance as a
precondition for allowing SG. The requirements are already strict.
There is no reason to forbid DMA SG for one driver which requires
padding, right?
Yes there is: Added complexity for everybody, based on a combination of
features which just does not make any sense.
No modern device should need the padding. No old device will be able to
use the SG feature as implemented. You only enable it on USB3, don't
you? If this feature is restricted to USB3 capable devices, then it most
certainly can be restricted to ZLP capable devices with absolutely no
difference in the resulting set of supported devices.
See above, if we can prove that padding isn't needed, we can remove
the padding, then the patch can be dropped. If we can't, the patch is needed.
Anyway, if you want to keep the padding for SG then maybe this will work
and allow you to drop the extra struct usbnet field and allocation:
if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
} else if (dev-can_dma_sg) {
sg_set_buf(urb-sg[urb-num_sgs++], skb-data,
1);
}
I.e. cheat and use the skb-data buffer twice, if that is allowed? The
actual value of the padding byte should not matter, I believe?
If so, we can remove the assignment of zero to skb-data[skb-len],
but probably it might cause regression, that is why I wouldn't like to
do that. Also looks introducing one extra dev-padding_frame doesn't
cause much complexity.
Thanks
--
Ming Lei
--
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] USBNET: fix handling padding packet
Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
if the usb host controller is capable of building packet from
discontinuous buffers, but missed handling padding packet when
building DMA SG.
This patch attachs the pre-allocated padding packet at the
end of the sg list, so padding packet can be sent to device
if drivers require that.
Reported-by: David Laight david.lai...@aculab.com
Cc: Oliver Neukum oli...@neukum.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/usb/usbnet.c | 27 +--
include/linux/usb/usbnet.h |1 +
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 7b331e6..4a9bed3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb, struct
urb *urb)
if (num_sgs == 1)
return 0;
- urb-sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
+ /* reserve one for zero packet */
+ urb-sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
+ GFP_ATOMIC);
if (!urb-sg)
return -ENOMEM;
@@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (build_dma_sg(skb, urb) 0)
goto drop;
}
- entry-length = length = urb-transfer_buffer_length;
+ length = urb-transfer_buffer_length;
/* don't assume the hardware handles USB_ZERO_PACKET
* NOTE: strictly conforming cdc-ether devices should expect
@@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (length % dev-maxpacket == 0) {
if (!(info-flags FLAG_SEND_ZLP)) {
if (!(info-flags FLAG_MULTI_PACKET)) {
- urb-transfer_buffer_length++;
- if (skb_tailroom(skb)) {
+ length++;
+ if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
- }
+ } else if (dev-can_dma_sg)
+ sg_set_buf(urb-sg[urb-num_sgs++],
+ dev-padding_pkt, 1);
}
} else
urb-transfer_flags |= URB_ZERO_PACKET;
}
+ entry-length = urb-transfer_buffer_length = length;
spin_lock_irqsave(dev-txq.lock, flags);
retval = usb_autopm_get_interface_async(dev-intf);
@@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
usb_kill_urb(dev-interrupt);
usb_free_urb(dev-interrupt);
+ kfree(dev-padding_pkt);
free_netdev(net);
}
@@ -1679,9 +1685,16 @@ usbnet_probe (struct usb_interface *udev, const struct
usb_device_id *prod)
/* initialize max rx_qlen and tx_qlen */
usbnet_update_max_qlen(dev);
+ if (dev-can_dma_sg !(info-flags FLAG_SEND_ZLP)
+ !(info-flags FLAG_MULTI_PACKET)) {
+ dev-padding_pkt = kzalloc(1, GFP_KERNEL);
+ if (!dev-padding_pkt)
+ goto out4;
+ }
+
status = register_netdev (net);
if (status)
- goto out4;
+ goto out5;
netif_info(dev, probe, dev-net,
register '%s' at usb-%s-%s, %s, %pM\n,
udev-dev.driver-name,
@@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct
usb_device_id *prod)
return 0;
+out5:
+ kfree(dev-padding_pkt);
out4:
usb_free_urb(dev-interrupt);
out3:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 9cb2fe8..e303eef 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -42,6 +42,7 @@ struct usbnet {
struct usb_host_endpoint *status;
unsignedmaxpacket;
struct timer_list delay;
+ const char *padding_pkt;
/* protocol/interface state */
struct net_device *net;
--
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: usbnet transmit path problems
On Mon, Sep 16, 2013 at 4:13 PM, Oliver Neukum oli...@neukum.org wrote: On Thu, 2013-09-12 at 09:56 +0800, Ming Lei wrote: On Thu, Sep 12, 2013 at 12:05 AM, David Laight david.lai...@aculab.com wrote: Patches are always welcome, :-) Indeed, I think your patch, if no better alternatives come up soon, should be taken. OK, will submit it later. Thanks, -- Ming Lei -- 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: usbnet transmit path problems
On Wed, Sep 11, 2013 at 8:56 PM, David Laight david.lai...@aculab.com wrote: 2) If 'length % dev-maxpacket == 0' for a multi-fragment packet then the extra byte isn't added correctly (the code probably falls off the end of the scatter-gather list). Indeed. Ming Lei, should usbnet handle this in the sg case or better leave it to the subdriver you introduced this for? Is the ZLP issue a problem with the host or with the target? Sorry, what do you mean the ZLP issue here? I understand Oliver thinks one commit from me may break ZLP handling, are you discussing this problem? If not, could you explain it in a bit detail? If it is a host problem then the necessity comes from the host, but the fix needs to be target dependant. If it is a common target problem then generic code can apply a common fix. All usbnet device should have sent one ZLP in case the size of bulk out transfer can be divided by max packet size, but the one byte transfer might be introduced for avoiding some target problem (can't deal with zlp well), as said by David, see below discussion: http://marc.info/?l=linux-usbm=127067487604112w=2 AFICT there are at least 3 fixes: 1) Extend the ethernet frame by one byte and hope the receiving system doesn't object to the padding. This is probably the only option if tx_fixup() doesn't add a header. 2) Put the ethernet frame length in the header and have the target discard the added pad byte (ax88179_178a.c). 3) Add a second zero-length frame in the same USB data block (ax88172a.c). Why do we need the above 3 fixes? The patch in my last email can fix the problem which is introduced recently, can't it? Only the third requires that tx_fixup() append to the packet. For the other two actual pad can be added by usbnet. IMO, it should be handled by usbnet, could you comment on below patch? Seems excessive to kmalloc() one byte! It isn't strange, many usb drivers have to do that(maybe kmalloc two or three, or four bytes) since the buffer is involved into DMA. If you can't assume that the 'dev' structure itself can be dma'd from allocate the extra byte in the sg list. It is better to always obey rule of DMA-API, so don't do that since one extra kmalloc() per device is needed, even though we can allocate only one global buffer for this purpose. Thanks, -- Ming Lei -- 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: usbnet transmit path problems
On Thu, Sep 12, 2013 at 12:05 AM, David Laight david.lai...@aculab.com wrote: On Wed, Sep 11, 2013 at 8:56 PM, David Laight david.lai...@aculab.com wrote: 2) If 'length % dev-maxpacket == 0' for a multi-fragment packet then the extra byte isn't added correctly (the code probably falls off the end of the scatter-gather list). Indeed. Ming Lei, should usbnet handle this in the sg case or better leave it to the subdriver you introduced this for? Is the ZLP issue a problem with the host or with the target? Sorry, what do you mean the ZLP issue here? I understand Oliver thinks one commit from me may break ZLP handling, are you discussing this problem? If not, could you explain it in a bit detail? I was thinking of the general ZLP problem. IMO the current approach should be general enough. If it is a host problem then the necessity comes from the host, but the fix needs to be target dependant. If it is a common target problem then generic code can apply a common fix. All usbnet device should have sent one ZLP in case the size of bulk out transfer can be divided by max packet size, but the one byte transfer might be introduced for avoiding some target problem (can't deal with zlp well), as said by David, see below discussion: http://marc.info/?l=linux-usbm=127067487604112w=2 AFAICT the code avoids sending a zero length packet (that would terminate a USB bulk transfer packet) by increasing the length of the bulk packet by (at least) one byte. No, the code on the link supports URB_ZERO_PACKET which should be the decent approach for the problem. AFICT there are at least 3 fixes: 1) Extend the ethernet frame by one byte and hope the receiving system doesn't object to the padding. This is what usbnet is doing at default if both FLAG_SEND_ZLP and FLAG_MULTI_PACKET aren't set. This is probably the only option if tx_fixup() doesn't add a header. 2) Put the ethernet frame length in the header and have the target discard the added pad byte (ax88179_178a.c). That is probably because the target need the padding flag, so that it can skip the one byte padding frame. 3) Add a second zero-length frame in the same USB data block (ax88172a.c). Actually it is a 4byte frame added by the subdriver, instead of adding one zlp. Since the subdriver adds the termination packet by itself, usbnet won't handle the case at all. There is no much difference among the 3 appoaches, all of which take the padding trick, and the difference is that if padding flag is required, and if the padding length is one byte or others, and both are device-dependent. So current approach is still general enough to cover all cases, isn't it? Why do we need the above 3 fixes? The patch in my last email can fix the problem which is introduced recently, can't it? I meant there are 3 ways of avoiding the ZLP, each driver will pick one of them. All the 3 ways are basically same or very similar, and the difference is that drivers may need the padding flag or take different length termination packet. I've just looked at all the drivers in net/usb. It doesn't look like they all handle fragmented skb, shared skb, or ZLP properly. A lot of common code could be removed if usbnet knew the size of the header and allocated it before calling tx_fixup(). Patches are always welcome, :-) None of this is helping me sort out why netperf udp rr tests with burst 19 are losing all the packets at once :-( If you can share your test case, guys in list may help you to troubleshoot it, :-) Thanks, -- Ming Lei -- 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 v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Thu, Aug 29, 2013 at 12:05 AM, Alan Stern st...@rowland.harvard.edu wrote: On Wed, 28 Aug 2013, Ming Lei wrote: Think about it this way: Why did you write the USB: EHCI: improve interrupt qh unlink patch in the first place? Essentially the same reason applies to uhci-hcd and ohci-hcd, and they will need similar patches. Right, but drivers which submit URBs from tasklet/workque/... scheduled from complete() may need these patches too, even without giveback URB in complete() change. That's a totally separate issue. This email thread is about changes to ehci-hcd, not bugs in other drivers. Drivers that submit isochronous URBs from tasklets/workqueues/whatever without URB_ISO_ASAP are broken and need to be fixed. You have said the same problem exists on these drivers already without giveback in tasklet patch, so for the problem and solution, there is no difference. What do you mean? We can't fix those other drivers by changing ehci-hcd. It's their fault if they misuse the isochronous API. OK, so submitting isoc URB has to be done from completion handler directly except for the start URBs, otherwise the usage violates isochronous API. Thanks, -- Ming Lei -- 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: [RFC 2/3] EHCI: convert the IRQ handler to a tasklet
On Tue, Aug 27, 2013 at 10:39 PM, Alan Stern st...@rowland.harvard.edu wrote: On Tue, 27 Aug 2013, Ming Lei wrote: Yes. A new spinlock would be needed to synchronize the top half and the bottom half. The same spinlock would also be used to avoid scheduling the tasklet when it is already running, like in your implementation. Then every HCD need to copy these kind of implementation... Yes. The pattern would be pretty much the same in each case. So when the above implementation is required in each HCDs change, I am wondering if it is 'pretty easy'. I think it is pretty easy for each HCD. Changing all of them would be a large job. Still not sure it is pretty easy since extra lock things have to be considered: (such as, order between two locks, disabling irq and acquiring lock) Those things would definitely need to be considered. But doing them properly wouldn't require much code. Even though interrupts are masked, the tasklet can still check the EHCI status register to see if new events have occurred while the tasklet was running, as I described above. The tasklet could do this check before returning. Yes, the tasklet can do it but some events(IAA, connection, fata error, ...) are handled with delay. That's true. It's also true that the existing driver handles them with delay. If an IAA, connection, or fatal error event occurs while ehci_work() is running, the event won't be handled until after ehci_work() finishes. If fatal error can be reported early, ehci_work() can scan and handle qh/iTD/siTD easily(quickly) since ehci state is checked at many places. I agree that masking interrupts while the bottom half runs would increase this delay, but I don't think it matters much. For example, consider disconnect events. Leaving interrupts masked might delay the event report for 10 ms (probably a lot less). But compare that to what happens when a device disconnects from an external hub -- hubs are polled for port status changes with an interval of 128 ms! By comparison, a 10-ms delay for the root hub is unimportant. More things done in top half, the change will become more complicated since more synchronizations need to consider. Not at all. ehci-lock will synchronize things perfectly well. It isn't good to hold ehci-lock in ehci_irq(), otherwise, ehci_irq has to spin on this lock when is held in tasklet. That was my point. We are in agreement that it is bad for the top half and the bottom half both to acquire ehci-lock. Your solution is to put everything but the givebacks (which don't need the lock) in the top half, whereas my solution is to put everything in the bottom half. But now you're complaining because that means some of the work won't get done in the top half! You can't have it both ways. If the lock isn't used in both places then the top half has to do either all or none of the work. It can be done with extra complication introduced, but that is we don't want to see. I prefer to enabling EHCI interrupt during tasklet handling. What for? It seems likely that the top half would have to acquire the So we can respond some events(IAA, fatal error, connection change) quickly. For example, when fatal error happened, ehci transfer descriptors might be written incorrectly by host, so it is better to let tasklet see it asap and handle transfer effectively(maybe correctly). You haven't thought this through. _Every_ QH and TD written by the host controller eventually gets scanned by ehci-hcd. This is true whether or not a fatal error occurs. The existing driver doesn't do anything special about incorrect TDs, and it never has. So why should we have to start doing it now? Similar considerations apply to connect and disconnect handling. Suppose a connect event occurs while ehci_work() is running. Suppose that the event can be handled without acquiring ehci-lock, so that the event is reported to usbcore immediately. What would happen next? Answer: Before doing anything else, khubd would issue a Get-Port-Status request, which acquires ehci-lock! Thus the response to the connect event would end up being delayed anyway. The one disadvantage to leaving interrupts masked while the tasklet runs is that new events won't be detected until all the existing givebacks are finished. In the old driver, new events could be detected as soon as the next giveback occurred, because the lock would be released then. In the end, this comes down to a decision about priorities. Do you want to give higher priority to new events or to givebacks? Overall, I don't think it matters. If possible, the events should be put higher priority, like events handling in ehci_irq() now, but as you said, it may not matter. So from the discussion, both approaches are pretty much same I have no objection if anyone plan to implement this approach. Thanks, -- Ming Lei -- To unsubscribe from this list: send
Re: [PATCH v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Wed, Aug 28, 2013 at 12:24 PM, Ming Lei tom.leim...@gmail.com wrote:
On Wed, 28 Aug 2013 12:02:03 +0800
Ming Lei ming@canonical.com wrote:
Actually the problem only happened when underrun without
URB_ISO_ASAP.
I have another fix which uses rebase trick and is simper(less change), could
you comment on the attachment patch?
Sorry, please ignore last attachment patch, and comment on the below one:
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 83be03f..4a3ddc9 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -1370,6 +1370,21 @@ sitd_slot_ok (
return 1;
}
+/* rebase in case of underun without ISO_ASAP together */
+static void iso_stream_rebase(struct ehci_hcd *ehci,
+ struct ehci_iso_stream *stream, u32 mod)
+{
+ u32 i;
+
+ if (ehci-last_base == -1)
+ return;
+
+ for (i = ehci-last_base; i != ehci-last_iso_frame;
+ i = (i + 1) (mod - 1))
+ if ((stream-next_uframe (mod - 1)) == i)
+ ehci-last_iso_frame = i;
+}
mod should switch to ehci-periodic_size, and stream-next_uframe
should convert to frame.
Also if the URB has been completed('now' is behind 'start + span'), the URB
need to complete immediately, or rescan after linking simply.
I will prepare for another one later.
+
/*
* This scheduler plans almost as far into the future as it has actual
* periodic schedule slots. (Affected by TUNE_FLS, which defaults to
@@ -1409,7 +1424,9 @@ iso_stream_schedule (
* (irq delays etc). If there are, the behavior depends on
* whether URB_ISO_ASAP is set.
*/
- if (likely (!list_empty (stream-td_list))) {
+ if (likely (!list_empty (stream-td_list) ||
+ hcd_complete_in_progress(
+ ehci_to_hcd(ehci), urb))) {
/* Take the isochronous scheduling threshold into account */
if (ehci-i_thresh)
@@ -1417,6 +1434,10 @@ iso_stream_schedule (
else
next = (now + 2 + 7) ~0x07; /* full frame cache */
+ if (list_empty(stream-td_list)
+ !(urb-transfer_flags URB_ISO_ASAP))
+ iso_stream_rebase(ehci, stream, mod);
+
/*
* Use ehci-last_iso_frame as the base. There can't be any
* TDs scheduled for earlier than that.
@@ -1517,6 +1538,7 @@ iso_stream_schedule (
/* Make sure scan_isoc() sees these */
if (ehci-isoc_count == 0)
ehci-last_iso_frame = now 3;
+ ehci-last_base = -1;
return 0;
fail:
@@ -2255,6 +2277,9 @@ static void scan_isoc(struct ehci_hcd *ehci)
}
ehci-now_frame = now_frame;
+ if (ehci-last_base == -1)
+ ehci-last_base = ehci-last_iso_frame;
+
frame = ehci-last_iso_frame;
for (;;) {
union ehci_shadow q, *q_p;
Thanks,
--
Ming Lei
--
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: [RFC 3/3] EHCI: handle late isochronous submissions
, *q_p;
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 2822e79..279e182 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -149,6 +149,7 @@ struct ehci_hcd { /* one per controller */
unsignedintr_unlink_wait_cycle;
unsignedintr_unlink_cycle;
unsignednow_frame; /* frame from HC hardware */
+ unsignedlast_base; /* previous 'last_iso_frame' */
unsignedlast_iso_frame; /* last frame scanned for iso */
unsignedintr_count; /* intr activity count */
unsignedisoc_count; /* isoc activity count */
Thanks,
--
Ming Lei
--
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: [RFC 3/3] EHCI: handle late isochronous submissions
On Thu, Aug 29, 2013 at 12:23 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Wed, 28 Aug 2013, Ming Lei wrote:
Below is the approach I proposed(mentioned in another thread), which should
be
simper than this one, any comments?
drivers/usb/host/ehci-sched.c | 53
++---
drivers/usb/host/ehci.h |1 +
2 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 83be03f..80ef95d 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -1370,6 +1370,33 @@ sitd_slot_ok (
return 1;
}
+/* rebase in case of underun without ISO_ASAP together */
+static int iso_stream_rebase(struct ehci_hcd *ehci, struct urb *urb,
+ struct ehci_iso_stream *stream, u32 span, u32 period)
+{
+ u32 i, end;
+ u32 mod = ehci-periodic_size;
+
+ if (ehci-last_base == -1)
+ return 0;
+
+ end = ((stream-next_uframe + span - period) 3) (mod - 1);
+ for (i = ehci-last_base; i != ehci-last_iso_frame;
+ i = (i + 1) (mod - 1)) {
+ /* don't schedule URB which is behind base totally */
+ if (end == i) {
+ for (i = 0; i urb-number_of_packets; i++) {
+ urb-iso_frame_desc[i].length = 0;
+ urb-iso_frame_desc[i].status = 0;
+ }
+ return 1;
+ }
+ if (((stream-next_uframe 3) (mod - 1)) == i)
+ ehci-last_iso_frame = i;
+ }
Why do you use a loop here? This looks like a straightforward thing to
test: Starting from last_base, which comes first: next_uframe or
last_iso_frame?
OK, that is simper.
It's not a very good idea to change iso_frame_desc[i].length. HCDs
aren't suppose to touch that field. Also, why set the frame
descriptor status to 0? It should remain equal to -EXDEV, because the
frame never gets sent over the bus.
Right.
You must never alter ehci-last_iso_frame like this. It violates the
driver's invariants for time to run backward. After all, there may
already be other TDs scheduled for the frames you are about to scan
No, no other TDs scheduled for the frames since the queue is empty
when iso_stream_rebase is running.
Thanks,
--
Ming Lei
--
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 v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Tue, Aug 27, 2013 at 4:05 AM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 26 Aug 2013, Ming Lei wrote: The problem is that once an interrupt QH has been unlinked, relinking it might not make it visible to the hardware until the next frame starts. Therefore interrupt endpoints with an interval of 1 ms would not work correctly; they would get unlinked and relinked in every second frame. Suppose so, I ague it still need be fixed without the giveback URB in tasklet patches, since HCDs should support submitting intr URB not from completion directly(tasklet, workqueue, or simple task). So sort of the change on interrupt unlinking isn't only for supporting giveback URB in tasklet, but also a fix on current HCDs. I don't understand your argument. The uhci-hcd _does_ support interrupt URBs being submitted from tasklets, workqueues, kernel threads, or whatever. The guarantee is that the interrupt URB will be scheduled right away if the endpoint queue has not emptied out. Considered it is common for interrupt queue that only one URB is submitted, there is no the guarantee if drivers submit URB in tasklet scheduled from complete(), so the URB isn't submitted right away always for the driver. _All_ the HCDs behave this way; there's nothing special about uhci-hcd. There's no promise to schedule an interrupt URB right away if the endpoint queue is empty. Actually the interrupt URB can be scheduled right away if the queue is idle, and no such constraint on other types of queue, so people is really confused about the difference. Submitting an URB from within the completion handler is a simple way to guarantee that the endpoint queue will never become empty. Or rather, it _was_ a simple way to guarantee this -- when the driver switches over to tasklets, that guarantee isn't valid any more. Also, the 1ms interval delay problem might exist on your RFC approach, when the tasklet schedule delay is a bit long, the resubmitting is still very late than the complete time from view of hardware. That is not relevant. Obviously we can do nothing about tasklet delay. I'm talking about the delay caused by unlinking and relinking the QH. But you mentioned the result of the delay, no matter what the reason is, the result is same. Could you explain it in detail? I mean we only discuss the change on isoc schedule in case of underrun when giveback in tasklet. The code in iso_stream_schedule() knows that any URB scheduled to end before ehci-last_iso_frame will already have been given back, and any Yes, the patch doesn't change the fact. resubmissions by the completion handler will have occurred already. That's what this comment means: /* * Use ehci-last_iso_frame as the base. There can't be any * TDs scheduled for earlier than that. */ But with your tasklet, this isn't true any more. Even though the It is still true, with the giveback urb in tasklet patch, ehci-last_iso_frame is updated to 'now' in scan_isoc(), then URBs scheduled in tasklet is always after 'now' in scan_isoc(). This is examined in greater detail below. Also even without my patches, HCD should support isoc URBs submitted by tasklet or workqueue which is scheduled in its complete(), right? No, none of the HCDs support that. More precisely, they will accept such URBs but they don't guarantee that the URB will be scheduled for the next slot after the last one used by the preceding URB. If you think there is problem caused by the patchset, the problem should have been existed in HCDs already, then it is unfair to count the change on my patch, :-) The problem is that the HCDs currently _do_ guarantee the isochronous URBs submitted by the completion handler will be scheduled immediately following the preceding URB (to the best of their ability -- not all the HCDs can do this). Maintaining this guarantee while giving back URBs in a tasklet is difficult. See the 3/3 patch in the RFC series I just posted to get an idea of the required changes. There are already several drivers which submits URB from tasklet scheduled in complete(), which doesn't submit URB directly. Is there any difference with what does in giveback in tasklet patch from view of HCD? It is still easy to extend my patch to support the case, just store the 'urb-ep' into percpu variable and compare it with resubmited urb-ep because urb-ep has to be correct before completing and during submitting. See the attachment patch. The 1/3 patch in my new RFC series is nearly the same as your attachment without the use of percpu variables (which makes it rather simpler). I suppose it could be simplified even farther by storing the the current urb-ep value in the giveback_urb_bh structure instead of adding a giveback_in_progress flag to the usb_host_endpoint. OK. There's a much simpler way to solve this problem. I
Re: [RFC 2/3] EHCI: convert the IRQ handler to a tasklet
On Mon, Aug 26, 2013 at 11:29 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 26 Aug 2013, Ming Lei wrote: On Sat, 24 Aug 2013, Ming Lei wrote: But the current interrupt from EHCI isn't be acked(USBSTS isn't cleared), so EHCI HW don't know the irq has been handled, then it is reasonable that the EHCI interrupt still interrupts CPU. EHCI spec(4.15) also states the point clearly: the host controller is not required to de-assert a currently active interrupt condition when software sets the interrupt enables (in the USBINR register, see Section 2.3.3) to a zero. The only reliable method software should use for acknowledging an interrupt is by transitioning the appropriate status bits in the USBSTS register (Section 2.3.2) from a one to a zero. Ooh, you're right! I had completely forgotten about this. It's a serious error in the RFC patch. Fixing it would be pretty easy, however. It is hard to say easy: - the USBSTS has to be cleared in top half, which means the status has to be stored somewhere(suppose it is ehci-irq_status) That's right. - ehci-irq_status need to be read in bottom half for handling irq, so one lock might be required for synchronizing the access on the variable Yes. A new spinlock would be needed to synchronize the top half and the bottom half. The same spinlock would also be used to avoid scheduling the tasklet when it is already running, like in your implementation. Then every HCD need to copy these kind of implementation... - also, once the current irq is Acked by clearing USBSTS, then later interrupts can come, so the irq status should have been saved into one queue instead of single variable(see below why disabling ehci irq isn't good) We don't need a queue. One variable is enough. The order in which the interrupt status bits turn on doesn't matter; all we need to know are which bits require handing. The top half would do: ehci-irq_status |= ehci_read(ehci, ehci-regs-status); Looks OK. The bottom half would do the same thing before checking ehci-irq_status, in case more bits got turned on while interrupts were masked. So when the above implementation is required in each HCDs change, I am wondering if it is 'pretty easy'. I think it is pretty easy for each HCD. Changing all of them would be a large job. Still not sure it is pretty easy since extra lock things have to be considered: (such as, order between two locks, disabling irq and acquiring lock) Remember, more than 99% of the work an HCD does is handling URBs. (HCDs do a couple of other things, such as detecting connections and disconnection or handling wakeups, but they are very small compared to the number of URBs it handles.) Consider also that an URB isn't done until it is given back to the driver that submitted it. Therefore, when measuring performance or latency of an HCD, what matters is how long the driver has to wait from URB submission to URB completion. If any part of that processing takes place in a tasklet, the tasklet delay will add in to the total time. It doesn't matter whether the tasklet handles only the giveback or some of the HCD's internal work as well. I am not sure if the idea of disabling EHCI irq inside ehci_irq() is good: one completed transfer may not be observed inside current tasklet handler since hardware interrupt is disabled, so the transfer can't be handled until next tasklet scheduled, then extra delay for these URBs is introduced. Even though interrupts are masked, the tasklet can still check the EHCI status register to see if new events have occurred while the tasklet was running, as I described above. The tasklet could do this check before returning. Yes, the tasklet can do it but some events(IAA, connection, fata error, ...) are handled with delay. At least, EHCI HW generates the fatal irq ASAP, This interrupt is not delayed to the next interrupt threshold.(4.15.2.4), so it is reasonable for software to handle it asap. That doesn't seem like a good reason. (Probably it just reflects the fact that once a fatal error has occurred, the controller hardware can't reliably determine when the next interrupt threshold will occur.) But of course, it would be easy to put that part of the code into the top half of the handler. More things done in top half, the change will become more complicated since more synchronizations need to consider. Not at all. ehci-lock will synchronize things perfectly well. It isn't good to hold ehci-lock in ehci_irq(), otherwise, ehci_irq has to spin on this lock when is held in tasklet. To put it another way, which would you prefer: To have three interrupts while the tasklet is running, or one interrupt as soon as it finishes? I prefer to enabling EHCI interrupt during tasklet handling. What for? It seems likely that the top half would have
Re: [RFC 2/3] EHCI: convert the IRQ handler to a tasklet
On Sun, Aug 25, 2013 at 10:45 PM, Alan Stern st...@rowland.harvard.edu wrote: To begin with, the whole point of this RFC was to show that moving the entire IRQ handler (or even a large part of it) to a tasklet would have been at least as simple as moving the givebacks alone. Now that I realize the hrtimer and unlink pathways would have to be changed too, it's not quite so simple as it seemed at first. Still, I think it would be no worse than the work you did. It also is more typical of the way people expect the work to be divided between a top half and a bottom half -- usually the top half does little more than acknowledge the interrupt. I'd like to compare the two implementations when it is 'basically ready'. Since the proposal was meant only as a proof of principle, I'm not going to develope it any farther. But I will respond to the points you raised in your review. OK, I will ask Greg to put back the patch 'USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled' into his tree. On Sat, 24 Aug 2013, Ming Lei wrote: But the current interrupt from EHCI isn't be acked(USBSTS isn't cleared), so EHCI HW don't know the irq has been handled, then it is reasonable that the EHCI interrupt still interrupts CPU. EHCI spec(4.15) also states the point clearly: the host controller is not required to de-assert a currently active interrupt condition when software sets the interrupt enables (in the USBINR register, see Section 2.3.3) to a zero. The only reliable method software should use for acknowledging an interrupt is by transitioning the appropriate status bits in the USBSTS register (Section 2.3.2) from a one to a zero. Ooh, you're right! I had completely forgotten about this. It's a serious error in the RFC patch. Fixing it would be pretty easy, however. It is hard to say easy: - the USBSTS has to be cleared in top half, which means the status has to be stored somewhere(suppose it is ehci-irq_status) - ehci-irq_status need to be read in bottom half for handling irq, so one lock might be required for synchronizing the access on the variable - also, once the current irq is Acked by clearing USBSTS, then later interrupts can come, so the irq status should have been saved into one queue instead of single variable(see below why disabling ehci irq isn't good) So when the above implementation is required in each HCDs change, I am wondering if it is 'pretty easy'. Not handling interrupts right away is the price we pay for using a bottom half. Your tasklet implementation isn't very different. Although the interrupt handler may realize quickly that a transfer has finished, there will still be a delay before the URB's completion handler can run. And the length of that delay will depend on the tasklet schedule delay. In my tasklet implementation, the next EHCI interrupt can be handled after the current hard interrupt is handled, but in this patch the EHCI hard irq can't be handled until the tasklet is handled, right? Right. But in the end, it doesn't matter. Remember, more than 99% of the work an HCD does is handling URBs. (HCDs do a couple of other things, such as detecting connections and disconnection or handling wakeups, but they are very small compared to the number of URBs it handles.) Consider also that an URB isn't done until it is given back to the driver that submitted it. Therefore, when measuring performance or latency of an HCD, what matters is how long the driver has to wait from URB submission to URB completion. If any part of that processing takes place in a tasklet, the tasklet delay will add in to the total time. It doesn't matter whether the tasklet handles only the giveback or some of the HCD's internal work as well. I am not sure if the idea of disabling EHCI irq inside ehci_irq() is good: one completed transfer may not be observed inside current tasklet handler since hardware interrupt is disabled, so the transfer can't be handled until next tasklet scheduled, then extra delay for these URBs is introduced. Thirdly, fatal error should have been handled immediately inside hard interrupt handler. Why? What difference does it make? At least, EHCI HW generates the fatal irq ASAP, This interrupt is not delayed to the next interrupt threshold.(4.15.2.4), so it is reasonable for software to handle it asap. That doesn't seem like a good reason. (Probably it just reflects the fact that once a fatal error has occurred, the controller hardware can't reliably determine when the next interrupt threshold will occur.) But of course, it would be easy to put that part of the code into the top half of the handler. More things done in top half, the change will become more complicated since more synchronizations need to consider. Finally, tasklet schedule should have been optimized here if the tasklet is running on another CPU
Re: [RFC 2/3] EHCI: convert the IRQ handler to a tasklet
On Sat, Aug 24, 2013 at 12:06 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Fri, 23 Aug 2013, Ming Lei wrote:
On Fri, Aug 23, 2013 at 4:39 AM, Alan Stern st...@rowland.harvard.edu
wrote:
This patch divides ehci-hcd's interrupt handler into a top half and a
bottom half, using a tasklet to execute the latter.
The conversion is very straightforward. The only subtle point is that
we have to ignore interrupts that arrive while the tasklet is running
(i.e., from another device on a shared IRQ line).
Not only for another device, the interrupt from ehci itself is still
delay-handled.
No. The EHCI interrupt-enable register is set to 0 in the IRQ handler,
and it can't generate interrupt requests until the tasklet finishes.
Therefore there won't be any interrupts from the controller to ignore.
But the current interrupt from EHCI isn't be acked(USBSTS isn't cleared),
so EHCI HW don't know the irq has been handled, then it is reasonable
that the EHCI interrupt still interrupts CPU.
EHCI spec(4.15) also states the point clearly:
the host controller is not required to de-assert a currently active
interrupt condition when software sets the interrupt enables (in the
USBINR register, see Section 2.3.3) to a zero. The only reliable
method software should use for acknowledging an interrupt is by
transitioning the appropriate status bits in the USBSTS register
(Section 2.3.2) from a one to a zero.
@@ -691,13 +696,40 @@ EXPORT_SYMBOL_GPL(ehci_setup);
/*-*/
-static irqreturn_t ehci_irq (struct usb_hcd *hcd)
+static irqreturn_t ehci_irq(struct usb_hcd *hcd)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
+ u32 status, masked_status;
+
+ if (ehci-irqs_are_masked)
+ return IRQ_NONE;
+
+ /*
+* We don't use STS_FLR, but some controllers don't like it to
+* remain on, so mask it out along with the other status bits.
+*/
+ status = ehci_readl(ehci, ehci-regs-status);
+ masked_status = status (INTR_MASK | STS_FLR);
+
+ /* Shared IRQ? */
+ if (!masked_status || unlikely(ehci-rh_state == EHCI_RH_HALTED))
+ return IRQ_NONE;
+
+ /* Mask IRQs and let the tasklet do the work */
+ ehci_writel(ehci, 0, ehci-regs-intr_enable);
+ ehci-irqs_are_masked = true;
+ tasklet_hi_schedule(ehci-tasklet);
+ return IRQ_HANDLED;
The irq is handled but not clearing its status, so interrupt controller
might interrupt CPU continuously and may cause irq flood before
the status is cleared. Disabling ehci interrupt may not prevent the
irq flooding since interrupt controller may latch the unhandled(from
ehci hw view) irq source.
I don't understand. If the interrupt controller has latched something,
the latch will be cleared by the system's IRQ handler. Otherwise _any_
IRQ would cause a flood.
As I said above, EHCI HW don't know the irq has been handled.
Secondly, not clearing USBSTS here may delay the next interrupt
handling for the host controller itself, and the delay depends on the
tasklet schedule delay.
Yes, of course. The same sort of thing is true for the original driver
-- the host controller can't issue a new interrupt until the handler
for the old one finishes.
Not handling interrupts right away is the price we pay for using a
bottom half. Your tasklet implementation isn't very different.
Although the interrupt handler may realize quickly that a transfer has
finished, there will still be a delay before the URB's completion
handler can run. And the length of that delay will depend on the
tasklet schedule delay.
In my tasklet implementation, the next EHCI interrupt can be handled
after the current hard interrupt is handled, but in this patch the EHCI
hard irq can't be handled until the tasklet is handled, right?
Thirdly, fatal error should have been handled immediately inside hard
interrupt handler.
Why? What difference does it make?
At least, EHCI HW generates the fatal irq ASAP, This interrupt is not
delayed to the next interrupt threshold.(4.15.2.4), so it is reasonable for
software to handle it asap.
Finally, tasklet schedule should have been optimized here if the tasklet
is running on another CPU, otherwise the current CPU may spin on
the tasklet lock until the same tasklet is completed on another CPU.
That could be added in. But doesn't the core kernel's tasklet
implementation already take care of this? The tasklet_hi_action()
routine uses tasklet_trylock(), so it doesn't spin.
OK, but extra tasklet schedule delay might be caused.
@@ -833,10 +863,16 @@ dead:
if (bh)
ehci_work (ehci);
- spin_unlock (ehci-lock);
+
+ done:
+ /* Unmask IRQs again */
+ ehci-irqs_are_masked
Re: [PATCH v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Fri, Aug 23, 2013 at 11:40 PM, Alan Stern st...@rowland.harvard.edu wrote: On Fri, 23 Aug 2013, Ming Lei wrote: On Fri, Aug 23, 2013 at 4:36 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 22 Aug 2013, Ming Lei wrote: This seems to be the most important factor. When you think about it, though, does it really minimize the changes? Consider all the other adjustments we had to make to ehci-hcd: the interrupt QH unlink change and the isochronous stream stuff (which also affects the usb-audio drivers). For other HCDs, this changes on interrupt QH unlink may not need at all if there is no much cost about relink. But for some HCDs, it will be needed. Which HCDs need them? I haven't checked in detail. It seems likely that uhci-hcd and ohci-hcd will need changes. Are you sure? If the change is absolutely required, there is certainly bug in them, since HCDs still need to support submitting interrupt URB in tasklet scheduled by its complete() handler? Even for EHCI HCD, the interrupt QH unlink change isn't necessary, and the patch is only for improving the situation, isn't it? For other HCDs, basically unlink interrupt queue need't the wait time, so don't need the change. Wrong: For other HCDs, unlinks _do_ need to wait for the hardware. Just take a quick look at UHCI code, looks there is no much difference about unlinking qh between async qh and intr qh, which means the change might not need for uhci since usb-storage is common case to support. Correct me if it is wrong. About isochronous stream stuff, the only change with moving giveback in tasklet is that iso_stream_schedule() may see list_empty(stream-td_list) when underrun, and we can solve it easily, can't we? No, it's not so easy. I have done some work on it, and it's more complicated than you might think. Not to mention that the snd-usb-audio driver (and perhaps others) will also need to be modified. As I said, the only change introduced with running giveback in tasklet is that iso_stream_schedule() may see list_empty(stream-td_list) when underrun, and we can handle it inside HCD and usbcore, nothing to do with drivers. That's not true, at least, not for ehci-hcd. Could you explain it in detail? I mean we only discuss the change on isoc schedule in case of underrun when giveback in tasklet. OK, I give you a simple solution in which only one line change is needed for these HCDs, see attachment patch. The patch is wrong. It keeps track of whether the URB is being resubmitted by the completion handler, not whether the endpoint queue list_empty(stream-td_list) is already checked in iso_stream_schedule(), so I am wondering what is the 'wrong' you mean. We need to know if the empty endpoint queue is caused by the last completed URB in queue, which will be resubmitted later. has emptied out. What happens if the completion handler for URB0 submits URB1? Do you have such example? It is possible for the two URBs which belong to different endpoint and let one of URBs to start the stream, but I am wondering it is reasonable that the two belong to same endpoint? Even for this case, we can handle it easily by checking if the two URBs belong to same endpoint. To do it properly, you would need to count the number of URBs on the endpoint queue. The count would be decremented after the completion handler returned, which means it probably would have to be an atomic_t variable -- and I know that you hate those! percpu variable should be ok since no preemption is possible during completing? and no irq handler may operate the variable. Also I remembered that you said the isochronous stream stuff needn't to consider on other HCDs. No, I didn't say that. It will have to be considered for ohci-hcd and uhci-hcd. Maybe I understand it incorrectly: We also don't have to change the isochronous code in every HCD http://www.spinics.net/lists/linux-usb/msg89826.html Right. We don't have to change the isochronous code in _every_ HCD, but we do need to change it in _some_ of them. So maybe the change on ehci HCD is a bit much, but for other HCDs, the change is just a little. The other HCDs will have to change too. Maybe not quite as much as ehci-hcd, but more than you seem to think. Per my solution, only one line change for the affected HCD is enough. No, it isn't. See above. Besides, even if your patch was correct, it still wouldn't be sufficient. ehci-hcd relies on the fact that the entries in a non-empty isochronous queue expire on or after ehci-last_iso_frame. With your patch, that wouldn't be true any more. Sorry, I don't understand your point, as I said, the only change on ISOC schedule introduced with running giveback in tasklet is that iso_stream_schedule() may see list_empty(stream-td_list) when underrun, and the above discussed patch should address this one. If you mean the delay introduced by tasklet, I
Re: [PATCH v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Wed, Aug 21, 2013 at 11:23 PM, Alan Stern st...@rowland.harvard.edu wrote: On Wed, 21 Aug 2013, Ming Lei wrote: By the way, even though it's a little late to ask this... Why did you decide to move only the giveback routine into a tasklet, instead of moving the entire interrupt handler? Looks below reasons in my mind before preparing the tasklet patch: 1, from my observation, on ARM, the most time-consuming part is dma mapping, unmapping and data copy over coherent memory, so moving giveback out of interrupt handler can decrease USB irq handling time a lot. Of course, this would remain true either way. 2, moving giveback out of irq handler can be done in usbcore, so changes can be minimized. This seems to be the most important factor. When you think about it, though, does it really minimize the changes? Consider all the other adjustments we had to make to ehci-hcd: the interrupt QH unlink change and the isochronous stream stuff (which also affects the usb-audio drivers). For other HCDs, this changes on interrupt QH unlink may not need at all if there is no much cost about relink. About isochronous stream stuff, the only change with moving giveback in tasklet is that iso_stream_schedule() may see list_empty(stream-td_list) when underrun, and we can solve it easily, can't we? Also I remembered that you said the isochronous stream stuff needn't to consider on other HCDs. So maybe the change on ehci HCD is a bit much, but for other HCDs, the change is just a little. Another good point with moving giveback only to tasklet is deadlock immune during resubmissions, as you mentioned in below link: http://www.spinics.net/lists/linux-usb/msg88710.html I'm starting to think that moving the entire handler to a tasklet would have been better. Not sure, if so: - one thing is that the HCD private lock can't be held in interrupt handler any more, so new lock need to introduce, not mention each hard irq handler need to split to two parts. - also it should have been better to avoid resubmissions in its giveback handler. 3, driver's complete() may do many driver specific things which may increase irq handling time randomly, so moving complete() to tasklet can help to decrease HCD irq handling time. This is like the first reason. Also moving only the giveback routine into a tasklet can avoid dropping HCD private lock during irq handler, which may simplify HCD code, and I have figured out ehci cleanup patches for this. I don't think we should make these changes. It's okay to keep the private lock during the giveback call, but let's not remove the other things. My feeling is that at some point we may indeed want to move the entire handler to a tasklet or a threaded IRQ. If that happens, all those simplifications would need to be undone. Better not to do them in the first place, since they add very little overhead. I think making HCDs simpler should have been welcome, :-) Thanks, -- Ming Lei -- 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 v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Thu, Aug 22, 2013 at 4:58 PM, Ming Lei ming@canonical.com wrote: On Wed, Aug 21, 2013 at 11:23 PM, Alan Stern st...@rowland.harvard.edu wrote: I'm starting to think that moving the entire handler to a tasklet would have been better. Not sure, if so: - one thing is that the HCD private lock can't be held in interrupt handler any more, so new lock need to introduce, not mention each hard irq handler need to split to two parts. Also one potential big problem of moving entire handler to tasklet is the hardware race, previously the entire hander is run with interrupt disabled and the big HCD private lock held, but after the conversion, both are not true, so each HCD change for this conversion might be big. If only moving giveback into tasklet, there is no such problem. Thanks, -- Ming Lei -- 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 v1 23/49] hid: usbhid: prepare for enabling irq in complete()
On Sun, Aug 18, 2013 at 12:24 AM, Ming Lei ming@canonical.com wrote: Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Jiri Kosina jkos...@suse.cz Cc: linux-in...@vger.kernel.org Signed-off-by: Ming Lei ming@canonical.com Jiri, could you review and give an Ack? Thanks, -- Ming Lei -- 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 v1 24/49] BT: btusb: prepare for enabling irq in complete()
On Sun, Aug 18, 2013 at 12:24 AM, Ming Lei ming@canonical.com wrote: Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Marcel Holtmann mar...@holtmann.org Cc: Gustavo Padovan gust...@padovan.org Cc: Johan Hedberg johan.hedb...@gmail.com Cc: linux-blueto...@vger.kernel.org Signed-off-by: Ming Lei ming@canonical.com Marcel, Gustavo and Johan, could you review and give an Ack if this one and 25/49 are OK so they can be merged via Greg's USB tree? Thanks, -- Ming Lei -- 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 v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Fri, Aug 23, 2013 at 4:36 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 22 Aug 2013, Ming Lei wrote: This seems to be the most important factor. When you think about it, though, does it really minimize the changes? Consider all the other adjustments we had to make to ehci-hcd: the interrupt QH unlink change and the isochronous stream stuff (which also affects the usb-audio drivers). For other HCDs, this changes on interrupt QH unlink may not need at all if there is no much cost about relink. But for some HCDs, it will be needed. Which HCDs need them? Even for EHCI HCD, the interrupt QH unlink change isn't necessary, and the patch is only for improving the situation, isn't it? For other HCDs, basically unlink interrupt queue need't the wait time, so don't need the change. About isochronous stream stuff, the only change with moving giveback in tasklet is that iso_stream_schedule() may see list_empty(stream-td_list) when underrun, and we can solve it easily, can't we? No, it's not so easy. I have done some work on it, and it's more complicated than you might think. Not to mention that the snd-usb-audio driver (and perhaps others) will also need to be modified. As I said, the only change introduced with running giveback in tasklet is that iso_stream_schedule() may see list_empty(stream-td_list) when underrun, and we can handle it inside HCD and usbcore, nothing to do with drivers. OK, I give you a simple solution in which only one line change is needed for these HCDs, see attachment patch. Also I remembered that you said the isochronous stream stuff needn't to consider on other HCDs. No, I didn't say that. It will have to be considered for ohci-hcd and uhci-hcd. Maybe I understand it incorrectly: We also don't have to change the isochronous code in every HCD http://www.spinics.net/lists/linux-usb/msg89826.html So maybe the change on ehci HCD is a bit much, but for other HCDs, the change is just a little. The other HCDs will have to change too. Maybe not quite as much as ehci-hcd, but more than you seem to think. Per my solution, only one line change for the affected HCD is enough. Another good point with moving giveback only to tasklet is deadlock immune during resubmissions, as you mentioned in below link: http://www.spinics.net/lists/linux-usb/msg88710.html That referred to giveback during submissions. It turns out that they aren't necessary in any case, although I didn't realize it at the time. I'm starting to think that moving the entire handler to a tasklet would have been better. Not sure, if so: - one thing is that the HCD private lock can't be held in interrupt handler any more, so new lock need to introduce, not mention each hard irq handler need to split to two parts. No new lock is needed -- the interrupt handler runs okay without one. Yes, the handler has to be split into two parts. But that's small and self-contained, and very few other changes are needed. How can you make sure the lockless hw operations in hard interrupt handler won't cause race with all other hw operations on host controller, anyway, previously hcd lock is held to operate on host controller, and no such race. We should be careful since the change might depends on each hardware internal things. - also it should have been better to avoid resubmissions in its giveback handler. Why? We are all set up to allow them now. Ruling them out won't make things much easier than they are already. Only for one example, we needn't consider race between qh unlink and completion inside HCD anymore, so for ehci, the below variable and related code can be removed: ehci-async_unlinking ehci-intr_unlinking qh-exception QH_STATE_COMPLETING Doesn't it make HCD simpler? Also moving only the giveback routine into a tasklet can avoid dropping HCD private lock during irq handler, which may simplify HCD code, and I have figured out ehci cleanup patches for this. I don't think we should make these changes. It's okay to keep the private lock during the giveback call, but let's not remove the other things. My feeling is that at some point we may indeed want to move the entire handler to a tasklet or a threaded IRQ. If that happens, all those simplifications would need to be undone. Better not to do them in the first place, since they add very little overhead. I think making HCDs simpler should have been welcome, :-) But it isn't simpler! Look at the extra code you had to add to ehci-hcd. It is now more complicated than it used to be, and the isochronous changes will make it even more so. Ruling out submissions during givebacks will make very little difference. See above. Also one potential big problem of moving entire handler to tasklet is the hardware race, previously the entire hander is run with interrupt disabled and the big HCD
[PATCH v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
This patch introduces ehci_disable_event(), which is applied on
IAA_WATCHDOG and START_UNLINK_INTR timeouts if the two corresponding
events(IAA and intr URB submission) happened, so that we may avoid
unnecessary CPU wakeup by canceling the timer.
One simple test indicates about 7~8 timer expirations/sec
(20% ~ 30% of totoal timer expirations/sec) can be saved when one asix
network NIC is connected to EHCI without network traffic(the device
responds interrupt poll 7~8 times per second constantly for link status
query).
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-hcd.c | 12 +---
drivers/usb/host/ehci-sched.c |4 +++-
drivers/usb/host/ehci-timer.c | 16
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 73c7299..549da36 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -738,17 +738,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
if (status STS_IAA) {
/* Turn off the IAA watchdog */
- ehci-enabled_hrtimer_events = ~BIT(EHCI_HRTIMER_IAA_WATCHDOG);
-
- /*
-* Mild optimization: Allow another IAAD to reset the
-* hrtimer, if one occurs before the next expiration.
-* In theory we could always cancel the hrtimer, but
-* tests show that about half the time it will be reset
-* for some other event anyway.
-*/
- if (ehci-next_hrtimer_event == EHCI_HRTIMER_IAA_WATCHDOG)
- ++ehci-next_hrtimer_event;
+ ehci_disable_event(ehci, EHCI_HRTIMER_IAA_WATCHDOG);
/* guard against (alleged) silicon errata */
if (cmd CMD_IAAD)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 6631089..83be03f 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -610,9 +610,11 @@ static void cancel_unlink_wait_intr(struct ehci_hcd *ehci,
struct ehci_qh *qh)
list_del_init(qh-unlink_node);
/*
-* TODO: disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
+* disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
* avoiding unnecessary CPU wakeup
*/
+ if (list_empty(ehci-intr_unlink_wait))
+ ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR);
}
static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
diff --git a/drivers/usb/host/ehci-timer.c b/drivers/usb/host/ehci-timer.c
index 424ac5d..983d063 100644
--- a/drivers/usb/host/ehci-timer.c
+++ b/drivers/usb/host/ehci-timer.c
@@ -99,6 +99,22 @@ static void ehci_enable_event(struct ehci_hcd *ehci,
unsigned event,
}
}
+/* Warning: don't call this function from hrtimer handler context */
+static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
+{
+ ehci-enabled_hrtimer_events = ~(1 event);
+
+ if (!ehci-enabled_hrtimer_events) {
+ ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
+ hrtimer_cancel(ehci-hrtimer);
+ } else if (ehci-next_hrtimer_event == event) {
+ ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
+ ehci_enable_event(ehci,
+ ffs(ehci-enabled_hrtimer_events) - 1,
+ false);
+ }
+}
+
/* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */
static void ehci_poll_ASS(struct ehci_hcd *ehci)
--
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 v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Wed, Aug 21, 2013 at 10:23 PM, Alan Stern st...@rowland.harvard.edu wrote: Okay. You can add Acked-by: Alan Stern st...@rowland.harvard.edu and submit it as a version-2 patch. I think Greg will add the ack, so needn't v2. By the way, even though it's a little late to ask this... Why did you decide to move only the giveback routine into a tasklet, instead of moving the entire interrupt handler? Looks below reasons in my mind before preparing the tasklet patch: 1, from my observation, on ARM, the most time-consuming part is dma mapping, unmapping and data copy over coherent memory, so moving giveback out of interrupt handler can decrease USB irq handling time a lot. 2, moving giveback out of irq handler can be done in usbcore, so changes can be minimized. 3, driver's complete() may do many driver specific things which may increase irq handling time randomly, so moving complete() to tasklet can help to decrease HCD irq handling time. Also moving only the giveback routine into a tasklet can avoid dropping HCD private lock during irq handler, which may simplify HCD code, and I have figured out ehci cleanup patches for this. Thanks, -- Ming Lei -- 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 3/3] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Mon, Aug 19, 2013 at 10:33 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:
Why would those events not need to be handled?
For IAA_WATCHDOG event, it needn't to handle when
IAA interrupt comes.
For START_UNLINK_INTR event, we don't need to unlink intr
qh any more if intr urb submit request comes.
What does this help with? Measurements please.
The patch may avoid unnecessary CPU wakeups caused by
timer expiration, and measurement is provided below.
On Mon, Aug 19, 2013 at 11:30 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Mon, 19 Aug 2013, Ming Lei wrote:
This patch introduces ehci_disable_event(), which is applied on
IAA_WATCHDOG and START_UNLINK_INTR events in case that the two
events needn't to be handled, so that we may avoid unnecessary CPU
wakeup.
@@ -100,6 +100,20 @@ static void ehci_enable_event(struct ehci_hcd *ehci,
unsigned event,
}
Only one blank line here, please.
OK.
+/* Warning: don't call this function from hrtimer handler context */
+static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
+{
+ ehci-enabled_hrtimer_events = ~(1 event);
+ if (!ehci-enabled_hrtimer_events) {
+ ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
+ hrtimer_cancel(ehci-hrtimer);
+ } else if (ehci-next_hrtimer_event == event) {
+ ehci-next_hrtimer_event =
+ ffs(ehci-enabled_hrtimer_events) - 1;
Should the timer be rescheduled here? It's hard to say without seeing
some test results.
You are right, the timer should be rescheduled here, otherwise there is no
effect on HCD with need_io_watchdog set.
With below change on ehci_disable_event(), about 7~8 times timer
expiration can be saved when one asix network NIC is connected
to EHCI without network traffic(the device responds interrupt poll
7~8 times per second constantly for link status query), no matter
ehci-need_io_watchdog is set or not.
BTW, the timer expiration event is observed via /proc/timer_stats.
/* Warning: don't call this function from hrtimer handler context */
static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
{
ehci-enabled_hrtimer_events = ~(1 event);
if (!ehci-enabled_hrtimer_events) {
ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
hrtimer_cancel(ehci-hrtimer);
} else if (ehci-next_hrtimer_event == event) {
ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
enum ehci_hrtimer_event next =
ffs(ehci-enabled_hrtimer_events) - 1;
ehci_enable_event(ehci, next, 0);
}
}
Thanks,
--
Ming Lei
--
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: Non-enumerable devices on USB and other enumerable buses
On Tue, Aug 20, 2013 at 12:01 AM, Mark Brown broo...@kernel.org wrote: On Mon, Aug 19, 2013 at 08:17:53PM +0800, Ming Lei wrote: On Sat, Aug 17, 2013 at 9:29 AM, Alan Stern st...@rowland.harvard.edu wrote: Aong those lines, I would like to point out that the device concept embodied in the kernel's data structures can be pretty thin. For example, it might be little more than a port number or bus address. Maybe the principle behind drivers/usb/core/usb-acpi.c is helpful for the problem, and DT may refer to ACPI to describe on-board USB devices, and the way to retrieve platform data too. I can't parse this at all well - why would DT want to refer to ACPI, do you mean people may wish to look at the code as an example? As Grant I mean usb-acpi provides one approach to retrieve platform data for USB device during device enumeration, and the idea might be helpful for you to implement similar things based on DT. noted DT already has some mechanisms for enumerable buses which looking at the code appears to be broadly what that's doing. If the mechanism is ready now, so looks you might post code for review and discussion? Thanks, -- Ming Lei -- 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 1/3] USB: use percpu counter to count submitted URBs per device
Because usb_hcd_submit_urb is in the hotest path of usb core,
so use percpu counter to count URB instead of using atomic variable
because atomic operations are much slower than percpu operations.
Cc: Oliver Neukum oli...@neukum.org
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/hcd.c |4 ++--
drivers/usb/core/sysfs.c |7 ++-
drivers/usb/core/usb.c |9 -
drivers/usb/core/usb.h |1 +
include/linux/usb.h |2 +-
5 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 19ad3d2..0b4d1ae 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1556,7 +1556,7 @@ int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
*/
usb_get_urb(urb);
atomic_inc(urb-use_count);
- atomic_inc(urb-dev-urbnum);
+ this_cpu_inc(*urb-dev-urbnum);
usbmon_urb_submit(hcd-self, urb);
/* NOTE requirements on root-hub callers (usbfs and the hub
@@ -1583,7 +1583,7 @@ int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
urb-hcpriv = NULL;
INIT_LIST_HEAD(urb-urb_list);
atomic_dec(urb-use_count);
- atomic_dec(urb-dev-urbnum);
+ this_cpu_dec(*urb-dev-urbnum);
if (atomic_read(urb-reject))
wake_up(usb_kill_urb_queue);
usb_put_urb(urb);
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index d9284b9..707f2ca 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -237,9 +237,14 @@ static ssize_t
show_urbnum(struct device *dev, struct device_attribute *attr, char *buf)
{
struct usb_device *udev;
+ unsigned int cnt = 0;
+ int i;
udev = to_usb_device(dev);
- return sprintf(buf, %d\n, atomic_read(udev-urbnum));
+ for_each_possible_cpu(i)
+ cnt += *per_cpu_ptr(udev-urbnum, i);
+
+ return sprintf(buf, %d\n, cnt);
}
static DEVICE_ATTR(urbnum, S_IRUGO, show_urbnum, NULL);
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 0a6ee2e..5111edb 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -271,6 +271,7 @@ static void usb_release_dev(struct device *dev)
kfree(udev-product);
kfree(udev-manufacturer);
kfree(udev-serial);
+ free_percpu(udev-urbnum);
kfree(udev);
}
@@ -433,7 +434,13 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
set_dev_node(dev-dev, dev_to_node(bus-controller));
dev-state = USB_STATE_ATTACHED;
dev-lpm_disable_count = 1;
- atomic_set(dev-urbnum, 0);
+
+ dev-urbnum = alloc_percpu(typeof(*dev-urbnum));
+ if (!dev-urbnum) {
+ usb_put_hcd(bus_to_hcd(bus));
+ kfree(dev);
+ return NULL;
+ }
INIT_LIST_HEAD(dev-ep0.urb_list);
dev-ep0.desc.bLength = USB_DT_ENDPOINT_SIZE;
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 8238577..12a0181 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -1,5 +1,6 @@
#include linux/pm.h
#include linux/acpi.h
+#include linux/percpu.h
struct usb_hub_descriptor;
struct dev_state;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 001629c..75332dc 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -561,7 +561,7 @@ struct usb_device {
int maxchild;
u32 quirks;
- atomic_t urbnum;
+ unsigned int __percpu *urbnum;
unsigned long active_duration;
--
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 3/3] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
This patch introduces ehci_disable_event(), which is applied on
IAA_WATCHDOG and START_UNLINK_INTR events in case that the two
events needn't to be handled, so that we may avoid unnecessary CPU
wakeup.
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-hcd.c | 12 +---
drivers/usb/host/ehci-sched.c |4 +++-
drivers/usb/host/ehci-timer.c | 14 ++
3 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 73c7299..549da36 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -738,17 +738,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
if (status STS_IAA) {
/* Turn off the IAA watchdog */
- ehci-enabled_hrtimer_events = ~BIT(EHCI_HRTIMER_IAA_WATCHDOG);
-
- /*
-* Mild optimization: Allow another IAAD to reset the
-* hrtimer, if one occurs before the next expiration.
-* In theory we could always cancel the hrtimer, but
-* tests show that about half the time it will be reset
-* for some other event anyway.
-*/
- if (ehci-next_hrtimer_event == EHCI_HRTIMER_IAA_WATCHDOG)
- ++ehci-next_hrtimer_event;
+ ehci_disable_event(ehci, EHCI_HRTIMER_IAA_WATCHDOG);
/* guard against (alleged) silicon errata */
if (cmd CMD_IAAD)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 6631089..83be03f 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -610,9 +610,11 @@ static void cancel_unlink_wait_intr(struct ehci_hcd *ehci,
struct ehci_qh *qh)
list_del_init(qh-unlink_node);
/*
-* TODO: disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
+* disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
* avoiding unnecessary CPU wakeup
*/
+ if (list_empty(ehci-intr_unlink_wait))
+ ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR);
}
static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
diff --git a/drivers/usb/host/ehci-timer.c b/drivers/usb/host/ehci-timer.c
index 424ac5d..89bce50 100644
--- a/drivers/usb/host/ehci-timer.c
+++ b/drivers/usb/host/ehci-timer.c
@@ -100,6 +100,20 @@ static void ehci_enable_event(struct ehci_hcd *ehci,
unsigned event,
}
+/* Warning: don't call this function from hrtimer handler context */
+static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
+{
+ ehci-enabled_hrtimer_events = ~(1 event);
+ if (!ehci-enabled_hrtimer_events) {
+ ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
+ hrtimer_cancel(ehci-hrtimer);
+ } else if (ehci-next_hrtimer_event == event) {
+ ehci-next_hrtimer_event =
+ ffs(ehci-enabled_hrtimer_events) - 1;
+ }
+}
+
+
/* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */
static void ehci_poll_ASS(struct ehci_hcd *ehci)
{
--
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
[RFC PATCH 2/3] USB: kill urb-use_count atomic variable
This patch kills atomic_inc/atomic_dec operations on
urb-use_count in URB submit/complete path.
The urb-use_count is only used for unlinking URB, and it isn't
necessary defined as atomic counter, so the variable is renamed
as urb-use_flag for this purpose, then reading/writing the
flag is still kept as atomic but ARCH's atomic operations(atomic_inc/
atomic_dec) are saved.
Cc: Oliver Neukum oli...@neukum.org
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/hcd.c | 14 +-
drivers/usb/core/urb.c |8 ++--
drivers/usb/core/usb.h |5 +
include/linux/usb.h|2 +-
4 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 0b4d1ae..9457c4e 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1555,7 +1555,7 @@ int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
* an error or calls giveback(), but not both.
*/
usb_get_urb(urb);
- atomic_inc(urb-use_count);
+ atomic_set(urb-use_flag, URB_USING);
this_cpu_inc(*urb-dev-urbnum);
usbmon_urb_submit(hcd-self, urb);
@@ -1582,7 +1582,7 @@ int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
usbmon_urb_submit_error(hcd-self, urb, status);
urb-hcpriv = NULL;
INIT_LIST_HEAD(urb-urb_list);
- atomic_dec(urb-use_count);
+ atomic_set(urb-use_flag, URB_UNUSED);
this_cpu_dec(*urb-dev-urbnum);
if (atomic_read(urb-reject))
wake_up(usb_kill_urb_queue);
@@ -1628,11 +1628,11 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
/* Prevent the device and bus from going away while
* the unlink is carried out. If they are already gone
-* then urb-use_count must be 0, since disconnected
+* then urb-use_flag must be URB_UNUSED, since disconnected
* devices can't have any active URBs.
*/
spin_lock_irqsave(hcd_urb_unlink_lock, flags);
- if (atomic_read(urb-use_count) 0) {
+ if (atomic_read(urb-use_flag) != URB_UNUSED) {
retval = 0;
usb_get_dev(urb-dev);
}
@@ -1672,6 +1672,8 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
/* pass ownership to the completion handler */
urb-status = status;
+ atomic_set(urb-use_flag, URB_UNUSING);
+
/*
* We disable local IRQs here avoid possible deadlock because
* drivers may call spin_lock() to hold lock which might be
@@ -1686,7 +1688,9 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
urb-complete(urb);
local_irq_restore(flags);
- atomic_dec(urb-use_count);
+ if (atomic_read(urb-use_flag) == URB_UNUSING)
+ atomic_set(urb-use_flag, URB_UNUSED);
+
if (unlikely(atomic_read(urb-reject)))
wake_up(usb_kill_urb_queue);
usb_put_urb(urb);
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index c12bc79..79d2534 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -9,6 +9,8 @@
#include linux/usb/hcd.h
#include linux/scatterlist.h
+#include usb.h
+
#define to_urb(d) container_of(d, struct urb, kref)
@@ -661,7 +663,8 @@ void usb_kill_urb(struct urb *urb)
atomic_inc(urb-reject);
usb_hcd_unlink_urb(urb, -ENOENT);
- wait_event(usb_kill_urb_queue, atomic_read(urb-use_count) == 0);
+ wait_event(usb_kill_urb_queue,
+ atomic_read(urb-use_flag) == URB_UNUSED);
atomic_dec(urb-reject);
}
@@ -705,7 +708,8 @@ void usb_poison_urb(struct urb *urb)
return;
usb_hcd_unlink_urb(urb, -ENOENT);
- wait_event(usb_kill_urb_queue, atomic_read(urb-use_count) == 0);
+ wait_event(usb_kill_urb_queue,
+ atomic_read(urb-use_flag) == URB_UNUSED);
}
EXPORT_SYMBOL_GPL(usb_poison_urb);
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 12a0181..bbdcf93 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -5,6 +5,11 @@
struct usb_hub_descriptor;
struct dev_state;
+/* urb use flag*/
+#define URB_UNUSED 0
+#define URB_USING 1
+#define URB_UNUSING2
+
/* Functions local to drivers/usb/core/ */
extern int usb_create_sysfs_dev_files(struct usb_device *dev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 75332dc..7f5f629 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1408,7 +1408,7 @@ struct urb {
/* private: usb core and host controller only fields in the urb */
struct kref kref; /* reference count of the URB */
void *hcpriv; /* private data for host controller */
- atomic_t use_count; /* concurrent submissions counter */
+ atomic_t use_flag; /* urb using flag
Re: Non-enumerable devices on USB and other enumerable buses
On Sat, Aug 17, 2013 at 9:29 AM, Alan Stern st...@rowland.harvard.edu wrote: On Fri, 16 Aug 2013, Mark Brown wrote: Besides, you need to get the platform information to the driver in any case, no matter how you decide to solve the chicken-and-egg problem. It shouldn't be a factor in deciding which solution to use. It's not that this is hard, it's that I don't see how if you already have some concept of the device in the kernel data structures (which you must have in order to be able to provide platform data when it's needed) anything is gained by not using that when dealing with bootstrapping issues. I agree. In fact, there's no choice but to use this device concept during startup. Otherwise there's no way to get the platform data to the driver when it is needed, because there's no way to tell which device the data applies to. The question is how elaborate the concept needs to be and how it gets used. Aong those lines, I would like to point out that the device concept embodied in the kernel's data structures can be pretty thin. For example, it might be little more than a port number or bus address. Maybe the principle behind drivers/usb/core/usb-acpi.c is helpful for the problem, and DT may refer to ACPI to describe on-board USB devices, and the way to retrieve platform data too. Anyway, I think it's time to try to implement something rather than talk about it. Hopefully this discussion has given you some ideas for alternative approachs, or at least helped to solidify your ideas. Thanks, -- Ming Lei -- 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 1/3] USB: use percpu counter to count submitted URBs per device
On Mon, Aug 19, 2013 at 10:00 PM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Mon, Aug 19, 2013 at 07:04:18PM +0800, Ming Lei wrote: Because usb_hcd_submit_urb is in the hotest path of usb core, so use percpu counter to count URB instead of using atomic variable because atomic operations are much slower than percpu operations. Cc: Oliver Neukum oli...@neukum.org Cc: Alan Stern st...@rowland.harvard.edu Signed-off-by: Ming Lei ming@canonical.com --- drivers/usb/core/hcd.c |4 ++-- drivers/usb/core/sysfs.c |7 ++- drivers/usb/core/usb.c |9 - drivers/usb/core/usb.h |1 + include/linux/usb.h |2 +- 5 files changed, 18 insertions(+), 5 deletions(-) And this really speeds things up? Exactly what does it? And it's not that atomic operations are slower, it's just that the For SMP, atomic_inc/atomic_dec are much slower than percpu variable inc/dec, see 4.1(Why Isn’t Concurrent Count-ing Trivial?) of [1]. However, it is slower: on a Intel Core Duo laptop, it is about six times slower than non-atomic increment when a single thread is incrementing, and more than ten times slower if two threads are incrementing. Considered that most of desktop laptop are SMP now, and with USB3.0, the submitted URBs per second may reach tens of thousand or more, and we can remove the atomic inc/dec operations in the hot path, so why don't do it? barriers involved can be slower, depending on what else is happening. If you look, you are already hitting atomic variables in the same path, so how can this change speed anything up? No, no barriers are involved in atomic_inc/atomic_dec at all. [1], Is Parallel Programming Hard, And, If So, What Can You Do About It? git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/perfbook.git Thanks, -- Ming Lei -- 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 1/3] USB: use percpu counter to count submitted URBs per device
On Mon, Aug 19, 2013 at 10:42 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 19 Aug 2013, Ming Lei wrote: Because usb_hcd_submit_urb is in the hotest path of usb core, so use percpu counter to count URB instead of using atomic variable because atomic operations are much slower than percpu operations. This seems like a ridiculous amount of additional overhead for a simple counter. The kernel doesn't even use this value for anything; it's only purpose is to allow userspace to see how many URBs have been transferred for a device. (I don't know what programs use this information. Powertop maybe?) That is why I want to remove the expensive atomic inc/dec, or can we remove the counter? Do you have any reason to believe this will really improve performance at all? Please see my reply on Greg's comments. Thanks, -- Ming Lei -- 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 1/3] USB: use percpu counter to count submitted URBs per device
On Mon, Aug 19, 2013 at 11:17 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 19 Aug 2013, Ming Lei wrote: On Mon, Aug 19, 2013 at 10:42 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 19 Aug 2013, Ming Lei wrote: Because usb_hcd_submit_urb is in the hotest path of usb core, so use percpu counter to count URB instead of using atomic variable because atomic operations are much slower than percpu operations. This seems like a ridiculous amount of additional overhead for a simple counter. The kernel doesn't even use this value for anything; it's only purpose is to allow userspace to see how many URBs have been transferred for a device. (I don't know what programs use this information. Powertop maybe?) That is why I want to remove the expensive atomic inc/dec, or can we remove the counter? No doubt somebody would complain if the counter was removed. Who added it in the first place, and for what reason? Do you have any reason to believe this will really improve performance at all? Please see my reply on Greg's comments. As far as I can see, this counter does not need to be exact. Why not simply make it a non-atomic unsigned int? It may becomes quite inaccurate, and 4.1 of the perfbook mentioned that half of counts might be lost with simple non-atomic unsigned int, so I think percpu variable is good choice. Thanks, -- Ming Lei -- 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 1/3] USB: use percpu counter to count submitted URBs per device
On Mon, Aug 19, 2013 at 11:14 PM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Mon, Aug 19, 2013 at 11:06:31PM +0800, Ming Lei wrote: On Mon, Aug 19, 2013 at 10:00 PM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Mon, Aug 19, 2013 at 07:04:18PM +0800, Ming Lei wrote: Because usb_hcd_submit_urb is in the hotest path of usb core, so use percpu counter to count URB instead of using atomic variable because atomic operations are much slower than percpu operations. Cc: Oliver Neukum oli...@neukum.org Cc: Alan Stern st...@rowland.harvard.edu Signed-off-by: Ming Lei ming@canonical.com --- drivers/usb/core/hcd.c |4 ++-- drivers/usb/core/sysfs.c |7 ++- drivers/usb/core/usb.c |9 - drivers/usb/core/usb.h |1 + include/linux/usb.h |2 +- 5 files changed, 18 insertions(+), 5 deletions(-) And this really speeds things up? Exactly what does it? And it's not that atomic operations are slower, it's just that the For SMP, atomic_inc/atomic_dec are much slower than percpu variable inc/dec, see 4.1(Why Isn’t Concurrent Count-ing Trivial?) of [1]. However, it is slower: on a Intel Core Duo laptop, it is about six times slower than non-atomic increment when a single thread is incrementing, and more than ten times slower if two threads are incrementing. Considered that most of desktop laptop are SMP now, and with USB3.0, the submitted URBs per second may reach tens of thousand or more, and we can remove the atomic inc/dec operations in the hot path, so why don't do it? Because you really didn't do it, there are lots of other atomic operations on that same path. Not lots in the path of usbcore. And, thens of thousands of urbs should be trivial, did you measure this to see if it changed anything? I'm not taking patches like this that are not quantifiable, sorry. The number may be too trivial to measure, but I will try to test with perf. The gating problem in USB right now is the hardware, it's the slowest thing, not the kernel, from everything I have ever tested, or seen. The problem may not speed up usb performance, but might decrease CPU utilization a bit, or cache miss. Well, bad host controller silicon is also a problem (i.e. raspberry pi), but there's not much we can do about braindead problems like that... barriers involved can be slower, depending on what else is happening. If you look, you are already hitting atomic variables in the same path, so how can this change speed anything up? No, no barriers are involved in atomic_inc/atomic_dec at all. None? Hm, you might want to rethink that statement :) Please see Documentation/memory-barriers.txt: The following also do _not_ imply memory barriers, and so may require explicit memory barriers under some circumstances (smp_mb__before_atomic_dec() for instance): atomic_add(); atomic_sub(); atomic_inc(); atomic_dec(); Thanks, -- Ming Lei -- 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 v1 02/49] USB: sg: prepare for enabling irq in complete()
On Sun, Aug 18, 2013 at 9:01 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Sun, 18 Aug 2013, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/message.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 82927e1..8bba734 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -266,8 +266,9 @@ static void sg_complete(struct urb *urb)
{
struct usb_sg_request *io = urb-context;
int status = urb-status;
+ unsigned long flags;
- spin_lock(io-lock);
+ spin_lock_irqsave(io-lock, flags);
/* In 2.5 we require hcds' endpoint queues not to progress after fault
* reports, until the completion callback (this!) returns. That lets
@@ -326,7 +327,7 @@ static void sg_complete(struct urb *urb)
if (!io-count)
complete(io-complete);
- spin_unlock(io-lock);
+ spin_unlock_irqrestore(io-lock, flags);
}
As far as I can see, these don't need to disable interrupts. All they
protect against is the code in usb_sg_wait() and usb_sg_cancel(), which
both run in process context.
Yes.
But will lockdep complain if they don't disable interrupts?
Looks lockdep won't complain because the lock can't be held in
another hardirq context.
As I mentioned in 00/50, the patchset is basically a mechanical
change, so one patch can be dropped if anyone reviews and
concludes it isn't needed.
Thanks,
--
Ming Lei
--
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 v1 26/49] input: cm109: prepare for enabling irq in complete()
Hi Dmitry, On Sun, Aug 18, 2013 at 11:37 AM, Dmitry Torokhov dmitry.torok...@gmail.com wrote: Hi Ming, On Sun, Aug 18, 2013 at 12:24:51AM +0800, Ming Lei wrote: Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). I think cm109 needs some love in it's URB handling, but this patch does not change anything, so: Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com Thank you. Or do you want me to pick it up for my tree? IMO, it might be easier to merge these patches via one tree, so Greg, would you like to manage all these patches via your tree? If not, I have to push these patches on each subsystem, then you need to pick it up for your input tree... Thanks, -- Ming Lei -- 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 v1 02/49] USB: sg: prepare for enabling irq in complete()
On Sun, Aug 18, 2013 at 10:44 PM, Alan Stern st...@rowland.harvard.edu wrote: On Sun, 18 Aug 2013, Ming Lei wrote: As far as I can see, these don't need to disable interrupts. All they protect against is the code in usb_sg_wait() and usb_sg_cancel(), which both run in process context. Yes. But will lockdep complain if they don't disable interrupts? Looks lockdep won't complain because the lock can't be held in another hardirq context. Don't be so sure. Suppose you have two mass-storage devices, one connected by EHCI and one connected by UHCI. The one using UHCI _will_ invoke the completion handler in hardirq context, because uhci-hcd doesn't support tasklets. Have you tested this? It can't be triggered since we don't enable local interrupts yet before calling completion handler. Also uhci-hcd/ohci-hcd should support tasklet later, even for xhci-hcd, there is only little performance loss with tasklet. As I mentioned in 00/50, the patchset is basically a mechanical change, so one patch can be dropped if anyone reviews and concludes it isn't needed. I'm afraid that it might be needed to keep lockdep happy, not to prevent real problems. Right, so the patch should be kept. Thanks, -- Ming Lei -- 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 v1 02/49] USB: sg: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/message.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 82927e1..8bba734 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -266,8 +266,9 @@ static void sg_complete(struct urb *urb)
{
struct usb_sg_request *io = urb-context;
int status = urb-status;
+ unsigned long flags;
- spin_lock(io-lock);
+ spin_lock_irqsave(io-lock, flags);
/* In 2.5 we require hcds' endpoint queues not to progress after fault
* reports, until the completion callback (this!) returns. That lets
@@ -326,7 +327,7 @@ static void sg_complete(struct urb *urb)
if (!io-count)
complete(io-complete);
- spin_unlock(io-lock);
+ spin_unlock_irqrestore(io-lock, flags);
}
--
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 v1 00/49] USB: prepare for enabling irq in complete()
/usb/serial/symbolserial.c |5 ++-- drivers/usb/serial/ti_usb_3410_5052.c |9 +++--- drivers/usb/serial/usb_wwan.c |5 ++-- drivers/usb/usb-skeleton.c| 11 --- sound/usb/caiaq/audio.c |5 ++-- sound/usb/midi.c |5 ++-- 54 files changed, 318 insertions(+), 221 deletions(-) Thanks, -- Ming Lei -- 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 v1 04/49] USB: cdc-wdm: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Oliver Neukum oli...@neukum.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/class/cdc-wdm.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index d3318a0..a54bbb2 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -143,10 +143,12 @@ found:
static void wdm_out_callback(struct urb *urb)
{
struct wdm_device *desc;
+ unsigned long flags;
+
desc = urb-context;
- spin_lock(desc-iuspin);
+ spin_lock_irqsave(desc-iuspin, flags);
desc-werr = urb-status;
- spin_unlock(desc-iuspin);
+ spin_unlock_irqrestore(desc-iuspin, flags);
kfree(desc-outbuf);
desc-outbuf = NULL;
clear_bit(WDM_IN_USE, desc-flags);
@@ -158,8 +160,9 @@ static void wdm_in_callback(struct urb *urb)
struct wdm_device *desc = urb-context;
int status = urb-status;
int length = urb-actual_length;
+ unsigned long flags;
- spin_lock(desc-iuspin);
+ spin_lock_irqsave(desc-iuspin, flags);
clear_bit(WDM_RESPONDING, desc-flags);
if (status) {
@@ -203,7 +206,7 @@ skip_error:
wake_up(desc-wait);
set_bit(WDM_READ, desc-flags);
- spin_unlock(desc-iuspin);
+ spin_unlock_irqrestore(desc-iuspin, flags);
}
static void wdm_int_callback(struct urb *urb)
@@ -213,6 +216,7 @@ static void wdm_int_callback(struct urb *urb)
int status = urb-status;
struct wdm_device *desc;
struct usb_cdc_notification *dr;
+ unsigned long flags;
desc = urb-context;
dr = (struct usb_cdc_notification *)desc-sbuf;
@@ -261,7 +265,7 @@ static void wdm_int_callback(struct urb *urb)
goto exit;
}
- spin_lock(desc-iuspin);
+ spin_lock_irqsave(desc-iuspin, flags);
clear_bit(WDM_READ, desc-flags);
responding = test_and_set_bit(WDM_RESPONDING, desc-flags);
if (!responding !test_bit(WDM_DISCONNECTING, desc-flags)
@@ -270,7 +274,7 @@ static void wdm_int_callback(struct urb *urb)
dev_dbg(desc-intf-dev, %s: usb_submit_urb %d,
__func__, rv);
}
- spin_unlock(desc-iuspin);
+ spin_unlock_irqrestore(desc-iuspin, flags);
if (rv 0) {
clear_bit(WDM_RESPONDING, desc-flags);
if (rv == -EPERM)
--
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 v1 01/49] USB: skeleton: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/usb-skeleton.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index ff97652..1743006 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -164,10 +164,11 @@ static int skel_flush(struct file *file, fl_owner_t id)
static void skel_read_bulk_callback(struct urb *urb)
{
struct usb_skel *dev;
+ unsigned long flags;
dev = urb-context;
- spin_lock(dev-err_lock);
+ spin_lock_irqsave(dev-err_lock, flags);
/* sync/async unlink faults aren't errors */
if (urb-status) {
if (!(urb-status == -ENOENT ||
@@ -182,7 +183,7 @@ static void skel_read_bulk_callback(struct urb *urb)
dev-bulk_in_filled = urb-actual_length;
}
dev-ongoing_read = 0;
- spin_unlock(dev-err_lock);
+ spin_unlock_irqrestore(dev-err_lock, flags);
wake_up_interruptible(dev-bulk_in_wait);
}
@@ -341,6 +342,8 @@ static void skel_write_bulk_callback(struct urb *urb)
/* sync/async unlink faults aren't errors */
if (urb-status) {
+ unsigned long flags;
+
if (!(urb-status == -ENOENT ||
urb-status == -ECONNRESET ||
urb-status == -ESHUTDOWN))
@@ -348,9 +351,9 @@ static void skel_write_bulk_callback(struct urb *urb)
%s - nonzero write bulk status received: %d\n,
__func__, urb-status);
- spin_lock(dev-err_lock);
+ spin_lock_irqsave(dev-err_lock, flags);
dev-errors = urb-status;
- spin_unlock(dev-err_lock);
+ spin_unlock_irqrestore(dev-err_lock, flags);
}
/* free up our allocated buffer */
--
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 v1 03/49] USB: devio: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/devio.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 737e3c1..826289c 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -495,8 +495,9 @@ static void async_completed(struct urb *urb)
u32 secid = 0;
const struct cred *cred = NULL;
int signr;
+ unsigned long flags;
- spin_lock(ps-lock);
+ spin_lock_irqsave(ps-lock, flags);
list_move_tail(as-asynclist, ps-async_completed);
as-status = urb-status;
signr = as-signr;
@@ -518,7 +519,7 @@ static void async_completed(struct urb *urb)
if (as-status 0 as-bulk_addr as-status != -ECONNRESET
as-status != -ENOENT)
cancel_bulk_urbs(ps, as-bulk_addr);
- spin_unlock(ps-lock);
+ spin_unlock_irqrestore(ps-lock, flags);
if (signr) {
kill_pid_info_as_cred(sinfo.si_signo, sinfo, pid, cred, secid);
--
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 v1 07/49] USB: misc: uss720: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so prepare for
the comming change.
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/uss720.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/uss720.c b/drivers/usb/misc/uss720.c
index 40ef40a..b2e76fa 100644
--- a/drivers/usb/misc/uss720.c
+++ b/drivers/usb/misc/uss720.c
@@ -132,8 +132,13 @@ static void async_complete(struct urb *urb)
(unsigned int)priv-reg[6]);
#endif
/* if nAck interrupts are enabled and we have an interrupt,
call the interrupt procedure */
- if (rq-reg[2] rq-reg[1] 0x10 pp)
+ if (rq-reg[2] rq-reg[1] 0x10 pp) {
+ unsigned long flags;
+
+ local_irq_save(flags);
parport_generic_irq(pp);
+ local_irq_restore(flags);
+ }
}
complete(rq-compl);
kref_put(rq-ref_count, destroy_async);
--
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 v1 05/49] USB: usblp: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Pete Zaitcev zait...@kotori.zaitcev.us
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/class/usblp.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index d4c47d5..04163d8 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -297,6 +297,7 @@ static void usblp_bulk_read(struct urb *urb)
{
struct usblp *usblp = urb-context;
int status = urb-status;
+ unsigned long flags;
if (usblp-present usblp-used) {
if (status)
@@ -304,14 +305,14 @@ static void usblp_bulk_read(struct urb *urb)
nonzero read bulk status received: %d\n,
usblp-minor, status);
}
- spin_lock(usblp-lock);
+ spin_lock_irqsave(usblp-lock, flags);
if (status 0)
usblp-rstatus = status;
else
usblp-rstatus = urb-actual_length;
usblp-rcomplete = 1;
wake_up(usblp-rwait);
- spin_unlock(usblp-lock);
+ spin_unlock_irqrestore(usblp-lock, flags);
usb_free_urb(urb);
}
@@ -320,6 +321,7 @@ static void usblp_bulk_write(struct urb *urb)
{
struct usblp *usblp = urb-context;
int status = urb-status;
+ unsigned long flags;
if (usblp-present usblp-used) {
if (status)
@@ -327,7 +329,7 @@ static void usblp_bulk_write(struct urb *urb)
nonzero write bulk status received: %d\n,
usblp-minor, status);
}
- spin_lock(usblp-lock);
+ spin_lock_irqsave(usblp-lock, flags);
if (status 0)
usblp-wstatus = status;
else
@@ -335,7 +337,7 @@ static void usblp_bulk_write(struct urb *urb)
usblp-no_paper = 0;
usblp-wcomplete = 1;
wake_up(usblp-wwait);
- spin_unlock(usblp-lock);
+ spin_unlock_irqrestore(usblp-lock, flags);
usb_free_urb(urb);
}
--
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 v1 06/49] USB: adutux: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Lisa Nguyen l...@xenapiadmin.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/adutux.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index 3eaa83f..a30efb8 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -159,11 +159,12 @@ static void adu_interrupt_in_callback(struct urb *urb)
{
struct adu_device *dev = urb-context;
int status = urb-status;
+ unsigned long flags;
adu_debug_data(dev-udev-dev, __func__,
urb-actual_length, urb-transfer_buffer);
- spin_lock(dev-buflock);
+ spin_lock_irqsave(dev-buflock, flags);
if (status != 0) {
if ((status != -ENOENT) (status != -ECONNRESET)
@@ -194,7 +195,7 @@ static void adu_interrupt_in_callback(struct urb *urb)
exit:
dev-read_urb_finished = 1;
- spin_unlock(dev-buflock);
+ spin_unlock_irqrestore(dev-buflock, flags);
/* always wake up so we recover from errors */
wake_up_interruptible(dev-read_wait);
}
@@ -203,6 +204,7 @@ static void adu_interrupt_out_callback(struct urb *urb)
{
struct adu_device *dev = urb-context;
int status = urb-status;
+ unsigned long flags;
adu_debug_data(dev-udev-dev, __func__,
urb-actual_length, urb-transfer_buffer);
@@ -217,10 +219,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
return;
}
- spin_lock(dev-buflock);
+ spin_lock_irqsave(dev-buflock, flags);
dev-out_urb_finished = 1;
wake_up(dev-write_wait);
- spin_unlock(dev-buflock);
+ spin_unlock_irqrestore(dev-buflock, flags);
}
static int adu_open(struct inode *inode, struct file *file)
--
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 v1 08/49] USB: iowarrior: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/iowarrior.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index d36f34e..010ed6d 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -162,6 +162,7 @@ static void iowarrior_callback(struct urb *urb)
int offset;
int status = urb-status;
int retval;
+ unsigned long flags;
switch (status) {
case 0:
@@ -175,7 +176,7 @@ static void iowarrior_callback(struct urb *urb)
goto exit;
}
- spin_lock(dev-intr_idx_lock);
+ spin_lock_irqsave(dev-intr_idx_lock, flags);
intr_idx = atomic_read(dev-intr_idx);
/* aux_idx become previous intr_idx */
aux_idx = (intr_idx == 0) ? (MAX_INTERRUPT_BUFFER - 1) : (intr_idx - 1);
@@ -211,7 +212,7 @@ static void iowarrior_callback(struct urb *urb)
*(dev-read_queue + offset + (dev-report_size)) = dev-serial_number++;
atomic_set(dev-intr_idx, aux_idx);
- spin_unlock(dev-intr_idx_lock);
+ spin_unlock_irqrestore(dev-intr_idx_lock, flags);
/* tell the blocking read about the new data */
wake_up_interruptible(dev-read_wait);
--
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 v1 14/49] USB: serial: io_edgeport: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/io_edgeport.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index c91481d..b9eeec6 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -567,6 +567,7 @@ static void edge_interrupt_callback(struct urb *urb)
int portNumber;
int result;
int status = urb-status;
+ unsigned long flags;
switch (status) {
case 0:
@@ -592,7 +593,7 @@ static void edge_interrupt_callback(struct urb *urb)
if (length 1) {
bytes_avail = data[0] | (data[1] 8);
if (bytes_avail) {
- spin_lock(edge_serial-es_lock);
+ spin_lock_irqsave(edge_serial-es_lock, flags);
edge_serial-rxBytesAvail += bytes_avail;
dev_dbg(dev,
%s - bytes_avail=%d, rxBytesAvail=%d,
read_in_progress=%d\n,
@@ -615,7 +616,7 @@ static void edge_interrupt_callback(struct urb *urb)
edge_serial-read_in_progress =
false;
}
}
- spin_unlock(edge_serial-es_lock);
+ spin_unlock_irqrestore(edge_serial-es_lock,
flags);
}
}
/* grab the txcredits for the ports if available */
@@ -628,9 +629,9 @@ static void edge_interrupt_callback(struct urb *urb)
port = edge_serial-serial-port[portNumber];
edge_port = usb_get_serial_port_data(port);
if (edge_port-open) {
- spin_lock(edge_port-ep_lock);
+ spin_lock_irqsave(edge_port-ep_lock,
flags);
edge_port-txCredits += txCredits;
- spin_unlock(edge_port-ep_lock);
+
spin_unlock_irqrestore(edge_port-ep_lock, flags);
dev_dbg(dev, %s - txcredits for port%d
= %d\n,
__func__, portNumber,
edge_port-txCredits);
@@ -671,6 +672,7 @@ static void edge_bulk_in_callback(struct urb *urb)
int retval;
__u16 raw_data_length;
int status = urb-status;
+ unsigned long flags;
if (status) {
dev_dbg(urb-dev-dev, %s - nonzero read bulk status
received: %d\n,
@@ -690,7 +692,7 @@ static void edge_bulk_in_callback(struct urb *urb)
usb_serial_debug_data(dev, __func__, raw_data_length, data);
- spin_lock(edge_serial-es_lock);
+ spin_lock_irqsave(edge_serial-es_lock, flags);
/* decrement our rxBytes available by the number that we just got */
edge_serial-rxBytesAvail -= raw_data_length;
@@ -714,7 +716,7 @@ static void edge_bulk_in_callback(struct urb *urb)
edge_serial-read_in_progress = false;
}
- spin_unlock(edge_serial-es_lock);
+ spin_unlock_irqrestore(edge_serial-es_lock, flags);
}
--
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 v1 16/49] USB: serial: mos7720: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/mos7720.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
index b013001..44b5e62 100644
--- a/drivers/usb/serial/mos7720.c
+++ b/drivers/usb/serial/mos7720.c
@@ -340,14 +340,15 @@ static void async_complete(struct urb *urb)
{
struct urbtracker *urbtrack = urb-context;
int status = urb-status;
+ unsigned long flags;
if (unlikely(status))
dev_dbg(urb-dev-dev, %s - nonzero urb status received:
%d\n, __func__, status);
/* remove the urbtracker from the active_urbs list */
- spin_lock(urbtrack-mos_parport-listlock);
+ spin_lock_irqsave(urbtrack-mos_parport-listlock, flags);
list_del(urbtrack-urblist_entry);
- spin_unlock(urbtrack-mos_parport-listlock);
+ spin_unlock_irqrestore(urbtrack-mos_parport-listlock, flags);
kref_put(urbtrack-ref_count, destroy_urbtracker);
}
--
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 v1 15/49] USB: serial: io_ti: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/io_ti.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 9c18f59..a481414 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -1612,6 +1612,7 @@ static void edge_bulk_in_callback(struct urb *urb)
int retval = 0;
int port_number;
int status = urb-status;
+ unsigned long flags;
switch (status) {
case 0:
@@ -1660,13 +1661,13 @@ static void edge_bulk_in_callback(struct urb *urb)
exit:
/* continue read unless stopped */
- spin_lock(edge_port-ep_lock);
+ spin_lock_irqsave(edge_port-ep_lock, flags);
if (edge_port-ep_read_urb_state == EDGE_READ_URB_RUNNING)
retval = usb_submit_urb(urb, GFP_ATOMIC);
else if (edge_port-ep_read_urb_state == EDGE_READ_URB_STOPPING)
edge_port-ep_read_urb_state = EDGE_READ_URB_STOPPED;
- spin_unlock(edge_port-ep_lock);
+ spin_unlock_irqrestore(edge_port-ep_lock, flags);
if (retval)
dev_err(dev, %s - usb_submit_urb failed with result %d\n,
__func__, retval);
}
--
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 v1 17/49] USB: serial: mos77840: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/mos7840.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index fdf9535..762dad4 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -807,17 +807,18 @@ static void mos7840_bulk_out_data_callback(struct urb
*urb)
struct usb_serial_port *port;
int status = urb-status;
int i;
+ unsigned long flags;
mos7840_port = urb-context;
port = mos7840_port-port;
- spin_lock(mos7840_port-pool_lock);
+ spin_lock_irqsave(mos7840_port-pool_lock, flags);
for (i = 0; i NUM_URBS; i++) {
if (urb == mos7840_port-write_urb_pool[i]) {
mos7840_port-busy[i] = 0;
break;
}
}
- spin_unlock(mos7840_port-pool_lock);
+ spin_unlock_irqrestore(mos7840_port-pool_lock, flags);
if (status) {
dev_dbg(port-dev, nonzero write bulk status received:%d\n,
status);
--
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 v1 12/49] USB: serial: cyberjack: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Matthias Bruestle and Harald Welte supp...@reiner-sct.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/cyberjack.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/cyberjack.c b/drivers/usb/serial/cyberjack.c
index 7814262..0ab0957 100644
--- a/drivers/usb/serial/cyberjack.c
+++ b/drivers/usb/serial/cyberjack.c
@@ -271,11 +271,12 @@ static void cyberjack_read_int_callback(struct urb *urb)
/* React only to interrupts signaling a bulk_in transfer */
if (urb-actual_length == 4 data[0] == 0x01) {
short old_rdtodo;
+ unsigned long flags;
/* This is a announcement of coming bulk_ins. */
unsigned short size = ((unsigned short)data[3]8)+data[2]+3;
- spin_lock(priv-lock);
+ spin_lock_irqsave(priv-lock, flags);
old_rdtodo = priv-rdtodo;
@@ -290,7 +291,7 @@ static void cyberjack_read_int_callback(struct urb *urb)
dev_dbg(dev, %s - rdtodo: %d\n, __func__, priv-rdtodo);
- spin_unlock(priv-lock);
+ spin_unlock_irqrestore(priv-lock, flags);
if (!old_rdtodo) {
result = usb_submit_urb(port-read_urb, GFP_ATOMIC);
@@ -317,6 +318,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
short todo;
int result;
int status = urb-status;
+ unsigned long flags;
usb_serial_debug_data(dev, __func__, urb-actual_length, data);
if (status) {
@@ -330,7 +332,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
tty_flip_buffer_push(port-port);
}
- spin_lock(priv-lock);
+ spin_lock_irqsave(priv-lock, flags);
/* Reduce urbs to do by one. */
priv-rdtodo -= urb-actual_length;
@@ -339,7 +341,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
priv-rdtodo = 0;
todo = priv-rdtodo;
- spin_unlock(priv-lock);
+ spin_unlock_irqrestore(priv-lock, flags);
dev_dbg(dev, %s - rdtodo: %d\n, __func__, todo);
@@ -359,6 +361,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
struct cyberjack_private *priv = usb_get_serial_port_data(port);
struct device *dev = port-dev;
int status = urb-status;
+ unsigned long flags;
set_bit(0, port-write_urbs_free);
if (status) {
@@ -367,7 +370,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
return;
}
- spin_lock(priv-lock);
+ spin_lock_irqsave(priv-lock, flags);
/* only do something if we have more data to send */
if (priv-wrfilled) {
@@ -411,7 +414,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
}
exit:
- spin_unlock(priv-lock);
+ spin_unlock_irqrestore(priv-lock, flags);
usb_serial_port_softint(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 v1 10/49] USB: legousbtower: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Juergen Stuber starb...@users.sourceforge.net
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/legousbtower.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index eb37c95..d1bc420 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -739,7 +739,9 @@ static void tower_interrupt_in_callback (struct urb *urb)
}
if (urb-actual_length 0) {
- spin_lock (dev-read_buffer_lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(dev-read_buffer_lock, flags);
if (dev-read_buffer_length + urb-actual_length
read_buffer_size) {
memcpy (dev-read_buffer + dev-read_buffer_length,
dev-interrupt_in_buffer,
@@ -752,7 +754,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
pr_warn(read_buffer overflow, %d bytes dropped\n,
urb-actual_length);
}
- spin_unlock (dev-read_buffer_lock);
+ spin_unlock_irqrestore(dev-read_buffer_lock, flags);
}
resubmit:
--
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 v1 09/49] USB: ldusb: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/ldusb.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index b1d5953..76ab55a 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -236,6 +236,7 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
unsigned int next_ring_head;
int status = urb-status;
int retval;
+ unsigned long flags;
if (status) {
if (status == -ENOENT ||
@@ -246,12 +247,12 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
dev_dbg(dev-intf-dev,
%s: nonzero status received: %d\n, __func__,
status);
- spin_lock(dev-rbsl);
+ spin_lock_irqsave(dev-rbsl, flags);
goto resubmit; /* maybe we can recover */
}
}
- spin_lock(dev-rbsl);
+ spin_lock_irqsave(dev-rbsl, flags);
if (urb-actual_length 0) {
next_ring_head = (dev-ring_head+1) % ring_buffer_size;
if (next_ring_head != dev-ring_tail) {
@@ -280,7 +281,7 @@ resubmit:
dev-buffer_overflow = 1;
}
}
- spin_unlock(dev-rbsl);
+ spin_unlock_irqrestore(dev-rbsl, flags);
exit:
dev-interrupt_in_done = 1;
wake_up_interruptible(dev-read_wait);
--
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 v1 13/49] USB: serial: digi_acceleportldusb: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Peter Berger pber...@brimson.com
Cc: Al Borchers alborch...@steinerpoint.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/digi_acceleport.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/serial/digi_acceleport.c
b/drivers/usb/serial/digi_acceleport.c
index 19b467f..95b1959 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -988,6 +988,7 @@ static void digi_write_bulk_callback(struct urb *urb)
struct digi_serial *serial_priv;
int ret = 0;
int status = urb-status;
+ unsigned long flags;
/* port and serial sanity check */
if (port == NULL || (priv = usb_get_serial_port_data(port)) == NULL) {
@@ -1006,15 +1007,15 @@ static void digi_write_bulk_callback(struct urb *urb)
/* handle oob callback */
if (priv-dp_port_num == serial_priv-ds_oob_port_num) {
dev_dbg(port-dev, digi_write_bulk_callback: oob callback\n);
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
priv-dp_write_urb_in_use = 0;
wake_up_interruptible(port-write_wait);
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
return;
}
/* try to send any buffered data on this port */
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
priv-dp_write_urb_in_use = 0;
if (priv-dp_out_buf_len 0) {
*((unsigned char *)(port-write_urb-transfer_buffer))
@@ -1037,7 +1038,7 @@ static void digi_write_bulk_callback(struct urb *urb)
/* lost the race in write_chan(). */
schedule_work(priv-dp_wakeup_work);
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
if (ret ret != -EPERM)
dev_err_console(port,
%s: usb_submit_urb failed, ret=%d, port=%d\n,
@@ -1388,6 +1389,7 @@ static int digi_read_inb_callback(struct urb *urb)
unsigned char *data = ((unsigned char *)urb-transfer_buffer) + 3;
int flag, throttled;
int status = urb-status;
+ unsigned long flags;
/* do not process callbacks on closed ports */
/* but do continue the read chain */
@@ -1404,7 +1406,7 @@ static int digi_read_inb_callback(struct urb *urb)
return -1;
}
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
/* check for throttle; if set, do not resubmit read urb */
/* indicate the read chain needs to be restarted on unthrottle */
@@ -1438,7 +1440,7 @@ static int digi_read_inb_callback(struct urb *urb)
tty_flip_buffer_push(port-port);
}
}
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
if (opcode == DIGI_CMD_RECEIVE_DISABLE)
dev_dbg(port-dev, %s: got RECEIVE_DISABLE\n, __func__);
@@ -1469,6 +1471,7 @@ static int digi_read_oob_callback(struct urb *urb)
int opcode, line, status, val;
int i;
unsigned int rts;
+ unsigned long flags;
/* handle each oob command */
for (i = 0; i urb-actual_length - 3;) {
@@ -1496,7 +1499,7 @@ static int digi_read_oob_callback(struct urb *urb)
rts = tty-termios.c_cflag CRTSCTS;
if (tty opcode == DIGI_CMD_READ_INPUT_SIGNALS) {
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
/* convert from digi flags to termiox flags */
if (val DIGI_READ_INPUT_SIGNALS_CTS) {
priv-dp_modem_signals |= TIOCM_CTS;
@@ -1524,12 +1527,12 @@ static int digi_read_oob_callback(struct urb *urb)
else
priv-dp_modem_signals = ~TIOCM_CD;
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
} else if (opcode == DIGI_CMD_TRANSMIT_IDLE) {
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
priv-dp_transmit_idle = 1;
wake_up_interruptible(priv-dp_transmit_idle_wait);
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
} else if (opcode == DIGI_CMD_IFLUSH_FIFO) {
wake_up_interruptible(priv-dp_flush_wait);
}
--
1.7.9.5
--
To unsubscribe from this list: send
[PATCH v1 18/49] USB: serial: quatech2: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/quatech2.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
index a24d59a..4daa5c9 100644
--- a/drivers/usb/serial/quatech2.c
+++ b/drivers/usb/serial/quatech2.c
@@ -632,16 +632,17 @@ static void qt2_write_bulk_callback(struct urb *urb)
{
struct usb_serial_port *port;
struct qt2_port_private *port_priv;
+ unsigned long flags;
port = urb-context;
port_priv = usb_get_serial_port_data(port);
- spin_lock(port_priv-urb_lock);
+ spin_lock_irqsave(port_priv-urb_lock, flags);
port_priv-urb_in_use = false;
usb_serial_port_softint(port);
- spin_unlock(port_priv-urb_lock);
+ spin_unlock_irqrestore(port_priv-urb_lock, flags);
}
--
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 v1 11/49] USB: usbtest: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/usbtest.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index aa28ac8..14277f8 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -804,11 +804,12 @@ static void ctrl_complete(struct urb *urb)
struct usb_ctrlrequest *reqp;
struct subcase *subcase;
int status = urb-status;
+ unsigned long flags;
reqp = (struct usb_ctrlrequest *)urb-setup_packet;
subcase = container_of(reqp, struct subcase, setup);
- spin_lock(ctx-lock);
+ spin_lock_irqsave(ctx-lock, flags);
ctx-count--;
ctx-pending--;
@@ -907,7 +908,7 @@ error:
/* signal completion when nothing's queued */
if (ctx-pending == 0)
complete(ctx-complete);
- spin_unlock(ctx-lock);
+ spin_unlock_irqrestore(ctx-lock, flags);
}
static int
@@ -1551,8 +1552,9 @@ struct iso_context {
static void iso_callback(struct urb *urb)
{
struct iso_context *ctx = urb-context;
+ unsigned long flags;
- spin_lock(ctx-lock);
+ spin_lock_irqsave(ctx-lock, flags);
ctx-count--;
ctx-packet_count += urb-number_of_packets;
@@ -1592,7 +1594,7 @@ static void iso_callback(struct urb *urb)
complete(ctx-done);
}
done:
- spin_unlock(ctx-lock);
+ spin_unlock_irqrestore(ctx-lock, flags);
}
static struct urb *iso_alloc_urb(
--
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 v1 20/49] USB: serial: symbolserial: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/symbolserial.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/symbolserial.c
b/drivers/usb/serial/symbolserial.c
index 9b16489..b4f5cbe 100644
--- a/drivers/usb/serial/symbolserial.c
+++ b/drivers/usb/serial/symbolserial.c
@@ -41,6 +41,7 @@ static void symbol_int_callback(struct urb *urb)
int status = urb-status;
int result;
int data_length;
+ unsigned long flags;
switch (status) {
case 0:
@@ -81,7 +82,7 @@ static void symbol_int_callback(struct urb *urb)
}
exit:
- spin_lock(priv-lock);
+ spin_lock_irqsave(priv-lock, flags);
/* Continue trying to always read if we should */
if (!priv-throttled) {
@@ -92,7 +93,7 @@ exit:
__func__, result);
} else
priv-actually_throttled = true;
- spin_unlock(priv-lock);
+ spin_unlock_irqrestore(priv-lock, flags);
}
static int symbol_open(struct tty_struct *tty, struct usb_serial_port *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 v1 22/49] USB: serial: usb_wwan: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/usb_wwan.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 8536578..92c9fa2 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -312,6 +312,7 @@ static void usb_wwan_outdat_callback(struct urb *urb)
struct usb_wwan_port_private *portdata;
struct usb_wwan_intf_private *intfdata;
int i;
+ unsigned long flags;
port = urb-context;
intfdata = port-serial-private;
@@ -319,9 +320,9 @@ static void usb_wwan_outdat_callback(struct urb *urb)
usb_serial_port_softint(port);
usb_autopm_put_interface_async(port-serial-interface);
portdata = usb_get_serial_port_data(port);
- spin_lock(intfdata-susp_lock);
+ spin_lock_irqsave(intfdata-susp_lock, flags);
intfdata-in_flight--;
- spin_unlock(intfdata-susp_lock);
+ spin_unlock_irqrestore(intfdata-susp_lock, flags);
for (i = 0; i N_OUT_URB; ++i) {
if (portdata-out_urbs[i] == urb) {
--
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 v1 21/49] USB: serial: ti_usb_3410_5052: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/ti_usb_3410_5052.c |9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
b/drivers/usb/serial/ti_usb_3410_5052.c
index 760b785..e955ee7 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1013,6 +1013,7 @@ static void ti_bulk_in_callback(struct urb *urb)
struct device *dev = urb-dev-dev;
int status = urb-status;
int retval = 0;
+ unsigned long flags;
switch (status) {
case 0:
@@ -1046,20 +1047,20 @@ static void ti_bulk_in_callback(struct urb *urb)
__func__);
else
ti_recv(port, urb-transfer_buffer, urb-actual_length);
- spin_lock(tport-tp_lock);
+ spin_lock_irqsave(tport-tp_lock, flags);
port-icount.rx += urb-actual_length;
- spin_unlock(tport-tp_lock);
+ spin_unlock_irqrestore(tport-tp_lock, flags);
}
exit:
/* continue to read unless stopping */
- spin_lock(tport-tp_lock);
+ spin_lock_irqsave(tport-tp_lock, flags);
if (tport-tp_read_urb_state == TI_READ_URB_RUNNING)
retval = usb_submit_urb(urb, GFP_ATOMIC);
else if (tport-tp_read_urb_state == TI_READ_URB_STOPPING)
tport-tp_read_urb_state = TI_READ_URB_STOPPED;
- spin_unlock(tport-tp_lock);
+ spin_unlock_irqrestore(tport-tp_lock, flags);
if (retval)
dev_err(dev, %s - resubmit read urb failed, %d\n,
__func__, retval);
--
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 v1 19/49] USB: serial: sierra: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Johan Hovold jhov...@gmail.com Signed-off-by: Ming Lei ming@canonical.com --- drivers/usb/serial/sierra.c |9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c index de958c5..e79b6ad 100644 --- a/drivers/usb/serial/sierra.c +++ b/drivers/usb/serial/sierra.c @@ -433,6 +433,7 @@ static void sierra_outdat_callback(struct urb *urb) struct sierra_port_private *portdata = usb_get_serial_port_data(port); struct sierra_intf_private *intfdata; int status = urb-status; + unsigned long flags; intfdata = port-serial-private; @@ -443,12 +444,12 @@ static void sierra_outdat_callback(struct urb *urb) dev_dbg(port-dev, %s - nonzero write bulk status received: %d\n, __func__, status); - spin_lock(portdata-lock); + spin_lock_irqsave(portdata-lock, flags); --portdata-outstanding_urbs; - spin_unlock(portdata-lock); - spin_lock(intfdata-susp_lock); + spin_unlock_irqrestore(portdata-lock, flags); + spin_lock_irqsave(intfdata-susp_lock, flags); --intfdata-in_flight; - spin_unlock(intfdata-susp_lock); + spin_unlock_irqrestore(intfdata-susp_lock, flags); usb_serial_port_softint(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 v1 23/49] hid: usbhid: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Jiri Kosina jkos...@suse.cz
Cc: linux-in...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/hid/usbhid/hid-core.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index bd38cdf..2445fd6 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -489,8 +489,9 @@ static void hid_ctrl(struct urb *urb)
struct hid_device *hid = urb-context;
struct usbhid_device *usbhid = hid-driver_data;
int unplug = 0, status = urb-status;
+ unsigned long flags;
- spin_lock(usbhid-lock);
+ spin_lock_irqsave(usbhid-lock, flags);
switch (status) {
case 0: /* success */
@@ -525,7 +526,7 @@ static void hid_ctrl(struct urb *urb)
}
clear_bit(HID_CTRL_RUNNING, usbhid-iofl);
- spin_unlock(usbhid-lock);
+ spin_unlock_irqrestore(usbhid-lock, flags);
usb_autopm_put_interface_async(usbhid-intf);
wake_up(usbhid-wait);
}
--
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
