On Tue, Aug 27, 2013 at 10:39 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Tue, 27 Aug 2013, Ming Lei wrote: > >> > 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... > > Yes. The pattern would be pretty much the same in each case. > >> >> 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) > > Those things would definitely need to be considered. But doing them > properly wouldn't require much code. > >> > 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. > > That's true. It's also true that the existing driver handles them with > delay. If an IAA, connection, or fatal error event occurs while > ehci_work() is running, the event won't be handled until after > ehci_work() finishes. If fatal error can be reported early, ehci_work() can scan and handle qh/iTD/siTD easily(quickly) since ehci state is checked at many places. > > I agree that masking interrupts while the bottom half runs would > increase this delay, but I don't think it matters much. For example, > consider disconnect events. Leaving interrupts masked might delay the > event report for 10 ms (probably a lot less). But compare that to what > happens when a device disconnects from an external hub -- hubs are > polled for port status changes with an interval of 128 ms! By > comparison, a 10-ms delay for the root hub is unimportant. > >> >> 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. > > That was my point. We are in agreement that it is bad for the top half > and the bottom half both to acquire ehci->lock. Your solution is to > put everything but the givebacks (which don't need the lock) in the top > half, whereas my solution is to put everything in the bottom half. > But now you're complaining because that means some of the work won't > get done in the top half! > > You can't have it both ways. If the lock isn't used in both places > then the top half has to do either all or none of the work. It can be done with extra complication introduced, but that is we don't want to see. > >> >> 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). > > You haven't thought this through. _Every_ QH and TD written by the > host controller eventually gets scanned by ehci-hcd. This is true > whether or not a fatal error occurs. The existing driver doesn't do > anything special about incorrect TDs, and it never has. So why should > we have to start doing it now? > > Similar considerations apply to connect and disconnect handling. > Suppose a connect event occurs while ehci_work() is running. Suppose > that the event can be handled without acquiring ehci->lock, so that > the event is reported to usbcore immediately. What would happen next? > > Answer: Before doing anything else, khubd would issue a Get-Port-Status > request, which acquires ehci->lock! Thus the response to the connect > event would end up being delayed anyway. > > The one disadvantage to leaving interrupts masked while the tasklet > runs is that new events won't be detected until all the existing > givebacks are finished. In the old driver, new events could be > detected as soon as the next giveback occurred, because the lock would > be released then. > > In the end, this comes down to a decision about priorities. Do you > want to give higher priority to new events or to givebacks? Overall, I > don't think it matters. If possible, the events should be put higher priority, like events handling in ehci_irq() now, but as you said, it may not matter. So from the discussion, both approaches are pretty much same I have no objection if anyone plan to implement this approach. 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
