On Monday 11 April 2005 2:26 pm, Alan Stern wrote: > On Mon, 11 Apr 2005, David Brownell wrote: > > > > That's just it. The first part applies only when an URB completes with a > > > hardware error, _not_ an unlink or kill! > > > > It was intended to cover _all_ fault reports. Unlink/kill is just > > a software-induced fault. > > Alright. I'll send an update that clearly states the first part refers > only to hardware errors, not software-induced faults (unlink or kill).
IMO not the way to go. There should be no real differences in how they're handled. Requiring different behavior asks for trouble .... multiple paths through complex code, which must act differently. I'd much rather eliminate special cases; that's most likely to increase robustness/reliability. > > I suspect a lurking issue here may really be that UHCI still > > has some issues with how it handles unlinking, which may not be > > fixable until it actually has a solid notion of endpoint queue. > > (Call that leftover 2.4-ism, if so. The whole notion of even > > being able to unlink _just one_ request is very dubious; if you > > look at the USB spec, it talks about killing _all_ IRPs at once.) > > Actually this is a new issue, not a lurking one. The whole point of that > patch was because I'm going to make the majority of the UHCI driver run in > a bottom half (a tasklet). This includes most of the work involved in > submitting and unlinking URBs. Erm ... why? That'll mean many of the relevant data structures will more likely be dcache-cold. And it'll involve more code to execute (icache-cold) and maintain too ... For unlinking, the operations should be just patching the schedule (under spinlock) and requesting an IRQ. Unlinking is both rare and, when it happens, on the critical path. Submitting is likewise on the critical path ... though it happens a lot. Neither of those would seem to me like good candidates for putting into a tasklet. I could see in some cases wanting the IRQ processing to be split into the classic top half (in_irq) and bottom half. I even had EHCI working that way, originally ... DaveM removed that, mostly to ensure that pt_regs got delivered to keyboard IRQ handlers (evidently some keyboard code really wants pt_regs), but also to make it a bit simpler. Is this because of those no-details-provided reports of high IRQ latencies with USB on one board? It'd have been interesting to see some accounting for those latencies. For example, oprofile traces of where the IRQ handlers spend time... > > > This may well occur before all the > > > preceding URBs have terminated, since completion handlers are allowed to > > > run out of sequence when an URB is unlinked. > > > > To safely run that out of sequence, its queue clearly stopped in some > > sense. > > Not if the URB was never in the hardware queue to begin with. From the usbcore perspective, there's only one queue. If you're proposing that usbcore start needing to think about two separate queues, that's significantly more invasive than anything that's happened in usbcore over the past few years (including the almost total rewrite of the hub and enumeration code)... > Yes, in > some sense it is on a queue of URBs waiting for the tasklet to add them to > the schedule, and _that_ queue will be stopped. But that queue isn't > associated with the hardware or with any particular endpoint. So it's a > question of how picky you want to get. I want to get picky about API requirements that expect (or facilitate) complex and error prone logic in HCDs, or which promote substantially different HCD behavior as visible to the USB device/interface drivers; yes. > > OK, so that new text is wrong then. The only problem case has ever > > been (3b), where the URB completed (at the hardware level) before the > > queue could be stopped. > > Nope, the new text describes exactly how the new driver will work. And > it's correct. (3b) isn't a problem in the sense that we don't violate the > guarantees made by the API. The queue _does_ stop, but it might not stop > until after the hardware has begun working on the URB following the one > that was unlinked. That is, you turn it into (3a) somehow. Which was handled by the original text ... Morphing (3b) into (3a) implies some sort of additional mechanism to couple completion reports to the (hardware) endpoint queue. > > So I'm not too worried about case (3b), although I think it should > > get fixed someday ... by removing the "unlink one URB" semantics, > > replacing them with a "kill this endpoint queue" request which is > > not "buggy-as-designed" (because of a hardware race in the API). > > We discussed this not long ago, remember? People generally were against > the idea of removing the "unlink one URB" operation. And there were a few > cases where the "unlink one" semantics really are needed -- ep0 being > one example. Actually I remember it differently. EP0 being the _only_ example of a case where it's potentially needed (because it's shared between interfaces), but otherwise folk liked removing hardware races from the APIs. I don't remember anyone speaking "against" that, except in the case of ep0 ... the discussion stopped only because of a lack of energy to rewrite the HCDs to address such API design bugs. (One observation about ep0: it's effectively a queue-of-queues, since each ep0 request involves multiple grouped transfers. For each of those transfers, the three cases I listed will apply.) - Dave ------------------------------------------------------- SF email is sponsored by - The IT Product Guide Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now. http://ads.osdn.com/?ad_ide95&alloc_id396&op=click _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
