On Sat, 24 Aug 2013, Ming Lei wrote: > >> >> 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? Yes, I am sure, and no, there is no bug. > >> 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. 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. > >> 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. 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 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 driver has called usb_hcd_giveback_urb, the completion handler may not have run yet. > >> 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. Hmmm. For one thing, the patch doesn't include a definition of hcd_is_urb_completing(). For another, it won't work if the driver keeps a pool of URBs and shares them among multiple endpoints. > 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? I posted a proposal for exactly this sort of thing a few weeks ago. The idea was that the URBs have to vary in size, depending on what the device is doing, so you can't predict in advance how much data a single URB will contain. But the driver wants to keep about the same amount of data on the output queue at all times. Therefore the completion handler will sometimes not submit any URBs, and sometimes it will submit more than one. > 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. There's a much simpler way to solve this problem. I will post an RFC later on. > > 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. Percpu variables are not always the best answer to everything! :-) > > 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 don't think there is no > irq handling delay. > > If you mean other things, could you explain in a bit detail please? 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? 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. 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. When this happens, ehci-hcd has to take special care not to link those TDs. I have written a patch to do this, and I will post it along with the RFC mentioned above. > >> ehci->async_unlinking > >> ehci->intr_unlinking > >> qh->exception > >> QH_STATE_COMPLETING > > > > Each of those uses about three lines of code. And you're wrong about > > qh->exception; that one is needed (or will be needed) for other > > reasons. > > No, with my patches, about 50 line codes can be saved. Also it may be not > related code decrease only, it is about race-killing, and you know people > always hate races, :-) The races have been taken care of for years. I prefer not to remove the code that handles those races, because there's a good chance it will be needed in the future. (For example, if someone decides to move ehci_work into the bottom half, as in my previous RFC.) > > It turns out that what I did in the RFC wasn't a valid solution, > > though. It will be necessary to make sure that givebacks never happen > > in an unlink call or timer handler; the tasklet will have to take care > > of them instead. That will complicate the patch, unfortunately. > > Yes, imagine other HCDs still need to deal with similar & complicated > things, so why don't we deal with most of things in usb core like only > giveback in tasklet patches? I wish we could, but we can't! All these other changes needed in ehci-hcd are proof of this -- they are things that _can't_ be dealt with in usbcore. > For ehci HCD example, ehci->need_rescan is set and returns when > HCD is scanning, so the problem is avoided, but it is implemented in > HCD code, and other HCDs may need to solve the same problem > with similar trick too.(UHCI uses similar trick) Of course they do. If they haven't solved this problem already then they are buggy, and have been buggy for a long time. > I mean we can do it in usb core like giveback in tasklet patches, > so HCDs won't worry about the problem any more, isn't it a > simplification for HCDs? 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. 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
