Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1348016 > unrolled thread

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

Started byMathias Nyman <mathias.nyman@linux.intel.com>
First post2016-03-02 14:00 +0100
Last post2016-03-03 07:10 +0100
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 07/10] usb: dbc: handle endpoint stall Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-03-02 14:00 +0100
    Re: [PATCH v7 07/10] usb: dbc: handle endpoint stall Felipe Balbi <balbi@kernel.org> - 2016-03-02 14:00 +0100
      Re: [PATCH v7 07/10] usb: dbc: handle endpoint stall Lu Baolu <baolu.lu@linux.intel.com> - 2016-03-03 07:20 +0100
    Re: [PATCH v7 07/10] usb: dbc: handle endpoint stall Lu Baolu <baolu.lu@linux.intel.com> - 2016-03-03 07:10 +0100

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

FromMathias Nyman <mathias.nyman@linux.intel.com>
Date2016-03-02 14:00 +0100
SubjectRe: [PATCH v7 07/10] usb: dbc: handle endpoint stall
Message-ID<r8exI-7Hb-15@gated-at.bofh.it>
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 (..)


+
>   static int xdbc_bulk_transfer(void *data, int size, int loops, bool read)
>   {
>   	u64 addr;
> @@ -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.
In this case the failure is that debug host failed to send a clearFeature(EP_HALT) request in time.

-Mathias

[toc] | [next] | [standalone]

#1348018

FromFelipe Balbi <balbi@kernel.org>
Date2016-03-02 14:00 +0100
Message-ID<r8exJ-7Hb-23@gated-at.bofh.it>
In reply to#1348016

[Multipart message — attachments visible in raw view] — view raw

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 (...)

>> @@ -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 ?

> 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 ?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1348808

FromLu Baolu <baolu.lu@linux.intel.com>
Date2016-03-03 07:20 +0100
Message-ID<r8uM9-2Y4-1@gated-at.bofh.it>
In reply to#1348018

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

>

[toc] | [prev] | [next] | [standalone]

#1348806

FromLu Baolu <baolu.lu@linux.intel.com>
Date2016-03-03 07:10 +0100
Message-ID<r8uCu-2Tn-19@gated-at.bofh.it>
In reply to#1348016

On 03/02/2016 08:58 PM, Mathias Nyman wrote:
> 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 (..)

Sure. I will refine this part of code.

>
>
> +
>>   static int xdbc_bulk_transfer(void *data, int size, int loops, bool read)
>>   {
>>       u64 addr;
>> @@ -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.
> In this case the failure is that debug host failed to send a clearFeature(EP_HALT) request in time.

Yes. This is done by debug host firmware. OS needs to check and wait.

This return value is used to tell the up layer what happened. -EPERM is definitely not appropriate here.
I will use EIO as suggested by Felipe.

>
> -Mathias
>

Thanks,
-Baolu

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web