Re: [PATCH v2 2/3] xhci: Set URB actual length for stopped control transfers

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 27.03.2017 17:47, Felipe Balbi wrote:
>
> Hi,
>
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>> A control transfer that stopped at the status stage incorrectly
>> warned about a "unexpected TRB Type 4", and did not set the
>> transferred actual_length for the URB.
>>
>> The transferred actual_length should be set the same way for
>> COMP_STOPPED control transfers as in the generic cases.
>>
>> generic case if we get an event at:
>>
>> TRB_SETUP stage:
>>    length = 0;
>>
>> TRB_DATA/TRB_NORMAL state:
>>    length = requested - remaining;
>>
>> TRB_STATUS stage:
>>    length = requested
>>
>> The URB actual_length for control transfers doesn't care about sent
>> bytes of the SETUP stage, or remaining bytes of the  STATUS stage.
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>   drivers/usb/host/xhci-ring.c | 20 +++++---------------
>>   1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index d9936c7..584b6fe 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1975,25 +1975,15 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
>>   		*status = 0;
>>   		break;
>>   	case COMP_STOPPED_SHORT_PACKET:
>> -		if (trb_type == TRB_DATA || trb_type == TRB_NORMAL)
>> +		if (trb_type == TRB_DATA || trb_type == TRB_NORMAL) {
>> +			td->urb_length_set = true;
>>   			td->urb->actual_length = remaining;
>> -		else
>> +		} else {
>>   			xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl setup or status TRB\n");
>> +		}
>>   		goto finish_td;
>>   	case COMP_STOPPED:
>> -		switch (trb_type) {
>> -		case TRB_SETUP:
>> -			td->urb->actual_length = 0;
>> -			goto finish_td;
>> -		case TRB_DATA:
>> -		case TRB_NORMAL:
>> -			td->urb->actual_length = requested - remaining;
>> -			goto finish_td;
>> -		default:
>> -			xhci_warn(xhci, "WARN: unexpected TRB Type %d\n",
>> -				  trb_type);
>> -			goto finish_td;
>> -		}
>> +		break;
>
> instead of this giant patch, why didn't you just add case TRB_STATUS to
> the switch statement above? It would've been a single line that would
> solve your problem.

giant ?
1 file changed, 5 insertions(+), 15 deletions(-)

why add an extra case when it can be handled with the generic case?
This is almost code duplication already.

>
> Not to mention that we loose the xhci_warn() for unexpected TRB types
> which might help finding valid issues (such as $subject).

That warning was added recently and turned out to be more harmful than useful.
The type can be seen from traces instead.

We're not warning about TRB types in other cases. And we would need to
worry about link, noop, event-data types as well.

>
> Note that now we do nothing on COMP_STOPPED, this means that we're
> relying on some generic case that, arguably, could be removed
> altogether. $subject is just re-adding part of obfuscation we had and
> took so much work to clean up.

?

It's the other way around.
We want the generic way to handle as much as it can, like the successful cases,
the short packets etc. we only want to mess with it when absolutely necessary

-Mathias