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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
