On Tue, Apr 24, 2012 at 11:20 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Tue, 24 Apr 2012, Ming Lei wrote: > >> > Instead of changing return codes or adding locks, how about >> > implementing a small state machine for each URB? >> > >> > Initially the state is ACTIVE. >> > >> > When the URB times out, acquire the lock. If the state is not >> > equal to ACTIVE, drop the lock and return immediately (the URB >> > is being unlinked concurrently). Otherwise set the state to >> > UNLINK_STARTED, drop the lock, call usb_unlink_urb, and >> > reacquire the lock. If the state hasn't changed, set it back >> > to ACTIVE. But if the state has changed to UNLINK_FINISHED, >> > set it to ACTIVE and resubmit. >> > >> > In the completion handler, grab the lock. If the state >> > is ACTIVE, resubmit. But if the state is UNLINK_STARTED, >> > change it to UNLINK_FINISHED and don't resubmit. >> >> Sounds a smart solution, but extra care should be put on submit urb >> in other context, maybe the lock is to be held to check URB state before >> submitting it because the lock is released during usb_unlink_urb. > > Don't you face a similar problem right now? What happens if the > timeout expires while the URB completion handler is running? Then the > timer routine could end up unlinking the resubmitted URB instead of the > original transfer. > > You need a sort of "generation number", or something like that, to > prevent this kind of mistake. The same mechanism should work with the > state machine. > > Are you saying that every place where the URB is submitted, the state > has to be set to ACTIVE first, and this has to done with the lock held? Maybe it is not enough. If the URB state is set as UNLINK_STARTED and the lock is released, will you continue the submitting? > Yes, that's right. > >> Also basically the URB state is maintained by device driver, so looks >> better to add the state into specific device driver instead of struct URB to >> save each URB storage, but if so, the solution will become more >> complicated than the way of adding lock. > > Why? In one case you add a state variable, in the other case you add a > spinlock. One isn't much harder than the other. I mean the whole URB state machine is to be maintained by device driver, not simply adding a state variable. But for the way of adding lock, just several lines of spin_lock/spin_unlock with check on urb->status are enough. Looks both ways are generic enough to avoid the problem, and adding lock is simpler. 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
