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. _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. 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. > >> 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. > 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. > > There's a much simpler way to solve this problem. I will post an RFC > > later on. > > Actually, my attachment patch is only about dozens of > lines(17+, 1-), so sounds the problem isn't difficult to solve > finally because of your simpler one, :-) If this is a contest, my 1/3 patch is 6+, 2-. :-) > > Suppose an URB is submitted while the endpoint queue is non-empty, so > > it is scheduled to start in the next slot after the end of the last URB > > in the queue. Does that next slot occur after the frame stored in > > ehci->last_iso_frame? > > I think it is yes, actually the new slot is always after current uframe > during iso_stream_schedule from tasklet, and it is behind > ehci->last_iso_frame which is updated in ehci_irq path. Wrong. Here are a couple of examples. Assume first that the endpoint's interval is 8 frames (it's a full-speed device): The last packet of the last URB on the queue is scheduled for frame 100. Because of an SMI, IRQs are delayed until the controller reaches frame 105. When the interrupt occurs, ehci-hcd will scan the siTDs for the endpoint and give them all back. At the end, last_iso_frame will be set to 104 (it lags one behind the current frame number). Let's say the tasklet takes 1 ms to start up, so the completion handler runs during frame 106. It submits an URB. This URB is scheduled for the next unused slot, which is 108 (i.e., 8 frames after the last packet of the last URB). Since 108 is larger than both 104 and 106, the next slot does indeed come after both ehci->last_iso_frame and the current frame number. Now assume the endpoint's interval is 1 frame: The last packet of the last URB on the queue is scheduled for frame 200. Because of an SMI, IRQs are delayed until the controller reaches frame 205. When the interrupt occurs, ehci-hcd will scan the siTDs for the endpoint and give them all back. At the end, last_iso_frame will be set to 204. The tasklet takes 1 ms to start up, so the completion handler runs during frame 206. It submits an URB. This URB is scheduled for the next unused slot, which is 201 (i.e., 1 frame after the last packet of the last URB). Since 201 is smaller than both 204 and 206, the next slot does indeed come before both ehci->last_iso_frame and the current frame number. In the second example, consider what ehci-hcd should do if the number of packets in the new URB is 3 (meaning the last packet would be scheduled for frame 203), or if the number of packets is 10 (the last packet would be schedule for frame 210). Remember that scan_isoc() will start scanning at frame 204 the next time it runs. > > With the old driver it always does, because the fact that the last URB > > hasn't been given back yet means that the last URB ends after > > last_iso_frame. Therefore the new URB will begin after last_iso_frame. > > If new slot of new URB is always after current uframe, there isn't the > problem. But it isn't always after the current uframe. See the second example. > > With the new driver it doesn't, because the endpoint queue will remain > > non-empty for some time after ehci-hcd calls giveback_urb for the last > > URB. As a result, the TDs for the new URB might not get scanned, > > because the scanning starts from the last_iso_frame position. > > Looks it is impossible, see above. I don't know what you mean. Consider the two examples. > > Yes, it is. But at the cost of making other things more complicated -- > > such as the way interrupt and isochronous URBs are handled in ehci-hcd. > > As I said, these other things are not only for giveback URB in tasklet > patches, but also fixes on current HCDs, which means they should have > been done no matter my patches are posted or not, :-) Not true. ehci-hcd was fine before moving to tasklets. The other drivers are fine now. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
