[src/trunk]: src/sys/dev/usb xhci(4): Fix edge case in simultaneous xfer abor...

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/53359113608e
branches:  trunk
changeset: 366106:53359113608e
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Sat May 14 19:44:26 2022 +0000

description:
xhci(4): Fix edge case in simultaneous xfer abort and failure.

On successful usbd_xfer_trycomplete, caller must set ux_status and
call usb_transfer_complete before releasing the pipe (bus) lock.
Failing to call usb_transfer_complete is a mistake.  Presumably this
was intended to claim the xfer to complete it only on the last
packet.

I previously introduced the violation of this rule when the code
looked like

        xfer->ux_status = err;
        if (trb stuff)
                usb_transfer_complete(xfer);

I mostly mechanically changed all the assignments of xfer->ux_status
to do usbd_xfer_trycomplete first and then usb_transfer_complete.

In the original, the extra assignment of xfer->ux_status in the event
we _don't_ immediately call usb_transfer_complete was likely
redundant and (except insofar as the abort protocol was broken)
harmless.  But now it is a problem because of the contract between
usbd_xfer_trycomplete and usb_transfer_complete under the pipe (bus)
lock.  In retrospect, the original probably should have been

        if (trb stuff) {
                xfer->ux_status = err;
                usb_transfer_complete(xfer);
        }

and my mechanical transformation should have worked, but also in
retrospect I should have put more thought into the change and done it
a little less mechanically.

diffstat:

 sys/dev/usb/xhci.c |  24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diffs (47 lines):

diff -r ae01e2a5be5b -r 53359113608e sys/dev/usb/xhci.c
--- a/sys/dev/usb/xhci.c        Sat May 14 17:52:10 2022 +0000
+++ b/sys/dev/usb/xhci.c        Sat May 14 19:44:26 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: xhci.c,v 1.164 2022/04/06 22:01:45 mlelstv Exp $       */
+/*     $NetBSD: xhci.c,v 1.165 2022/05/14 19:44:26 riastradh Exp $     */
 
 /*
  * Copyright (c) 2013 Jonathan A. Kollasch
@@ -34,7 +34,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: xhci.c,v 1.164 2022/04/06 22:01:45 mlelstv Exp $");
+__KERNEL_RCSID(0, "$NetBSD: xhci.c,v 1.165 2022/05/14 19:44:26 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_usb.h"
@@ -2473,18 +2473,18 @@
                break;
        }
 
-       /*
-        * Try to claim this xfer for completion.  If it has already
-        * completed or aborted, drop it on the floor.
-        */
-       if (!usbd_xfer_trycomplete(xfer))
-               return;
-
-       /* Set the status.  */
-       xfer->ux_status = err;
-
        if ((trb_3 & XHCI_TRB_3_ED_BIT) == 0 ||
            (trb_0 & 0x3) == 0x0) {
+               /*
+                * Try to claim this xfer for completion.  If it has
+                * already completed or aborted, drop it on the floor.
+                */
+               if (!usbd_xfer_trycomplete(xfer))
+                       return;
+
+               /* Set the status.  */
+               xfer->ux_status = err;
+
                usb_transfer_complete(xfer);
        }
 }