LinuxLists.cc - [PATCH v2 1/3] usb: dwc3: gadget: Fix incorrect DEPCMD and DGCMD status macros

[permalink] [raw]

Subject: [PATCH v2 2/3] usb: dwc3: gadget: return error if command sent to DGCMD register fails

We need to return error to caller if command is not sent to
controller succesfully.

Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
Fixes: b09bb64239c8 (usb: dwc3: gadget: implement Global Command support)
Cc: <[email protected]> #v3.5+
---
v2 changes:
Added Fixes and Cc in commit message.

drivers/usb/dwc3/gadget.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8946c32..fcbe120 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -291,6 +291,8 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 param)
dwc3_trace(trace_dwc3_gadget,
"Command Complete --> %d",
DWC3_DGCMD_STATUS(reg));
+ if (DWC3_DGCMD_STATUS(reg))
+ return -EINVAL;
return 0;
}

--
1.7.4

2015-05-21 10:32:21

[permalink] [raw]

Subject: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

We need to return error to caller if command is not sent to
controller succesfully.

Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
Cc: <[email protected]>
---
v2 changes:
Added Fixes and Cc in commit message.

drivers/usb/dwc3/gadget.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fcbe120..55b5edc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -330,6 +330,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
dwc3_trace(trace_dwc3_gadget,
"Command Complete --> %d",
DWC3_DEPCMD_STATUS(reg));
+ if (DWC3_DEPCMD_STATUS(reg))
+ return -EINVAL;
return 0;
}

--
1.7.4

2015-06-29 21:47:11

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi,

On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta wrote:
> We need to return error to caller if command is not sent to
> controller succesfully.
>
> Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> Cc: <[email protected]>
> ---
> v2 changes:
> Added Fixes and Cc in commit message.

I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
when used with g_zero and testusb. As of now, it could be that silicon
is mis-behaving because I got a Transfer Complete before the failing Set
Endpoint Transfer Resource command.

In any case, can you run on your setup with g_zero and test.sh/testusb
[1]/[2] just to verify that it really works for you ?

Meanwhile, I'll continue testing on my end.

cheers

[1] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:testusb.c
[2] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:test.sh

--
balbi

Attachments:

(No filename) (1.00 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-06-29 21:49:07

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi again,

On Mon, Jun 29, 2015 at 04:47:01PM -0500, Felipe Balbi wrote:
> On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta wrote:
> > We need to return error to caller if command is not sent to
> > controller succesfully.
> >
> > Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> > Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> > Cc: <[email protected]>
> > ---
> > v2 changes:
> > Added Fixes and Cc in commit message.
>
> I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
> when used with g_zero and testusb. As of now, it could be that silicon
> is mis-behaving because I got a Transfer Complete before the failing Set
> Endpoint Transfer Resource command.
>
> In any case, can you run on your setup with g_zero and test.sh/testusb
> [1]/[2] just to verify that it really works for you ?
>
> Meanwhile, I'll continue testing on my end.
>
> cheers
>
> [1] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:testusb.c
> [2] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:test.sh

Adding John here. John, any chance you could fire up dwc3 on HAPS and
see wether it works or fails for you ?

cheers

--
balbi

Attachments:

(No filename) (1.23 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-06-29 23:59:52

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

On 6/29/2015 2:48 PM, Felipe Balbi wrote:
> Hi again,
>
> On Mon, Jun 29, 2015 at 04:47:01PM -0500, Felipe Balbi wrote:
>> On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta wrote:
>>> We need to return error to caller if command is not sent to
>>> controller succesfully.
>>>
>>> Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
>>> Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
>>> Cc: <[email protected]>
>>> ---
>>> v2 changes:
>>> Added Fixes and Cc in commit message.
>>
>> I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
>> when used with g_zero and testusb. As of now, it could be that silicon
>> is mis-behaving because I got a Transfer Complete before the failing Set
>> Endpoint Transfer Resource command.
>>
>> In any case, can you run on your setup with g_zero and test.sh/testusb
>> [1]/[2] just to verify that it really works for you ?
>>
>> Meanwhile, I'll continue testing on my end.
>>
>> cheers
>>
>> [1] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:testusb.c
>> [2] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:test.sh
>
> Adding John here. John, any chance you could fire up dwc3 on HAPS and
> see wether it works or fails for you ?
>

Sure. I'll try to get to it tomorrow.

John

2015-06-30 00:34:27

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

On Mon, Jun 29, 2015 at 11:59:42PM +0000, John Youn wrote:
> On 6/29/2015 2:48 PM, Felipe Balbi wrote:
> > Hi again,
> >
> > On Mon, Jun 29, 2015 at 04:47:01PM -0500, Felipe Balbi wrote:
> >> On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta wrote:
> >>> We need to return error to caller if command is not sent to
> >>> controller succesfully.
> >>>
> >>> Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> >>> Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> >>> Cc: <[email protected]>
> >>> ---
> >>> v2 changes:
> >>> Added Fixes and Cc in commit message.
> >>
> >> I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
> >> when used with g_zero and testusb. As of now, it could be that silicon
> >> is mis-behaving because I got a Transfer Complete before the failing Set
> >> Endpoint Transfer Resource command.
> >>
> >> In any case, can you run on your setup with g_zero and test.sh/testusb
> >> [1]/[2] just to verify that it really works for you ?
> >>
> >> Meanwhile, I'll continue testing on my end.
> >>
> >> cheers
> >>
> >> [1] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:testusb.c
> >> [2] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:test.sh
> >
> > Adding John here. John, any chance you could fire up dwc3 on HAPS and
> > see wether it works or fails for you ?
> >
>
> Sure. I'll try to get to it tomorrow.

Thanks a lot :-) If you want to get low latency traces from dwc3 you
can:

# mount -t debugfs none /sys/kernel/debug
# cd /sys/kernel/debug/tracing
# echo 1024 > buffer_size_kb
# echo 1 > events/dwc3/enable

then load g_zero and, on the host, run:

# testusb -t 9 -c 5000 -s 2048 -a

After it fails just read the trace file:

# cat trace

To me, it's failing after 5 iterations. It seems that DWC3 either
notifies transfer completion too early (before Transfer Resource is
actually freed up) or it's returning 1 on Set Endpoint Transfer Resource
by mistake.

cheers

ps: please send along result from trace :-) Mine's attached

--
balbi

Attachments:

(No filename) (0.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-07-01 09:03:09

[permalink] [raw]

Subject: RE: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi Felipe,

> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Tuesday, June 30, 2015 3:17 AM
> To: Subbaraya Sundeep Bhatta
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Subbaraya Sundeep
> Bhatta
> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command
> sent to DEPCMD register fails
>
> Hi,
>
> On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta
> wrote:
> > We need to return error to caller if command is not sent to controller
> > succesfully.
> >
> > Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> > Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> > Cc: <[email protected]>
> > ---
> > v2 changes:
> > Added Fixes and Cc in commit message.
>
> I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
> when used with g_zero and testusb. As of now, it could be that silicon is
> mis-behaving because I got a Transfer Complete before the failing Set
> Endpoint Transfer Resource command.
>
> In any case, can you run on your setup with g_zero and test.sh/testusb
> [1]/[2] just to verify that it really works for you ?

Ok I will do that.

Thanks,
Sundeep.B.S.

>
> Meanwhile, I'll continue testing on my end.
>
> cheers
>
> [1] https://gitorious.org/usb/usb-
> tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:testusb.c
> [2] https://gitorious.org/usb/usb-
> tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:test.sh
>
> --
> balbi

2015-07-02 02:03:25

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

2015-07-02 03:00:21

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

On Thu, Jul 02, 2015 at 02:03:14AM +0000, John Youn wrote:
> On 6/29/2015 2:48 PM, Felipe Balbi wrote:
> > Hi again,
> >
> > On Mon, Jun 29, 2015 at 04:47:01PM -0500, Felipe Balbi wrote:
> >> On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta wrote:
> >>> We need to return error to caller if command is not sent to
> >>> controller succesfully.
> >>>
> >>> Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> >>> Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> >>> Cc: <[email protected]>
> >>> ---
> >>> v2 changes:
> >>> Added Fixes and Cc in commit message.
> >>
> >> I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
> >> when used with g_zero and testusb. As of now, it could be that silicon
> >> is mis-behaving because I got a Transfer Complete before the failing Set
> >> Endpoint Transfer Resource command.
> >>
> >> In any case, can you run on your setup with g_zero and test.sh/testusb
> >> [1]/[2] just to verify that it really works for you ?
> >>
> >> Meanwhile, I'll continue testing on my end.
> >>
> >> cheers
> >>
> >> [1] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:testusb.c
> >> [2] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:test.sh
> >
> > Adding John here. John, any chance you could fire up dwc3 on HAPS and
> > see wether it works or fails for you ?
> >
> > cheers
> >
>
> Hi Felipe,
>
> Just an update on this.
>
> I'm trying to get this working with our latest IP with dwc3 from your
> testing/next branch. It fails the usbtest with a problem unrelated to
> this patch.
>
> It passes on 4.1.1.
>
> I'll have to look into the failure but I won't get to it until next
> week as I'm off the rest of this week.

interesting... If you could post failure signature, I can help looking
at it, but I guess it's too late to ask :-)

thanks for helping though

--
balbi

Attachments:

(No filename) (1.91 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-07-06 17:07:51

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

On Wed, Jul 01, 2015 at 07:29:28AM +0000, Subbaraya Sundeep Bhatta wrote:
> Hi Felipe,
>
> > -----Original Message-----
> > From: Felipe Balbi [mailto:[email protected]]
> > Sent: Tuesday, June 30, 2015 3:17 AM
> > To: Subbaraya Sundeep Bhatta
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; Subbaraya Sundeep
> > Bhatta
> > Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command
> > sent to DEPCMD register fails
> >
> > Hi,
> >
> > On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta
> > wrote:
> > > We need to return error to caller if command is not sent to controller
> > > succesfully.
> > >
> > > Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> > > Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> > > Cc: <[email protected]>
> > > ---
> > > v2 changes:
> > > Added Fixes and Cc in commit message.
> >
> > I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
> > when used with g_zero and testusb. As of now, it could be that silicon is
> > mis-behaving because I got a Transfer Complete before the failing Set
> > Endpoint Transfer Resource command.
> >
> > In any case, can you run on your setup with g_zero and test.sh/testusb
> > [1]/[2] just to verify that it really works for you ?
>
> Ok I will do that.

Did you manage to run the test I asked ? If we don't get down to this,
I'll have to revert your patch as it regresses my platforms.

--
balbi

Attachments:

(No filename) (1.50 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-07-07 02:10:36

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

On 7/1/2015 8:00 PM, Felipe Balbi wrote:
> On Thu, Jul 02, 2015 at 02:03:14AM +0000, John Youn wrote:
>> On 6/29/2015 2:48 PM, Felipe Balbi wrote:
>>> Hi again,
>>>
>>> On Mon, Jun 29, 2015 at 04:47:01PM -0500, Felipe Balbi wrote:
>>>> On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta wrote:
>>>>> We need to return error to caller if command is not sent to
>>>>> controller succesfully.
>>>>>
>>>>> Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
>>>>> Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
>>>>> Cc: <[email protected]>
>>>>> ---
>>>>> v2 changes:
>>>>> Added Fixes and Cc in commit message.
>>>>
>>>> I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
>>>> when used with g_zero and testusb. As of now, it could be that silicon
>>>> is mis-behaving because I got a Transfer Complete before the failing Set
>>>> Endpoint Transfer Resource command.
>>>>
>>>> In any case, can you run on your setup with g_zero and test.sh/testusb
>>>> [1]/[2] just to verify that it really works for you ?
>>>>
>>>> Meanwhile, I'll continue testing on my end.
>>>>
>>>> cheers
>>>>
>>>> [1] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:testusb.c
>>>> [2] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:test.sh
>>>
>>> Adding John here. John, any chance you could fire up dwc3 on HAPS and
>>> see wether it works or fails for you ?
>>>
>>> cheers
>>>
>>
>> Hi Felipe,
>>
>> Just an update on this.
>>
>> I'm trying to get this working with our latest IP with dwc3 from your
>> testing/next branch. It fails the usbtest with a problem unrelated to
>> this patch.
>>
>> It passes on 4.1.1.
>>
>> I'll have to look into the failure but I won't get to it until next
>> week as I'm off the rest of this week.
>
> interesting... If you could post failure signature, I can help looking
> at it, but I guess it's too late to ask :-)
>
> thanks for helping though
>

Hi Felipe,

Nevermind about my issue, it ended up being a setup-related
problem.

I actually do see the same error as you due to this series of
patches. Except I see it happening before even the first
iteration. I get a completion status of 1 for the Set Endpoint
Transfer Resources command. I'm not sure why this is.

I don't see any conflict with any previous Transfer Complete.

I will have to consult with some hardware engineers tomorrow to
look into it further.

The trace is attached.

John

Attachments:

dwc3-trace.txt (51.09 kB)
dwc3-trace.txt

2015-07-07 03:25:01

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi,

On Tue, Jul 07, 2015 at 02:10:26AM +0000, John Youn wrote:
> On 7/1/2015 8:00 PM, Felipe Balbi wrote:
> > On Thu, Jul 02, 2015 at 02:03:14AM +0000, John Youn wrote:
> >> On 6/29/2015 2:48 PM, Felipe Balbi wrote:
> >>> Hi again,
> >>>
> >>> On Mon, Jun 29, 2015 at 04:47:01PM -0500, Felipe Balbi wrote:
> >>>> On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep Bhatta wrote:
> >>>>> We need to return error to caller if command is not sent to
> >>>>> controller succesfully.
> >>>>>
> >>>>> Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> >>>>> Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> >>>>> Cc: <[email protected]>
> >>>>> ---
> >>>>> v2 changes:
> >>>>> Added Fixes and Cc in commit message.
> >>>>
> >>>> I noticed that this breaks at least my AM437x silicon with DWC3 2.40a
> >>>> when used with g_zero and testusb. As of now, it could be that silicon
> >>>> is mis-behaving because I got a Transfer Complete before the failing Set
> >>>> Endpoint Transfer Resource command.
> >>>>
> >>>> In any case, can you run on your setup with g_zero and test.sh/testusb
> >>>> [1]/[2] just to verify that it really works for you ?
> >>>>
> >>>> Meanwhile, I'll continue testing on my end.
> >>>>
> >>>> cheers
> >>>>
> >>>> [1] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:testusb.c
> >>>> [2] https://gitorious.org/usb/usb-tools/source/47ef073d9b6c0eae816204c81374aafb795c6e40:test.sh
> >>>
> >>> Adding John here. John, any chance you could fire up dwc3 on HAPS and
> >>> see wether it works or fails for you ?
> >>>
> >>> cheers
> >>>
> >>
> >> Hi Felipe,
> >>
> >> Just an update on this.
> >>
> >> I'm trying to get this working with our latest IP with dwc3 from your
> >> testing/next branch. It fails the usbtest with a problem unrelated to
> >> this patch.
> >>
> >> It passes on 4.1.1.
> >>
> >> I'll have to look into the failure but I won't get to it until next
> >> week as I'm off the rest of this week.
> >
> > interesting... If you could post failure signature, I can help looking
> > at it, but I guess it's too late to ask :-)
> >
> > thanks for helping though
> >
>
>
> Hi Felipe,
>
> Nevermind about my issue, it ended up being a setup-related
> problem.
>
> I actually do see the same error as you due to this series of
> patches. Except I see it happening before even the first
> iteration. I get a completion status of 1 for the Set Endpoint
> Transfer Resources command. I'm not sure why this is.
>
> I don't see any conflict with any previous Transfer Complete.
>
> I will have to consult with some hardware engineers tomorrow to
> look into it further.

cool, thanks. Just let me know if it ends up being something larger,
then we can revert that commit for the time being until we come to a
conclusion. Thanks a lot for helping with testing.

> The trace is attached.

thanks a lot :-) Do you mind letting me know which version are you
using? 3.00a ?

--
balbi

Attachments:

(No filename) (2.92 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-07-07 05:17:55

[permalink] [raw]

Subject: RE: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi Felipe,

> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Monday, July 06, 2015 10:38 PM
> To: Subbaraya Sundeep Bhatta
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command
> sent to DEPCMD register fails
>
> On Wed, Jul 01, 2015 at 07:29:28AM +0000, Subbaraya Sundeep Bhatta
> wrote:
> > Hi Felipe,
> >
> > > -----Original Message-----
> > > From: Felipe Balbi [mailto:[email protected]]
> > > Sent: Tuesday, June 30, 2015 3:17 AM
> > > To: Subbaraya Sundeep Bhatta
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; Subbaraya Sundeep Bhatta
> > > Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if
> > > command sent to DEPCMD register fails
> > >
> > > Hi,
> > >
> > > On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep
> Bhatta
> > > wrote:
> > > > We need to return error to caller if command is not sent to
> > > > controller succesfully.
> > > >
> > > > Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> > > > Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> > > > Cc: <[email protected]>
> > > > ---
> > > > v2 changes:
> > > > Added Fixes and Cc in commit message.
> > >
> > > I noticed that this breaks at least my AM437x silicon with DWC3
> > > 2.40a when used with g_zero and testusb. As of now, it could be that
> > > silicon is mis-behaving because I got a Transfer Complete before the
> > > failing Set Endpoint Transfer Resource command.
> > >
> > > In any case, can you run on your setup with g_zero and
> > > test.sh/testusb [1]/[2] just to verify that it really works for you ?
> >
> > Ok I will do that.
>
> Did you manage to run the test I asked ? If we don't get down to this, I'll
> have to revert your patch as it regresses my platforms.

Sorry I caught up with some other customer reported issue on Zynq (Chipidea). I will do it by this weekend at any cost. Please wait.

Thanks,
Sundeep.B.S.

>
> --
> balbi

2015-07-08 09:51:02

[permalink] [raw]

Subject: RE: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi Felipe,

> -----Original Message-----
> From: Subbaraya Sundeep Bhatta
> Sent: Tuesday, July 07, 2015 10:32 AM
> To: '[email protected]'
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: RE: [PATCH v2 3/3] usb: dwc3: gadget: return error if command
> sent to DEPCMD register fails
>
> Hi Felipe,
>
> > -----Original Message-----
> > From: Felipe Balbi [mailto:[email protected]]
> > Sent: Monday, July 06, 2015 10:38 PM
> > To: Subbaraya Sundeep Bhatta
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command
> > sent to DEPCMD register fails
> >
> > On Wed, Jul 01, 2015 at 07:29:28AM +0000, Subbaraya Sundeep Bhatta
> > wrote:
> > > Hi Felipe,
> > >
> > > > -----Original Message-----
> > > > From: Felipe Balbi [mailto:[email protected]]
> > > > Sent: Tuesday, June 30, 2015 3:17 AM
> > > > To: Subbaraya Sundeep Bhatta
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; Subbaraya Sundeep Bhatta
> > > > Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if
> > > > command sent to DEPCMD register fails
> > > >
> > > > Hi,
> > > >
> > > > On Thu, May 21, 2015 at 03:46:48PM +0530, Subbaraya Sundeep
> > Bhatta
> > > > wrote:
> > > > > We need to return error to caller if command is not sent to
> > > > > controller succesfully.
> > > > >
> > > > > Signed-off-by: Subbaraya Sundeep Bhatta <[email protected]>
> > > > > Fixes: 72246da40f37 (usb: Introduce DesignWare USB3 DRD Driver)
> > > > > Cc: <[email protected]>
> > > > > ---
> > > > > v2 changes:
> > > > > Added Fixes and Cc in commit message.
> > > >
> > > > I noticed that this breaks at least my AM437x silicon with DWC3
> > > > 2.40a when used with g_zero and testusb. As of now, it could be
> > > > that silicon is mis-behaving because I got a Transfer Complete
> > > > before the failing Set Endpoint Transfer Resource command.
> > > >
> > > > In any case, can you run on your setup with g_zero and
> > > > test.sh/testusb [1]/[2] just to verify that it really works for you ?

Can you please send test.sh and testusb.c in attachment. I guess am not able to access gitorious because of its migration.

Thanks,
Sundeep

> > >
> > > Ok I will do that.
> >
> > Did you manage to run the test I asked ? If we don't get down to this,
> > I'll have to revert your patch as it regresses my platforms.
>
> Sorry I caught up with some other customer reported issue on Zynq
> (Chipidea). I will do it by this weekend at any cost. Please wait.
>
> Thanks,
> Sundeep.B.S.
>
> >
> > --
> > balbi

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

2015-07-08 18:16:55

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi,

On Wed, Jul 08, 2015 at 09:50:47AM +0000, Subbaraya Sundeep Bhatta wrote:
> > > > > In any case, can you run on your setup with g_zero and
> > > > > test.sh/testusb [1]/[2] just to verify that it really works for you ?
>
> Can you please send test.sh and testusb.c in attachment. I guess am
> not able to access gitorious because of its migration.

heh, gitorious is closing down ? Wow, I'll move the repository to
github after vacations, meanwhile, both attached.

--
balbi

Attachments:

(No filename) (0.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-07-11 19:29:19

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi,

On Sat, Jul 11, 2015 at 05:17:32PM +0000, Subbaraya Sundeep Bhatta wrote:
> > > >> Hi Felipe,
> > > >>
> > > >> Just an update on this.
> > > >>
> > > >> I'm trying to get this working with our latest IP with dwc3 from
> > > >> your testing/next branch. It fails the usbtest with a problem
> > > >> unrelated to this patch.
> > > >>.
> > > >> It passes on 4.1.1.
> > > >>
> > > >> I'll have to look into the failure but I won't get to it until next
> > > >> week as I'm off the rest of this week.
> > > >
> > > > interesting... If you could post failure signature, I can help
> > > > looking at it, but I guess it's too late to ask :-)
> > > >
> > > > thanks for helping though
> > > >
> > >
> > >
> > > Hi Felipe,
> > >
> > > Nevermind about my issue, it ended up being a setup-related problem.
> > >
> > > I actually do see the same error as you due to this series of patches.
> > > Except I see it happening before even the first iteration. I get a
> > > completion status of 1 for the Set Endpoint Transfer Resources
> > > command. I'm not sure why this is.
> > >
> > > I don't see any conflict with any previous Transfer Complete.
>
> Same behavior at my end too. Fails before first iteration and I get
> completion status of 1 for Set Endpoint Resource command. Attached the
> logs of testing done with this patch and without this patch.
> Without this patch I often see completion status of 1 for Set Endpoint
> Transfer Resources command for Bulk and Isoc endpoints but test
> proceeds because driver just logs command completion status and moves
> on. We can revert this patch for time being. IP version is 2.90a.

yeah, that's what I mean, it really seems like it's the IP misbehaving.

John, let's try to figure out what's the root cause of this, we really
want to use command completion status at some point, but for now we need
to revert the patch :-(

Let me know if you want me to log STARS ticket on your solvnet system.

cheers

--
balbi

Attachments:

(No filename) (1.92 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-07-13 17:50:54

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

On 7/11/2015 12:29 PM, Felipe Balbi wrote:
> Hi,
>
> On Sat, Jul 11, 2015 at 05:17:32PM +0000, Subbaraya Sundeep Bhatta wrote:
>>>>>> Hi Felipe,
>>>>>>
>>>>>> Just an update on this.
>>>>>>
>>>>>> I'm trying to get this working with our latest IP with dwc3 from
>>>>>> your testing/next branch. It fails the usbtest with a problem
>>>>>> unrelated to this patch.
>>>>>> .
>>>>>> It passes on 4.1.1.
>>>>>>
>>>>>> I'll have to look into the failure but I won't get to it until next
>>>>>> week as I'm off the rest of this week.
>>>>>
>>>>> interesting... If you could post failure signature, I can help
>>>>> looking at it, but I guess it's too late to ask :-)
>>>>>
>>>>> thanks for helping though
>>>>>
>>>>
>>>>
>>>> Hi Felipe,
>>>>
>>>> Nevermind about my issue, it ended up being a setup-related problem.
>>>>
>>>> I actually do see the same error as you due to this series of patches.
>>>> Except I see it happening before even the first iteration. I get a
>>>> completion status of 1 for the Set Endpoint Transfer Resources
>>>> command. I'm not sure why this is.
>>>>
>>>> I don't see any conflict with any previous Transfer Complete.
>>
>> Same behavior at my end too. Fails before first iteration and I get
>> completion status of 1 for Set Endpoint Resource command. Attached the
>> logs of testing done with this patch and without this patch.
>> Without this patch I often see completion status of 1 for Set Endpoint
>> Transfer Resources command for Bulk and Isoc endpoints but test
>> proceeds because driver just logs command completion status and moves
>> on. We can revert this patch for time being. IP version is 2.90a.
>
> yeah, that's what I mean, it really seems like it's the IP misbehaving.
>
> John, let's try to figure out what's the root cause of this, we really
> want to use command completion status at some point, but for now we need
> to revert the patch :-(
>
> Let me know if you want me to log STARS ticket on your solvnet system.
>
> cheers
>

Hi Felipe,

We found the issue last week.

The start config command isn't getting called during SET_INTERFACE. Thus the transfer resource index isn't getting reset, and with multiple SET_INTERFACE commands it will eventually exhaust the resources.

I tried out a fix and it works for me. I'll send it out separately for review.

Also, I noticed that the trace message that shows control transfers doesn't show the SET_INTERFACE properly. Any idea why this is?

For example, here is the line in the trace that corresponds to the SET_INTERFACE:
irq/33-dwc3-10808 [003] d... 2443.494368: dwc3_ctrl_req: bRequestType 01 bRequest 0b wValue 0001 wIndex 0000 wLength 0

John

2015-07-13 18:59:05

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi,

On Mon, Jul 13, 2015 at 05:50:49PM +0000, John Youn wrote:
> On 7/11/2015 12:29 PM, Felipe Balbi wrote:
> > Hi,
> >
> > On Sat, Jul 11, 2015 at 05:17:32PM +0000, Subbaraya Sundeep Bhatta wrote:
> >>>>>> Hi Felipe,
> >>>>>>
> >>>>>> Just an update on this.
> >>>>>>
> >>>>>> I'm trying to get this working with our latest IP with dwc3 from
> >>>>>> your testing/next branch. It fails the usbtest with a problem
> >>>>>> unrelated to this patch.
> >>>>>> .
> >>>>>> It passes on 4.1.1.
> >>>>>>
> >>>>>> I'll have to look into the failure but I won't get to it until next
> >>>>>> week as I'm off the rest of this week.
> >>>>>
> >>>>> interesting... If you could post failure signature, I can help
> >>>>> looking at it, but I guess it's too late to ask :-)
> >>>>>
> >>>>> thanks for helping though
> >>>>>
> >>>>
> >>>>
> >>>> Hi Felipe,
> >>>>
> >>>> Nevermind about my issue, it ended up being a setup-related problem.
> >>>>
> >>>> I actually do see the same error as you due to this series of patches.
> >>>> Except I see it happening before even the first iteration. I get a
> >>>> completion status of 1 for the Set Endpoint Transfer Resources
> >>>> command. I'm not sure why this is.
> >>>>
> >>>> I don't see any conflict with any previous Transfer Complete.
> >>
> >> Same behavior at my end too. Fails before first iteration and I get
> >> completion status of 1 for Set Endpoint Resource command. Attached the
> >> logs of testing done with this patch and without this patch.
> >> Without this patch I often see completion status of 1 for Set Endpoint
> >> Transfer Resources command for Bulk and Isoc endpoints but test
> >> proceeds because driver just logs command completion status and moves
> >> on. We can revert this patch for time being. IP version is 2.90a.
> >
> > yeah, that's what I mean, it really seems like it's the IP misbehaving.
> >
> > John, let's try to figure out what's the root cause of this, we really
> > want to use command completion status at some point, but for now we need
> > to revert the patch :-(
> >
> > Let me know if you want me to log STARS ticket on your solvnet system.
> >
> > cheers
> >
>
> Hi Felipe,
>
> We found the issue last week.
>
> The start config command isn't getting called during SET_INTERFACE.
> Thus the transfer resource index isn't getting reset, and with
> multiple SET_INTERFACE commands it will eventually exhaust the
> resources.
>
> I tried out a fix and it works for me. I'll send it out separately for
> review.

Thanks a lot John. Not sure how come we missed that for such a long time
:-) Let's Cc stable and get it plugged ASAP :-)

> Also, I noticed that the trace message that shows control transfers
> doesn't show the SET_INTERFACE properly. Any idea why this is?
>
> For example, here is the line in the trace that corresponds to the
> SET_INTERFACE:
> irq/33-dwc3-10808 [003] d... 2443.494368: dwc3_ctrl_req: bRequestType 01 bRequest 0b wValue 0001 wIndex 0000 wLength 0

I'll have a look at this when I'm back in the office (Jul 18th).

--
balbi

Attachments:

(No filename) (2.98 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-07-15 10:04:11

[permalink] [raw]

Subject: RE: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi John,

> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Tuesday, July 14, 2015 12:29 AM
> To: John Youn
> Cc: [email protected]; Subbaraya Sundeep Bhatta; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command
> sent to DEPCMD register fails
>
> Hi,
>
> On Mon, Jul 13, 2015 at 05:50:49PM +0000, John Youn wrote:
> > On 7/11/2015 12:29 PM, Felipe Balbi wrote:
> > > Hi,
> > >
> > > On Sat, Jul 11, 2015 at 05:17:32PM +0000, Subbaraya Sundeep Bhatta
> wrote:
> > >>>>>> Hi Felipe,
> > >>>>>>
> > >>>>>> Just an update on this.
> > >>>>>>
> > >>>>>> I'm trying to get this working with our latest IP with dwc3
> > >>>>>> from your testing/next branch. It fails the usbtest with a
> > >>>>>> problem unrelated to this patch.
> > >>>>>> .
> > >>>>>> It passes on 4.1.1.
> > >>>>>>
> > >>>>>> I'll have to look into the failure but I won't get to it until
> > >>>>>> next week as I'm off the rest of this week.
> > >>>>>
> > >>>>> interesting... If you could post failure signature, I can help
> > >>>>> looking at it, but I guess it's too late to ask :-)
> > >>>>>
> > >>>>> thanks for helping though
> > >>>>>
> > >>>>
> > >>>>
> > >>>> Hi Felipe,
> > >>>>
> > >>>> Nevermind about my issue, it ended up being a setup-related
> problem.
> > >>>>
> > >>>> I actually do see the same error as you due to this series of patches.
> > >>>> Except I see it happening before even the first iteration. I get
> > >>>> a completion status of 1 for the Set Endpoint Transfer Resources
> > >>>> command. I'm not sure why this is.
> > >>>>
> > >>>> I don't see any conflict with any previous Transfer Complete.
> > >>
> > >> Same behavior at my end too. Fails before first iteration and I get
> > >> completion status of 1 for Set Endpoint Resource command. Attached
> > >> the logs of testing done with this patch and without this patch.
> > >> Without this patch I often see completion status of 1 for Set
> > >> Endpoint Transfer Resources command for Bulk and Isoc endpoints but
> > >> test proceeds because driver just logs command completion status
> > >> and moves on. We can revert this patch for time being. IP version is
> 2.90a.
> > >
> > > yeah, that's what I mean, it really seems like it's the IP misbehaving.
> > >
> > > John, let's try to figure out what's the root cause of this, we
> > > really want to use command completion status at some point, but for
> > > now we need to revert the patch :-(
> > >
> > > Let me know if you want me to log STARS ticket on your solvnet system.
> > >
> > > cheers
> > >
> >
> > Hi Felipe,
> >
> > We found the issue last week.
> >
> > The start config command isn't getting called during SET_INTERFACE.
> > Thus the transfer resource index isn't getting reset, and with
> > multiple SET_INTERFACE commands it will eventually exhaust the
> > resources.
> >
> > I tried out a fix and it works for me. I'll send it out separately for
> > review.

Thanks John for debugging :). Yes we are not handling SET_INTERFACE similar to
SET_CONFIGURATION in driver. I guess we follow
"Alternate Initialization on SetInterface Request" sequence as per data book.
Felipe can confirm this.

>
> Thanks a lot John. Not sure how come we missed that for such a long time
> :-) Let's Cc stable and get it plugged ASAP :-)
>
> > Also, I noticed that the trace message that shows control transfers
> > doesn't show the SET_INTERFACE properly. Any idea why this is?
> >
> > For example, here is the line in the trace that corresponds to the
> > SET_INTERFACE:
> > irq/33-dwc3-10808 [003] d... 2443.494368: dwc3_ctrl_req:
> bRequestType
> > 01 bRequest 0b wValue 0001 wIndex 0000 wLength 0

Can you please elaborate? What is expected here? Did you mean it shows wrong info
(other than the request actually sent by Host) ?

Thanks,
Sundeep.B.S.

>
> I'll have a look at this when I'm back in the office (Jul 18th).
>
> --
> balbi

2015-07-20 17:51:57

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

Hi,

On Wed, Jul 15, 2015 at 09:49:05AM +0000, Subbaraya Sundeep Bhatta wrote:
> Hi John,
>
> > -----Original Message-----
> > From: Felipe Balbi [mailto:[email protected]]
> > Sent: Tuesday, July 14, 2015 12:29 AM
> > To: John Youn
> > Cc: [email protected]; Subbaraya Sundeep Bhatta; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command
> > sent to DEPCMD register fails
> >
> > Hi,
> >
> > On Mon, Jul 13, 2015 at 05:50:49PM +0000, John Youn wrote:
> > > On 7/11/2015 12:29 PM, Felipe Balbi wrote:
> > > > Hi,
> > > >
> > > > On Sat, Jul 11, 2015 at 05:17:32PM +0000, Subbaraya Sundeep Bhatta
> > wrote:
> > > >>>>>> Hi Felipe,
> > > >>>>>>
> > > >>>>>> Just an update on this.
> > > >>>>>>
> > > >>>>>> I'm trying to get this working with our latest IP with dwc3
> > > >>>>>> from your testing/next branch. It fails the usbtest with a
> > > >>>>>> problem unrelated to this patch.
> > > >>>>>> .
> > > >>>>>> It passes on 4.1.1.
> > > >>>>>>
> > > >>>>>> I'll have to look into the failure but I won't get to it until
> > > >>>>>> next week as I'm off the rest of this week.
> > > >>>>>
> > > >>>>> interesting... If you could post failure signature, I can help
> > > >>>>> looking at it, but I guess it's too late to ask :-)
> > > >>>>>
> > > >>>>> thanks for helping though
> > > >>>>>
> > > >>>>
> > > >>>>
> > > >>>> Hi Felipe,
> > > >>>>
> > > >>>> Nevermind about my issue, it ended up being a setup-related
> > problem.
> > > >>>>
> > > >>>> I actually do see the same error as you due to this series of patches.
> > > >>>> Except I see it happening before even the first iteration. I get
> > > >>>> a completion status of 1 for the Set Endpoint Transfer Resources
> > > >>>> command. I'm not sure why this is.
> > > >>>>
> > > >>>> I don't see any conflict with any previous Transfer Complete.
> > > >>
> > > >> Same behavior at my end too. Fails before first iteration and I get
> > > >> completion status of 1 for Set Endpoint Resource command. Attached
> > > >> the logs of testing done with this patch and without this patch.
> > > >> Without this patch I often see completion status of 1 for Set
> > > >> Endpoint Transfer Resources command for Bulk and Isoc endpoints but
> > > >> test proceeds because driver just logs command completion status
> > > >> and moves on. We can revert this patch for time being. IP version is
> > 2.90a.
> > > >
> > > > yeah, that's what I mean, it really seems like it's the IP misbehaving.
> > > >
> > > > John, let's try to figure out what's the root cause of this, we
> > > > really want to use command completion status at some point, but for
> > > > now we need to revert the patch :-(
> > > >
> > > > Let me know if you want me to log STARS ticket on your solvnet system.
> > > >
> > > > cheers
> > > >
> > >
> > > Hi Felipe,
> > >
> > > We found the issue last week.
> > >
> > > The start config command isn't getting called during SET_INTERFACE.
> > > Thus the transfer resource index isn't getting reset, and with
> > > multiple SET_INTERFACE commands it will eventually exhaust the
> > > resources.
> > >
> > > I tried out a fix and it works for me. I'll send it out separately for
> > > review.
>
> Thanks John for debugging :). Yes we are not handling SET_INTERFACE similar to
> SET_CONFIGURATION in driver. I guess we follow
> "Alternate Initialization on SetInterface Request" sequence as per data book.
> Felipe can confirm this.
>
> >
> > Thanks a lot John. Not sure how come we missed that for such a long time
> > :-) Let's Cc stable and get it plugged ASAP :-)
> >
> > > Also, I noticed that the trace message that shows control transfers
> > > doesn't show the SET_INTERFACE properly. Any idea why this is?
> > >
> > > For example, here is the line in the trace that corresponds to the
> > > SET_INTERFACE:
> > > irq/33-dwc3-10808 [003] d... 2443.494368: dwc3_ctrl_req:
> > bRequestType
> > > 01 bRequest 0b wValue 0001 wIndex 0000 wLength 0
>
> Can you please elaborate? What is expected here? Did you mean it shows wrong info
> (other than the request actually sent by Host) ?

I have been looking more at this and the reason why we're failing is a
patch from Paul, which I quote below:

commit b23c843992b659d537514e6493d673284f5d6724
Author: Paul Zimmerman <[email protected]>
Date: Fri Sep 30 10:58:42 2011 +0300

usb: dwc3: gadget: fix DEPSTARTCFG for non-EP0 EPs

DEPSTARTCFG for non-EP0 EPs must only be sent once per config

[ [email protected] : changed config_start to start_config_issued ]

Signed-off-by: Paul Zimmerman <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 0231eba1f53d..502582ce1fc6 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -529,6 +529,7 @@ static inline void dwc3_trb_to_nat(struct dwc3_trb_hw *hw, struct dwc3_trb *nat)
* @ep0_status_pending: ep0 status response without a req is pending
* @ep0_bounced: true when we used bounce buffer
* @ep0_expect_in: true when we expect a DATA IN transfer
+ * @start_config_issued: true when StartConfig command has been issued
* @ep0_next_event: hold the next expected event
* @ep0state: state of endpoint zero
* @link_state: link state
@@ -576,6 +577,7 @@ struct dwc3 {
unsigned ep0_status_pending:1;
unsigned ep0_bounced:1;
unsigned ep0_expect_in:1;
+ unsigned start_config_issued:1;

enum dwc3_ep0_next ep0_next_event;
enum dwc3_ep0_state ep0state;
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index cc27c4ec498d..2def48ed30ea 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -455,6 +455,7 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
u32 cfg;
int ret;

+ dwc->start_config_issued = false;
cfg = le16_to_cpu(ctrl->wValue);

switch (dwc->dev_state) {
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index b2820aa9fa46..9c0174a8f46c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -237,8 +237,12 @@ static int dwc3_gadget_start_config(struct dwc3 *dwc, struct dwc3_ep *dep)
if (dep->number != 1) {
cmd = DWC3_DEPCMD_DEPSTARTCFG;
/* XferRscIdx == 0 for ep0 and 2 for the remaining */
- if (dep->number > 1)
+ if (dep->number > 1) {
+ if (dwc->start_config_issued)
+ return 0;
+ dwc->start_config_issued = true;
cmd |= DWC3_DEPCMD_PARAM(2);
+ }

return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, &params);
}
@@ -1161,6 +1165,8 @@ static int dwc3_gadget_start(struct usb_gadget *g,
reg |= DWC3_DCFG_SUPERSPEED;
dwc3_writel(dwc->regs, DWC3_DCFG, reg);

+ dwc->start_config_issued = false;
+
/* Start with SuperSpeed Default */
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);

@@ -1592,6 +1598,7 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)

dwc3_stop_active_transfers(dwc);
dwc3_disconnect_gadget(dwc);
+ dwc->start_config_issued = false;

dwc->gadget.speed = USB_SPEED_UNKNOWN;
}
@@ -1643,6 +1650,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)

dwc3_stop_active_transfers(dwc);
dwc3_clear_stall_all_ep(dwc);
+ dwc->start_config_issued = false;

/* Reset device address to zero */
reg = dwc3_readl(dwc->regs, DWC3_DCFG);

So the problem is that even though endpoint *is* indeed disabled,
start_config_issued is still true and we return before sending that
command. According to Paul's patch it's important that we send
start_config only once per config.

However, I do see that section 9.1.5, table 9-5 of Databook 2.90a has no
conditionals around DEPSTARTCFG. Reverting that patch from Paul, I can
see that it all works fine.

I'll send the revert still today, but I need you guys to confirm that it
really works on your platforms. According to Paul he "can't see how the
driver would work without" start_config_issued trick.

cheers

--
balbi

Attachments:

(No filename) (7.89 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-07-20 18:16:50

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

On 7/20/2015 10:51 AM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Jul 15, 2015 at 09:49:05AM +0000, Subbaraya Sundeep Bhatta wrote:
>> Hi John,
>>
>>> -----Original Message-----
>>> From: Felipe Balbi [mailto:[email protected]]
>>> Sent: Tuesday, July 14, 2015 12:29 AM
>>> To: John Youn
>>> Cc: [email protected]; Subbaraya Sundeep Bhatta; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]
>>> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command
>>> sent to DEPCMD register fails
>>>
>>> Hi,
>>>
>>> On Mon, Jul 13, 2015 at 05:50:49PM +0000, John Youn wrote:
>>>> On 7/11/2015 12:29 PM, Felipe Balbi wrote:
>>>>> Hi,
>>>>>
>>>>> On Sat, Jul 11, 2015 at 05:17:32PM +0000, Subbaraya Sundeep Bhatta
>>> wrote:
>>>>>>>>>> Hi Felipe,
>>>>>>>>>>
>>>>>>>>>> Just an update on this.
>>>>>>>>>>
>>>>>>>>>> I'm trying to get this working with our latest IP with dwc3
>>>>>>>>>> from your testing/next branch. It fails the usbtest with a
>>>>>>>>>> problem unrelated to this patch.
>>>>>>>>>> .
>>>>>>>>>> It passes on 4.1.1.
>>>>>>>>>>
>>>>>>>>>> I'll have to look into the failure but I won't get to it until
>>>>>>>>>> next week as I'm off the rest of this week.
>>>>>>>>>
>>>>>>>>> interesting... If you could post failure signature, I can help
>>>>>>>>> looking at it, but I guess it's too late to ask :-)
>>>>>>>>>
>>>>>>>>> thanks for helping though
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Felipe,
>>>>>>>>
>>>>>>>> Nevermind about my issue, it ended up being a setup-related
>>> problem.
>>>>>>>>
>>>>>>>> I actually do see the same error as you due to this series of patches.
>>>>>>>> Except I see it happening before even the first iteration. I get
>>>>>>>> a completion status of 1 for the Set Endpoint Transfer Resources
>>>>>>>> command. I'm not sure why this is.
>>>>>>>>
>>>>>>>> I don't see any conflict with any previous Transfer Complete.
>>>>>>
>>>>>> Same behavior at my end too. Fails before first iteration and I get
>>>>>> completion status of 1 for Set Endpoint Resource command. Attached
>>>>>> the logs of testing done with this patch and without this patch.
>>>>>> Without this patch I often see completion status of 1 for Set
>>>>>> Endpoint Transfer Resources command for Bulk and Isoc endpoints but
>>>>>> test proceeds because driver just logs command completion status
>>>>>> and moves on. We can revert this patch for time being. IP version is
>>> 2.90a.
>>>>>
>>>>> yeah, that's what I mean, it really seems like it's the IP misbehaving.
>>>>>
>>>>> John, let's try to figure out what's the root cause of this, we
>>>>> really want to use command completion status at some point, but for
>>>>> now we need to revert the patch :-(
>>>>>
>>>>> Let me know if you want me to log STARS ticket on your solvnet system.
>>>>>
>>>>> cheers
>>>>>
>>>>
>>>> Hi Felipe,
>>>>
>>>> We found the issue last week.
>>>>
>>>> The start config command isn't getting called during SET_INTERFACE.
>>>> Thus the transfer resource index isn't getting reset, and with
>>>> multiple SET_INTERFACE commands it will eventually exhaust the
>>>> resources.
>>>>
>>>> I tried out a fix and it works for me. I'll send it out separately for
>>>> review.
>>
>> Thanks John for debugging :). Yes we are not handling SET_INTERFACE similar to
>> SET_CONFIGURATION in driver. I guess we follow
>> "Alternate Initialization on SetInterface Request" sequence as per data book.
>> Felipe can confirm this.
>>
>>>
>>> Thanks a lot John. Not sure how come we missed that for such a long time
>>> :-) Let's Cc stable and get it plugged ASAP :-)
>>>
>>>> Also, I noticed that the trace message that shows control transfers
>>>> doesn't show the SET_INTERFACE properly. Any idea why this is?
>>>>
>>>> For example, here is the line in the trace that corresponds to the
>>>> SET_INTERFACE:
>>>> irq/33-dwc3-10808 [003] d... 2443.494368: dwc3_ctrl_req:
>>> bRequestType
>>>> 01 bRequest 0b wValue 0001 wIndex 0000 wLength 0
>>
>> Can you please elaborate? What is expected here? Did you mean it shows wrong info
>> (other than the request actually sent by Host) ?
>
> I have been looking more at this and the reason why we're failing is a
> patch from Paul, which I quote below:
>
> commit b23c843992b659d537514e6493d673284f5d6724
> Author: Paul Zimmerman <[email protected]>
> Date: Fri Sep 30 10:58:42 2011 +0300
>
> usb: dwc3: gadget: fix DEPSTARTCFG for non-EP0 EPs
>
> DEPSTARTCFG for non-EP0 EPs must only be sent once per config
>
> [ [email protected] : changed config_start to start_config_issued ]
>
> Signed-off-by: Paul Zimmerman <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 0231eba1f53d..502582ce1fc6 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -529,6 +529,7 @@ static inline void dwc3_trb_to_nat(struct dwc3_trb_hw *hw, struct dwc3_trb *nat)
> * @ep0_status_pending: ep0 status response without a req is pending
> * @ep0_bounced: true when we used bounce buffer
> * @ep0_expect_in: true when we expect a DATA IN transfer
> + * @start_config_issued: true when StartConfig command has been issued
> * @ep0_next_event: hold the next expected event
> * @ep0state: state of endpoint zero
> * @link_state: link state
> @@ -576,6 +577,7 @@ struct dwc3 {
> unsigned ep0_status_pending:1;
> unsigned ep0_bounced:1;
> unsigned ep0_expect_in:1;
> + unsigned start_config_issued:1;
>
> enum dwc3_ep0_next ep0_next_event;
> enum dwc3_ep0_state ep0state;
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index cc27c4ec498d..2def48ed30ea 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -455,6 +455,7 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
> u32 cfg;
> int ret;
>
> + dwc->start_config_issued = false;
> cfg = le16_to_cpu(ctrl->wValue);
>
> switch (dwc->dev_state) {
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index b2820aa9fa46..9c0174a8f46c 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -237,8 +237,12 @@ static int dwc3_gadget_start_config(struct dwc3 *dwc, struct dwc3_ep *dep)
> if (dep->number != 1) {
> cmd = DWC3_DEPCMD_DEPSTARTCFG;
> /* XferRscIdx == 0 for ep0 and 2 for the remaining */
> - if (dep->number > 1)
> + if (dep->number > 1) {
> + if (dwc->start_config_issued)
> + return 0;
> + dwc->start_config_issued = true;
> cmd |= DWC3_DEPCMD_PARAM(2);
> + }
>
> return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, &params);
> }
> @@ -1161,6 +1165,8 @@ static int dwc3_gadget_start(struct usb_gadget *g,
> reg |= DWC3_DCFG_SUPERSPEED;
> dwc3_writel(dwc->regs, DWC3_DCFG, reg);
>
> + dwc->start_config_issued = false;
> +
> /* Start with SuperSpeed Default */
> dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
>
> @@ -1592,6 +1598,7 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>
> dwc3_stop_active_transfers(dwc);
> dwc3_disconnect_gadget(dwc);
> + dwc->start_config_issued = false;
>
> dwc->gadget.speed = USB_SPEED_UNKNOWN;
> }
> @@ -1643,6 +1650,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>
> dwc3_stop_active_transfers(dwc);
> dwc3_clear_stall_all_ep(dwc);
> + dwc->start_config_issued = false;
>
> /* Reset device address to zero */
> reg = dwc3_readl(dwc->regs, DWC3_DCFG);
>
>
> So the problem is that even though endpoint *is* indeed disabled,
> start_config_issued is still true and we return before sending that
> command. According to Paul's patch it's important that we send
> start_config only once per config.
>
> However, I do see that section 9.1.5, table 9-5 of Databook 2.90a has no
> conditionals around DEPSTARTCFG. Reverting that patch from Paul, I can
> see that it all works fine.
>
> I'll send the revert still today, but I need you guys to confirm that it
> really works on your platforms. According to Paul he "can't see how the
> driver would work without" start_config_issued trick.
>
> cheers
>

Hi Felipe,

I think Paul's patch is still valid. However the
DEPSTARTCFG(XferRscIdx=2) should only be issued once per
SET_CONFIG *OR* SET_INTERFACE.

This command will reset the Transfer Resource Index to 2 (there
are already 2 used for EP0). Then subsequent DEPXFERCFG commands
will take the next XferRscIdx and increment. Without this, you
will assign the same transfer resource for different endpoints
which could potentially cause problems.

The only other time DEPSTARTCFG should be issued is a power on or
reset before you configure EP0. In that case you should do
DEPSTARTCFG(XferRscIdx=0).

This caused problems because, although it was reset on
SET_CONFIG, the usbtest did many SET_INTERFACE requests where it
wasn't being reset. Eventually it ran out of transfer resources
and errored out.

I'll send you a patch to review shortly. However, I have not been
able to test it yet beyond a few simple cases and usbtest.

John

2015-07-20 18:37:47