On Mon, Aug 26, 2013 at 11:29 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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 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). > private lock before doing any significant processing (i.e., anything > more than scheduling the tasklet). This would force it to spin until > the tasklet was finished running, which would definitely be a > disadvantage. Yes, that is why I said it isn't easy to add more things in irq handler in your approach. > >> I understand it might still depend on irqs_are_masked assignment to be >> ordered before the write to the intr_enable register, but not true for >> shared interrupts. >> >> Suppose there are two barriers around irq handler, so >> >> CPU0 CPU1 >> >> smp_mb() >> read A >> write A >> smp_mb() >> smp_mb() >> read A >> write A >> smp_mb() >> >> Looks we can't make sure 'read A' on CPU1 may observe >> the latest value of A updated in CPU0, if there isn't one 'write C' >> after the 2nd barrier on CPU0 and one 'read C' before the >> 1st barrier on CPU1. >> >> Even I am not sure if there are implicit memory barriers inside system >> irq handling path since Documentation/memory-barriers.txt doesn't >> mention the point. > > Let's raise this question on LKML and CC: David Howells and Paul > McKenney. I'll post a message. That is great, :-) But you needn't worry about memory synchronization any more since we concluded one lock is required. 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
