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. 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. > >> 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. 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
