On Tue, Aug 27, 2013 at 4:05 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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 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-. :-) No, I doesn't mean a contest, :-) > >> > 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. Yes, but the TDs(URB) can't be missed, suppose there are 8 packets, so the last packet of the new URB will be completed in 164, when IRQ comes at 165, ehci_irq() will scan from 104 to 165, so the URB can be completed successfully, then update last_iso_frame as 164. Then what is wrong? > > 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. iso_stream_schedule() will figure out that the next slot(201) is behind the scheduling threshold(case of 'start < next'), then update the next slot as one new slot which is larger 206 if URB_ISO_ASAP, so looks the TDs(USB) can't be missed too. Without URB_ISO_ASAP, there might be problem since 201 won't be scanned next time in scan_isoc, so is it what your RFC3 is fixing? > > 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. Yes, only applies in !URB_ISO_ASAP case. > >> > 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. If I understand the above problem correctly, the same problem exists on drivers which submit URB in tasklet scheduled from complete() too before the moving, doesn't it? Thanks, -- Ming Lei -- 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
