Re: [PATCH v7 07/10] usb: dbc: handle endpoint stall

[Lu Baolu <baolu.lu@linux.intel.com>]

On 03/02/2016 08:58 PM, Felipe Balbi wrote:
> Hi,
>
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>> [ text/plain ]
>> On 26.01.2016 14:58, Lu Baolu wrote:
>>> In case of endpoint stall, software is able to detect the situation
>>> by reading DCCTRL.HIT or DCCTRL.HOT bits. DbC follows the normal USB
>>> framework to handle endpoint stall. When software detects endpoint
>>> stall situation, it should wait until endpoint is recovered before
>>> read or write oprations.
>>>
>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>   drivers/usb/early/xhci-dbc.c | 36 ++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
>>> index c81df40..344d93e 100644
>>> --- a/drivers/usb/early/xhci-dbc.c
>>> +++ b/drivers/usb/early/xhci-dbc.c
>>> @@ -1163,6 +1163,37 @@ static int xdbc_wait_until_dbc_configured(void)
>>>   	return -ETIMEDOUT;
>>>   }
>>>
>>> +static int xdbc_wait_until_epstall_cleared(bool read)
>>> +{
>>> +	int timeout = 0;
>>> +
>>> +	if (read) {
>>> +		do {
>>> +			if (!(readl(&xdbcp->xdbc_reg->control) & CTRL_HIT)) {
>>> +				xdbcp->in_ep_state = EP_RUNNING;
>>> +
>>> +				return 0;
>>> +			}
>>> +
>>> +			xdbcp->in_ep_state = EP_HALTED;
>>> +			xdbc_udelay(10);
>>> +		} while (timeout++ < XDBC_LOOPS);
>>> +	} else {
>>> +		do {
>>> +			if (!(readl(&xdbcp->xdbc_reg->control) & CTRL_HOT)) {
>>> +				xdbcp->out_ep_state = EP_RUNNING;
>>> +
>>> +				return 0;
>>> +			}
>>> +
>>> +			xdbcp->out_ep_state = EP_HALTED;
>>> +			xdbc_udelay(10);
>>> +		} while (timeout++ < XDBC_LOOPS);
>>> +	}
>>> +
>>> +	return -ETIMEDOUT;
>>> +}
>>>
>> how about something like:
>>
>> enum xdbc_ep_state *ep_state;
>> u32 halt_bit;
>>
>> if (read) {
>> 	ep_state = &xdbcp->in_ep_state
>> 	halt_bit = CTRL_HIT
>> } else {
>>   	ep_state = &xdbcp->out_ep_state
>> 	halt_bit = CTRL_HOT
>> }
>> do {
>> 	if (!(readl(..) & halt_bit)) {
>> 		*ep_state = EP_RUNNING;
>> 		return 0;
>> 	}
>> 	*ep_state = EP_HALTED;
>> 	delay()
>> } while (..)
> I'll agree, this looks better. Might also want to refactor the handshake
> loop to its own function:
>
> static int xdbg_ep_state_handshake(enum xdbc_ep_state *ep_state, u32 halt_bit)
> {
>         do {
> 		if (!(readl(...) & halt_bit)) {
>                 	...
>                 }
>                 *ep_state = EP_HALTED;
>                 delay(...);
>         } while (...)

Sure. Thanks.

>
>>> @@ -1182,6 +1213,11 @@ static int xdbc_bulk_transfer(void *data, int size, int loops, bool read)
>>>   		return -EPERM;
>>>   	}
>>>
>>> +	if (xdbc_wait_until_epstall_cleared(read)) {
>>> +		xdbc_trace("%s: endpoint not ready\n", __func__);
>>> +		return -EPERM;
>> Is -EPERM appropriate here?
>>
>> Not sure about what error codes the device side is using, but usually
>> HALT is set due to some Data buffer/transmission/TRB error.
> EIO perhaps ?

Sure. I will replace it with -EIO.

>
>> In this case the failure is that debug host failed to send a
>> clearFeature(EP_HALT) request in time.
> I haven't read the spec, but does it define a maximum time for this to
> happen ?

I took a quick look at chapter 9.4 USB3 spec. It only requires that
"ClearFeature (ENDPOINT_HALT) request results in the endpoint
no longer returning a STALL Transaction Packet". xHCI spec doesn't
define a maximum time for this either.

Thanks,
-Baolu

>