Hi,
Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
> Stop Endpoint command can come at any point and we
> have no control of that. We should make sure to
> handle COMP_STOP on SETUP phase as well, otherwise
> urb->actual_lenght might be set to negative values
> in some occasions such as below:
>
> urb->length = 4;
> build_control_transfer_td_for(urb, ep);
>
> stop_endpoint(ep);
>
> COMP_STOP:
> [...]
> urb->actual_length = urb->length - trb->length;
>
> trb->length is 8 for SETUP stage (8 control request
> bytes), so actual_length would be set to -4 in this
> case.
>
> While doing that, also make sure to use TRB_TYPE
> field of the actual TRB instead of matching pointers
> to figure out in which stage of the control transfer
> we got our completion event.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 76402b44f832..70067b81cc8f 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1939,8 +1939,9 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> struct xhci_ep_ctx *ep_ctx;
> u32 trb_comp_code;
> u32 remaining, requested;
> - bool on_data_stage;
> + u32 trb_type;
>
> + trb_type = TRB_FIELD_TO_TYPE(le32_to_cpu(ep_trb->generic.field[3]));
> slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
> xdev = xhci->devs[slot_id];
> ep_index = TRB_TO_EP_ID(le32_to_cpu(event->flags)) - 1;
> @@ -1950,14 +1951,11 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> requested = td->urb->transfer_buffer_length;
> remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
>
> - /* not setup (dequeue), or status stage means we are at data stage */
> - on_data_stage = (ep_trb != ep_ring->dequeue && ep_trb != td->last_trb);
> -
> switch (trb_comp_code) {
> case COMP_SUCCESS:
> - if (ep_trb != td->last_trb) {
> + if (trb_type != TRB_STATUS) {
> xhci_warn(xhci, "WARN: Success on ctrl %s TRB without IOC set?\n",
> - on_data_stage ? "data" : "setup");
> + (trb_type == TRB_DATA) ? "data" : "setup");
> *status = -ESHUTDOWN;
> break;
> }
> @@ -1967,15 +1965,23 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> *status = 0;
> break;
> case COMP_STOP_SHORT:
> - if (on_data_stage)
> + if (trb_type == TRB_DATA)
> td->urb->actual_length = remaining;
> else
> xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl setup or status TRB\n");
> goto finish_td;
> case COMP_STOP:
> - if (on_data_stage)
> + switch (trb_type) {
> + case TRB_SETUP:
> + td->urb->actual_length = 0;
> + goto finish_td;
> + case TRB_DATA:
there's a detail to fix here. Data stage can be composed of several
TRBs, in that case we will have one TRB_DATA and N TRB_NORMAL, so we
need to handle that too.
I'll update this patch (and consequently all other patches I've written)
and resend everything as a single series. Before doing that, however,
I'll wait a few more days for comments on any of the patches I've sent.
--
balbi
Attachment:
signature.asc
Description: PGP signature
