[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb
Subject:    Re: [PATCH v2 2/4] usb: host: xhci: handle COMP_STOP from SETUP phase too
From:       Felipe Balbi <felipe.balbi () linux ! intel ! com>
Date:       2016-11-30 11:43:44
Message-ID: 87mvghmbv3.fsf () linux ! intel ! com
[Download RAW message or body]

Hi,

Felipe Balbi <felipe.balbi@linux.intel.com> 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@linux.intel.com>
> ---
>  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

["signature.asc" (application/pgp-signature)]
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[prev in list] [next in list] [prev in thread] [next in thread]