Am Mittwoch, 25. April 2012, 09:02:58 schrieb Ming Lei:
> On Wed, Apr 25, 2012 at 2:19 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
>
> >> @@ -546,8 +557,13 @@ static void __usbhid_submit_report(struct
> >> hid_device *hid, struct hid_report *re
> >> * no race because this is called under
> >> * spinlock
> >> */
> >> - if (time_after(jiffies, usbhid->last_out + HZ * 5))
> >> +
> >> + if (time_after(jiffies, usbhid->last_out + HZ * 5) &&
> >> + !usbhid->urbout->unlinked) {
> >> + spin_unlock(&usbhid->lock);
> >> usb_unlink_urb(usbhid->urbout);
> >> + spin_lock(&usbhid->lock);
> >> + }
> >> }
> >> return;
> >> }
> >
> > Same objection. You are just making the race unlikelier. The flag
> > needs to be set under a lock you hold while checking time_after().
>
> urb->unlinked is checked with holding both the two lockes, but set with
> holding the unlink lock or the lock or both, looks it is enough to
> avoid the race, isn't it?
That would work. Yet, having a second lock is not the best solution.
> > We'd be back at the original proposal.
>
> Introducing a new flag to describe 'unlinked' is still OK, but
> borrowing urb->unlinked is better, the bad is that it is private.
And it needs to be explicitly checked. We'd better use urb->reject.
I've posted a patch to do so.
Regards
Oliver
--
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
