Re: [PATCHv2 1/4] xhci: don't start a halted endpoint before its new dequeue is set

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 17.11.2014 16:52, David Laight wrote:
> From: Mathias Nyman
>> A halted endpoint ring must first be reset, then move the ring
>> dequeue pointer past the problematic TRB. If we start the ring too
>> early after reset, but before moving the dequeue pointer we
>> will end up executing the same problematic TRB again.
>>
>> As we always issue a set transfer dequeue command after a reset
>> endpoint command we can skip starting endpoint rings at reset endpoint
>> command completion.
>>
>> Without this fix we end up trying to handle the same faulty TD for
>> contol endpoints. causing timeout, and failing testusb ctrl_out write
>> tests.
>>
>> Fixes: e9df17e (USB: xhci: Correct assumptions about number of rings per endpoint.)
>> Cc: <stable@xxxxxxxxxxxxxxx> #v2.6.35
>> Tested-by: Felipe Balbi <balbi@xxxxxx>
>> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/usb/host/xhci-ring.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index bc6fcbc..d6646db 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1069,7 +1069,6 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,
>>  	} else {
>>  		/* Clear our internal halted state and restart the ring(s) */
>>  		xhci->devs[slot_id]->eps[ep_index].ep_state &= ~EP_HALTED;
>> -		ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
> 
> The comment is now wrong - you aren't restarting the ring(s) here.

Thats right, comment is still wrong, thanks.
I'll resend.

> 
> Also I'm not at all certain that delaying 'ringing the doorbell' is in anyway
> guaranteed to stop the xhci hardware from processing the next ring entry.
> 
> It might be true if the ring is absolutely known to be inactive, and that
> the controller isn't polling the rings on timeout.
> But if the controller is still processing the previous ring entry it
> will process the next one without the bell being rung.
> Plausibly some other driver path might also ring the bell.

The ring is known to be inactive. The only use for reset endpoint command is to
revocer a halted endpoint ring. A reset endpoint on a non-halted endpoint will fail.

A reset endpoint command will move the ring from a halted to a stopped state.
Ringing the doorbell will restart the stopped ring. But we don't want to ring the doorbell
until we moved past the problematic TRB with a set dq pointer command.

xhci 4.6.8:

The Reset Endpoint Command is issued by software to recover from a halted condition on an endpoint.
...
If the endpoint is not in the Halted state when an Reset Endpoint Command is executed:
The xHC shall reject the command and generate a Command Completion Event with the
Completion Code set to Context State Error.


> 
> For safety you shouldn't change the 'cycle' bit until it is safe
> for the controller to process the ring(s).
> 

It won't, as said, reset endpoint will only work if endpoint is halted. 

-Mathias

--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]