Now
MAIN commitmail json YAML
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.
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.