On Tue, Jul 28, 2020 at 12:47:30PM +0300, Eli Billauer wrote:
> Oliver, Alan,
>
> I understand that there's a disagreement in what is allowed or not with the
> anchor API. Effectively that means that I have to assume that driver
> programmers will go either way. I have to admit that my view was that a
> driver must proactively make sure it doesn't submit further URBs to an
> anchor as long as usb_kill_anchored_urbs() runs, through completers or
> otherwise. I formed the current patch accordingly.
>
> To make things trickier, a driver might rely (correctly or not) on that
> usb_kill_urb() makes sure that resubmission of a URB by the completer, while
> usb_kill_urb() is killing it, will fail. Or at least so says the description
> of this function.
>
> And once again, the resubmitted URB will remain untouched if the said race
> condition occurs. So a driver's programmer, who relied on usb_kill_urb() to
> prevent the resubmission, might get the impression that he did correctly
> when testing the driver, but then the kernel panic will happen rarely and
> far from the eye.
>
> Writing an additional API without this problem is beyond the scope of this
> discussion. I'm focused on resolving the problem of the current one. The
> existing API must be safe to use, even if it's planned to phase out.
>
> Given the discussion so far, I realized that the resubmission by completer
> case must be handled properly as well. So I suggest modifying the patch to
> something like
>
> do {
> spin_lock_irq(&anchor->lock);
> while (!list_empty(&anchor->urb_list)) {
> /* URB kill loop */
> }
> spin_unlock_irq(&anchor->lock);
> } while (unlikely(!usb_anchor_check_wakeup(anchor)));
>
> The do-while loop will almost never make any difference. But it will loop
> like a waiting spinlock in the rare event of the said race condition, while
> the completer callback executes.
>
> And if the completer submitted a URB, it will be removed as well this way.
> Recall that this loops only in the event of a race condition, so it will NOT
> play cat-and-mouse with the completer callback, but rather finish this up
> rather quickly.
>
> And I've dropped the WARN(): If some people consider resubmission of a URB
> to be OK, even while usb_kill_anchored_urbs() is called, no noise should be
> made if that causes a rare but tricky situation.
>
> And since I'm at it, I'll make the same change to
> usb_poison_anchored_urbs(), which suffers from exactly the same problem.
>
> What do you think?
This seems like a good way to fix up the existing API, until drivers can
be converted over to Oliver's "mooring" scheme. Of course, Oliver might
not agree...
Alan Stern
