On Mon, 19 Aug 2013, Ming Lei wrote: > This patch kills atomic_inc/atomic_dec operations on > urb->use_count in URB submit/complete path. > > The urb->use_count is only used for unlinking URB, and it isn't > necessary defined as atomic counter, so the variable is renamed > as urb->use_flag for this purpose, then reading/writing the > flag is still kept as atomic but ARCH's atomic operations(atomic_inc/ > atomic_dec) are saved. "Flag" is the wrong word. It implies a binary value, but the use_count can take on 3 values. Also, the names you selected aren't very good. I'd suggest something like the following: URB_INACTIVE (or URB_IDLE), URB_ACTIVE, URB_GIVING_BACK, URB_RESUBMITTED (or URB_ACTIVE_AND_GIVING_BACK) The state transitions will then be much clearer. But a counter seems equally clear to me. Besides, there's no way to avoid using atomic operations here. Your patch does this in __usb_hcd_giveback_urb(): > - atomic_dec(&urb->use_count); > + if (atomic_read(&urb->use_flag) == URB_UNUSING) > + atomic_set(&urb->use_flag, URB_UNUSED); This has a race. What happens if the driver submits the URB after the atomic_read and before the atomic_set? 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
