diff mbox

[U-Boot] usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC

Message ID 1403546568-30830-1-git-send-email-swarren@wwwdotorg.org
State Accepted
Delegated to: Marek Vasut
Headers show

Commit Message

Stephen Warren June 23, 2014, 6:02 p.m. UTC 
From: Stephen Warren <swarren@nvidia.com>

ci_udc.c's usb_gadget_unregister_driver() doesn't call driver->unbind()
unlike other USB gadget drivers. Fix it to do this.

Without this, when ether.c's CDC Ethernet device is torn down,
eth_unbind() is never called, so dev->gadget is never set to NULL.
For some reason, usb_eth_halt() is called both at the end of the first
use of the Ethernet device, and prior to any subsequent use. Since
dev->gadget is never cleared, all calls to usb_eth_halt() attempt to
stop, disconnect, and clean up the device, resulting in double cleanup,
which hangs U-Boot on my Tegra device at least.

ci_udc allocates its own singleton EP0 request object, and cleans it up
during usb_gadget_unregister_driver(). This appears necessary when using
the USB gadget framework in U-Boot, since that does not allocate/free
the EP0 request. However, the CDC Ethernet driver *does* allocate and
free its own EP0 requests. Consequently, we must protect
ci_ep_free_request() against double-freeing the request.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/ci_udc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Eric Nelson June 23, 2014, 7:03 p.m. UTC | #1 
Thanks Stephen,

On 06/23/2014 11:02 AM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> ci_udc.c's usb_gadget_unregister_driver() doesn't call driver->unbind()
> unlike other USB gadget drivers. Fix it to do this.
> 
> Without this, when ether.c's CDC Ethernet device is torn down,
> eth_unbind() is never called, so dev->gadget is never set to NULL.
> For some reason, usb_eth_halt() is called both at the end of the first
> use of the Ethernet device, and prior to any subsequent use. Since
> dev->gadget is never cleared, all calls to usb_eth_halt() attempt to
> stop, disconnect, and clean up the device, resulting in double cleanup,
> which hangs U-Boot on my Tegra device at least.
> 
> ci_udc allocates its own singleton EP0 request object, and cleans it up
> during usb_gadget_unregister_driver(). This appears necessary when using
> the USB gadget framework in U-Boot, since that does not allocate/free
> the EP0 request. However, the CDC Ethernet driver *does* allocate and
> free its own EP0 requests. Consequently, we must protect
> ci_ep_free_request() against double-freeing the request.
> 

Tested-by: Eric Nelson <eric.nelson@boundarydevices.com>

Tested on Nitrogen6x (i.MX6). On this platform, without the patch, I
could execute ping exactly once.

With the patch, repeated pings work great!

Regards,


Eric
Marek Vasut June 25, 2014, 1:51 p.m. UTC | #2 
On Monday, June 23, 2014 at 08:02:48 PM, Stephen Warren wrote:

+CC Jorg, rest of email is intact. Jorg, does this patch fix your issue?

> From: Stephen Warren <swarren@nvidia.com>
> 
> ci_udc.c's usb_gadget_unregister_driver() doesn't call driver->unbind()
> unlike other USB gadget drivers. Fix it to do this.
> 
> Without this, when ether.c's CDC Ethernet device is torn down,
> eth_unbind() is never called, so dev->gadget is never set to NULL.
> For some reason, usb_eth_halt() is called both at the end of the first
> use of the Ethernet device, and prior to any subsequent use. Since
> dev->gadget is never cleared, all calls to usb_eth_halt() attempt to
> stop, disconnect, and clean up the device, resulting in double cleanup,
> which hangs U-Boot on my Tegra device at least.
> 
> ci_udc allocates its own singleton EP0 request object, and cleans it up
> during usb_gadget_unregister_driver(). This appears necessary when using
> the USB gadget framework in U-Boot, since that does not allocate/free
> the EP0 request. However, the CDC Ethernet driver *does* allocate and
> free its own EP0 requests. Consequently, we must protect
> ci_ep_free_request() against double-freeing the request.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/usb/gadget/ci_udc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index b18bee43ad89..c3f6467b7db4 100644
> --- a/drivers/usb/gadget/ci_udc.c
> +++ b/drivers/usb/gadget/ci_udc.c
> @@ -226,8 +226,11 @@ static void ci_ep_free_request(struct usb_ep *ep,
> struct usb_request *req) int num;
> 
>  	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> -	if (num == 0)
> +	if (num == 0) {
> +		if (!controller.ep0_req)
> +			return;
>  		controller.ep0_req = 0;
> +	}
> 
>  	if (ci_req->b_buf)
>  		free(ci_req->b_buf);
> @@ -909,6 +912,9 @@ int usb_gadget_unregister_driver(struct
> usb_gadget_driver *driver) {
>  	udc_disconnect();
> 
> +	driver->unbind(&controller.gadget);
> +	controller.driver = NULL;
> +
>  	ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req);
>  	free(controller.items_mem);
>  	free(controller.epts);

Best regards,
Marek Vasut
Stephen Warren June 25, 2014, 4:06 p.m. UTC | #3 
On 06/25/2014 07:51 AM, Marek Vasut wrote:
> On Monday, June 23, 2014 at 08:02:48 PM, Stephen Warren wrote:
> 
> +CC Jorg, rest of email is intact. Jorg, does this patch fix your issue?

Jorg's issue was timeouts during transfers, whereas this patch addresses
cleanup issues once the device isn't being used any more. I can't
imagine how this patch could influence the issue Jorg is seeing.

>> From: Stephen Warren <swarren@nvidia.com>
>>
>> ci_udc.c's usb_gadget_unregister_driver() doesn't call driver->unbind()
>> unlike other USB gadget drivers. Fix it to do this.
>>
>> Without this, when ether.c's CDC Ethernet device is torn down,
>> eth_unbind() is never called, so dev->gadget is never set to NULL.
>> For some reason, usb_eth_halt() is called both at the end of the first
>> use of the Ethernet device, and prior to any subsequent use. Since
>> dev->gadget is never cleared, all calls to usb_eth_halt() attempt to
>> stop, disconnect, and clean up the device, resulting in double cleanup,
>> which hangs U-Boot on my Tegra device at least.
>>
>> ci_udc allocates its own singleton EP0 request object, and cleans it up
>> during usb_gadget_unregister_driver(). This appears necessary when using
>> the USB gadget framework in U-Boot, since that does not allocate/free
>> the EP0 request. However, the CDC Ethernet driver *does* allocate and
>> free its own EP0 requests. Consequently, we must protect
>> ci_ep_free_request() against double-freeing the request.
Marek Vasut June 25, 2014, 8:20 p.m. UTC | #4 
On Monday, June 23, 2014 at 08:02:48 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> ci_udc.c's usb_gadget_unregister_driver() doesn't call driver->unbind()
> unlike other USB gadget drivers. Fix it to do this.
> 
> Without this, when ether.c's CDC Ethernet device is torn down,
> eth_unbind() is never called, so dev->gadget is never set to NULL.
> For some reason, usb_eth_halt() is called both at the end of the first
> use of the Ethernet device, and prior to any subsequent use. Since
> dev->gadget is never cleared, all calls to usb_eth_halt() attempt to
> stop, disconnect, and clean up the device, resulting in double cleanup,
> which hangs U-Boot on my Tegra device at least.
> 
> ci_udc allocates its own singleton EP0 request object, and cleans it up
> during usb_gadget_unregister_driver(). This appears necessary when using
> the USB gadget framework in U-Boot, since that does not allocate/free
> the EP0 request. However, the CDC Ethernet driver *does* allocate and
> free its own EP0 requests. Consequently, we must protect
> ci_ep_free_request() against double-freeing the request.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied, thanks.

Best regards,
Marek Vasut
Jörg Krause June 27, 2014, 9:37 p.m. UTC | #5 
I added the last series of patches beginning from 2014-06-10 for testing 
purposes. The patches from 2014-05-29 were already applied.

First series of patches:

    Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
    Applying: usb: ci_udc: fix freeing of ep0 req
    Applying: usb: ci_udc: fix probe error cleanup
    Applying: usb: ci_udc: clean up all allocations in unregister

Calling tftp the first time after a reset runs fine, but calling it a 
second time ends always in a crash. I have to reset the device.

Next patch:

    Applying: usb: ci_udc: terminate ep0 INs with a zlp when required

Did not help.

Next patch:

    Applying: usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC

This helps! U-Boot does not crash anymore. But there is still a problem: 
I have to wait some seconds before I can run a second time tftp. This is 
the output from U-Boot:

    => run update_rootfs
    Updating rootfs ...
    using ci_udc, OUT ep- IN ep- STATUS ep-
    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
    USB network up!
    Using usb_ether device
    [snip]

    => run update_rootfs
    Updating rootfs ...
    using ci_udc, OUT ep- IN ep- STATUS ep-
    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
    ERROR: The remote end did not respond in time.
    at drivers/usb/gadget/ether.c:2388/usb_eth_init()

Wait some seconds ...

    => run update_rootfs
    Updating rootfs ...
    using ci_udc, OUT ep- IN ep- STATUS ep-
    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
    USB network up!
    Using usb_ether device
    [snip]

Best regards
Jörg Krause

On 06/25/2014 06:06 PM, Stephen Warren wrote:
> On 06/25/2014 07:51 AM, Marek Vasut wrote:
>> On Monday, June 23, 2014 at 08:02:48 PM, Stephen Warren wrote:
>>
>> +CC Jorg, rest of email is intact. Jorg, does this patch fix your issue?
> Jorg's issue was timeouts during transfers, whereas this patch addresses
> cleanup issues once the device isn't being used any more. I can't
> imagine how this patch could influence the issue Jorg is seeing.
>
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> ci_udc.c's usb_gadget_unregister_driver() doesn't call driver->unbind()
>>> unlike other USB gadget drivers. Fix it to do this.
>>>
>>> Without this, when ether.c's CDC Ethernet device is torn down,
>>> eth_unbind() is never called, so dev->gadget is never set to NULL.
>>> For some reason, usb_eth_halt() is called both at the end of the first
>>> use of the Ethernet device, and prior to any subsequent use. Since
>>> dev->gadget is never cleared, all calls to usb_eth_halt() attempt to
>>> stop, disconnect, and clean up the device, resulting in double cleanup,
>>> which hangs U-Boot on my Tegra device at least.
>>>
>>> ci_udc allocates its own singleton EP0 request object, and cleans it up
>>> during usb_gadget_unregister_driver(). This appears necessary when using
>>> the USB gadget framework in U-Boot, since that does not allocate/free
>>> the EP0 request. However, the CDC Ethernet driver *does* allocate and
>>> free its own EP0 requests. Consequently, we must protect
>>> ci_ep_free_request() against double-freeing the request.
Jörg Krause June 27, 2014, 9:52 p.m. UTC | #6 
I have tested a little bit more.

It does NOT help waiting some seconds before running tftp again. 
Sometimes it just works running tfpd immediately after a previous tfpd. 
Sometimes waiting even a minute ends up in the described error.

Odd, very odd, sorry :-)


On 06/27/2014 11:37 PM, Jörg Krause wrote:
> I added the last series of patches beginning from 2014-06-10 for 
> testing purposes. The patches from 2014-05-29 were already applied.
>
> First series of patches:
>
>    Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>    Applying: usb: ci_udc: fix freeing of ep0 req
>    Applying: usb: ci_udc: fix probe error cleanup
>    Applying: usb: ci_udc: clean up all allocations in unregister
>
> Calling tftp the first time after a reset runs fine, but calling it a 
> second time ends always in a crash. I have to reset the device.
>
> Next patch:
>
>    Applying: usb: ci_udc: terminate ep0 INs with a zlp when required
>
> Did not help.
>
> Next patch:
>
>    Applying: usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC
>
> This helps! U-Boot does not crash anymore. But there is still a 
> problem: I have to wait some seconds before I can run a second time 
> tftp. This is the output from U-Boot:
>
>    => run update_rootfs
>    Updating rootfs ...
>    using ci_udc, OUT ep- IN ep- STATUS ep-
>    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>    USB network up!
>    Using usb_ether device
>    [snip]
>
>    => run update_rootfs
>    Updating rootfs ...
>    using ci_udc, OUT ep- IN ep- STATUS ep-
>    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>    ERROR: The remote end did not respond in time.
>    at drivers/usb/gadget/ether.c:2388/usb_eth_init()
>
> Wait some seconds ...
>
>    => run update_rootfs
>    Updating rootfs ...
>    using ci_udc, OUT ep- IN ep- STATUS ep-
>    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>    USB network up!
>    Using usb_ether device
>    [snip]
>
> Best regards
> Jörg Krause
>
> On 06/25/2014 06:06 PM, Stephen Warren wrote:
>> On 06/25/2014 07:51 AM, Marek Vasut wrote:
>>> On Monday, June 23, 2014 at 08:02:48 PM, Stephen Warren wrote:
>>>
>>> +CC Jorg, rest of email is intact. Jorg, does this patch fix your 
>>> issue?
>> Jorg's issue was timeouts during transfers, whereas this patch addresses
>> cleanup issues once the device isn't being used any more. I can't
>> imagine how this patch could influence the issue Jorg is seeing.
>>
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> ci_udc.c's usb_gadget_unregister_driver() doesn't call 
>>>> driver->unbind()
>>>> unlike other USB gadget drivers. Fix it to do this.
>>>>
>>>> Without this, when ether.c's CDC Ethernet device is torn down,
>>>> eth_unbind() is never called, so dev->gadget is never set to NULL.
>>>> For some reason, usb_eth_halt() is called both at the end of the first
>>>> use of the Ethernet device, and prior to any subsequent use. Since
>>>> dev->gadget is never cleared, all calls to usb_eth_halt() attempt to
>>>> stop, disconnect, and clean up the device, resulting in double 
>>>> cleanup,
>>>> which hangs U-Boot on my Tegra device at least.
>>>>
>>>> ci_udc allocates its own singleton EP0 request object, and cleans 
>>>> it up
>>>> during usb_gadget_unregister_driver(). This appears necessary when 
>>>> using
>>>> the USB gadget framework in U-Boot, since that does not allocate/free
>>>> the EP0 request. However, the CDC Ethernet driver *does* allocate and
>>>> free its own EP0 requests. Consequently, we must protect
>>>> ci_ep_free_request() against double-freeing the request.
>
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Stephen Warren June 27, 2014, 9:55 p.m. UTC | #7 
On 06/27/2014 03:37 PM, Jörg Krause wrote:
> I added the last series of patches beginning from 2014-06-10 for testing
> purposes. The patches from 2014-05-29 were already applied.
> 
> First series of patches:
> 
>     Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>     Applying: usb: ci_udc: fix freeing of ep0 req
>     Applying: usb: ci_udc: fix probe error cleanup
>     Applying: usb: ci_udc: clean up all allocations in unregister
> 
> Calling tftp the first time after a reset runs fine,

I thought the issue you reported was that the *first* time you run the
"tftp" command, it has issues such as timeouts? Did I misunderstand, or
did that issue somehow go away?

> but calling it a
> second time ends always in a crash. I have to reset the device.

Yes, issues running "tftp" (or perhaps similar commands too) failing the
second time are expected with just those patches applied...

> Next patch:
> 
>     Applying: usb: ci_udc: terminate ep0 INs with a zlp when required
> 
> Did not help.
> 
> Next patch:
> 
>     Applying: usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC
> 
> This helps! U-Boot does not crash anymore.

Yes, that is certainly expected to solve issues with *multiple*
invocations of "tftp" or similar commands and ethernet over ci_udc. The
first hunk in that patch fixes something I probably introduced during
one of my ci_udc patches (probably the one that you originally replied
to "usb: ci_udc: allow multiple buffer allocs per ep"), but the second
hunk fixes a problem that I /think/ has always been there. Did multiple
invocations of "tftp" using ci_udc as the network device ever work for
you before, without random crashes or memory leaks?

> But there is still a problem:
> I have to wait some seconds before I can run a second time tftp. This is
> the output from U-Boot:
> 
>     => run update_rootfs
>     Updating rootfs ...
>     using ci_udc, OUT ep- IN ep- STATUS ep-
>     high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>     USB network up!
>     Using usb_ether device
>     [snip]
> 
>     => run update_rootfs
>     Updating rootfs ...
>     using ci_udc, OUT ep- IN ep- STATUS ep-
>     high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>     ERROR: The remote end did not respond in time.
>     at drivers/usb/gadget/ether.c:2388/usb_eth_init()
> 
> Wait some seconds ...
> 
>     => run update_rootfs
>     Updating rootfs ...
>     using ci_udc, OUT ep- IN ep- STATUS ep-
>     high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>     USB network up!
>     Using usb_ether device
>     [snip]

Hmm. That's odd. I didn't notice that, but I'll try retesting sometime.
What exactly does $update_rootfs contain? It might be useful to know
some details of your network topology (e.g. is the TFTP server on the
machine that the USB cable is plugged into or further away, and are the
machine and network lightly loaded) and rough sizes of the files you're
downloading.
Stephen Warren June 27, 2014, 9:56 p.m. UTC | #8 
On 06/27/2014 03:52 PM, Jörg Krause wrote:
> I have tested a little bit more.
> 
> It does NOT help waiting some seconds before running tftp again.
> Sometimes it just works running tfpd immediately after a previous tfpd.
> Sometimes waiting even a minute ends up in the described error.
> 
> Odd, very odd, sorry :-)

OK, that makes a little more sense. Intermittent issues are fine, but
intermittent issues affected by time delays are odd!

Does the *first* invocation of "tftp" after the board is powered on
always work perfectly?

> On 06/27/2014 11:37 PM, Jörg Krause wrote:
>> I added the last series of patches beginning from 2014-06-10 for
>> testing purposes. The patches from 2014-05-29 were already applied.
>>
>> First series of patches:
>>
>>    Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>    Applying: usb: ci_udc: fix freeing of ep0 req
>>    Applying: usb: ci_udc: fix probe error cleanup
>>    Applying: usb: ci_udc: clean up all allocations in unregister
>>
>> Calling tftp the first time after a reset runs fine, but calling it a
>> second time ends always in a crash. I have to reset the device.
>>
>> Next patch:
>>
>>    Applying: usb: ci_udc: terminate ep0 INs with a zlp when required
>>
>> Did not help.
>>
>> Next patch:
>>
>>    Applying: usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC
>>
>> This helps! U-Boot does not crash anymore. But there is still a
>> problem: I have to wait some seconds before I can run a second time
>> tftp. This is the output from U-Boot:
>>
>>    => run update_rootfs
>>    Updating rootfs ...
>>    using ci_udc, OUT ep- IN ep- STATUS ep-
>>    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>    USB network up!
>>    Using usb_ether device
>>    [snip]
>>
>>    => run update_rootfs
>>    Updating rootfs ...
>>    using ci_udc, OUT ep- IN ep- STATUS ep-
>>    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>    ERROR: The remote end did not respond in time.
>>    at drivers/usb/gadget/ether.c:2388/usb_eth_init()
>>
>> Wait some seconds ...
>>
>>    => run update_rootfs
>>    Updating rootfs ...
>>    using ci_udc, OUT ep- IN ep- STATUS ep-
>>    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>    USB network up!
>>    Using usb_ether device
>>    [snip]
>>
>> Best regards
>> Jörg Krause
>>
>> On 06/25/2014 06:06 PM, Stephen Warren wrote:
>>> On 06/25/2014 07:51 AM, Marek Vasut wrote:
>>>> On Monday, June 23, 2014 at 08:02:48 PM, Stephen Warren wrote:
>>>>
>>>> +CC Jorg, rest of email is intact. Jorg, does this patch fix your
>>>> issue?
>>> Jorg's issue was timeouts during transfers, whereas this patch addresses
>>> cleanup issues once the device isn't being used any more. I can't
>>> imagine how this patch could influence the issue Jorg is seeing.
>>>
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> ci_udc.c's usb_gadget_unregister_driver() doesn't call
>>>>> driver->unbind()
>>>>> unlike other USB gadget drivers. Fix it to do this.
>>>>>
>>>>> Without this, when ether.c's CDC Ethernet device is torn down,
>>>>> eth_unbind() is never called, so dev->gadget is never set to NULL.
>>>>> For some reason, usb_eth_halt() is called both at the end of the first
>>>>> use of the Ethernet device, and prior to any subsequent use. Since
>>>>> dev->gadget is never cleared, all calls to usb_eth_halt() attempt to
>>>>> stop, disconnect, and clean up the device, resulting in double
>>>>> cleanup,
>>>>> which hangs U-Boot on my Tegra device at least.
>>>>>
>>>>> ci_udc allocates its own singleton EP0 request object, and cleans
>>>>> it up
>>>>> during usb_gadget_unregister_driver(). This appears necessary when
>>>>> using
>>>>> the USB gadget framework in U-Boot, since that does not allocate/free
>>>>> the EP0 request. However, the CDC Ethernet driver *does* allocate and
>>>>> free its own EP0 requests. Consequently, we must protect
>>>>> ci_ep_free_request() against double-freeing the request.
>>
>>
>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
Jörg Krause June 27, 2014, 11:16 p.m. UTC | #9 
On 06/27/2014 11:55 PM, Stephen Warren wrote:
> On 06/27/2014 03:37 PM, Jörg Krause wrote:
>> I added the last series of patches beginning from 2014-06-10 for testing
>> purposes. The patches from 2014-05-29 were already applied.
>>
>> First series of patches:
>>
>>      Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>      Applying: usb: ci_udc: fix freeing of ep0 req
>>      Applying: usb: ci_udc: fix probe error cleanup
>>      Applying: usb: ci_udc: clean up all allocations in unregister
>>
>> Calling tftp the first time after a reset runs fine,
> I thought the issue you reported was that the *first* time you run the
> "tftp" command, it has issues such as timeouts? Did I misunderstand, or
> did that issue somehow go away?
That's right! This was the state before applying a series of patches 
after allow multiple buffer allocs per ep. Now, the first run of tftp 
runs without any errors.
> [snip]
> Yes, that is certainly expected to solve issues with *multiple*
> invocations of "tftp" or similar commands and ethernet over ci_udc. The
> first hunk in that patch fixes something I probably introduced during
> one of my ci_udc patches (probably the one that you originally replied
> to "usb: ci_udc: allow multiple buffer allocs per ep"), but the second
> hunk fixes a problem that I /think/ has always been there. Did multiple
> invocations of "tftp" using ci_udc as the network device ever work for
> you before, without random crashes or memory leaks?
Yes, it did. If I can remember right it stopped working after a series 
of patches from 2014-04-24. But I am not sure.
>> But there is still a problem:
>> I have to wait some seconds before I can run a second time tftp. This is
>> the output from U-Boot:
>>
>>      => run update_rootfs
>>      Updating rootfs ...
>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>      USB network up!
>>      Using usb_ether device
>>      [snip]
>>
>>      => run update_rootfs
>>      Updating rootfs ...
>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>      ERROR: The remote end did not respond in time.
>>      at drivers/usb/gadget/ether.c:2388/usb_eth_init()
>>
>> Wait some seconds ...
>>
>>      => run update_rootfs
>>      Updating rootfs ...
>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>      USB network up!
>>      Using usb_ether device
>>      [snip]
> Hmm. That's odd. I didn't notice that, but I'll try retesting sometime.
> What exactly does $update_rootfs contain? It might be useful to know
> some details of your network topology (e.g. is the TFTP server on the
> machine that the USB cable is plugged into or further away, and are the
> machine and network lightly loaded) and rough sizes of the files you're
> downloading.
This is what update_rootfs is doing:

       "update_rootfs=echo Updating rootfs ...; " \
         "if tftp ${rootfs_file}; then " \
           "mtdparts default; " \
           "nand erase.part rootfs; " \
           "ubi part rootfs; " \
           "ubi create rootfs; " \
           "ubi write ${loadaddr} rootfs ${filesize}; " \
         "fi; " \
         "\0" \

Filesize of rootfs.ubifs is about 13 MB.

The tftp server (tftp-hpa 5.2-4) is running on my notebook (running Arch 
Linux), where the device is plugged via USB cable. Ethernet is not used, 
but wireless network, which is used "normal" I would say.
Jörg Krause June 27, 2014, 11:27 p.m. UTC | #10 
On 06/27/2014 11:56 PM, Stephen Warren wrote:
> On 06/27/2014 03:52 PM, Jörg Krause wrote:
>> I have tested a little bit more.
>>
>> It does NOT help waiting some seconds before running tftp again.
>> Sometimes it just works running tfpd immediately after a previous tfpd.
>> Sometimes waiting even a minute ends up in the described error.
>>
>> Odd, very odd, sorry :-)
> OK, that makes a little more sense. Intermittent issues are fine, but
> intermittent issues affected by time delays are odd!
>
> Does the *first* invocation of "tftp" after the board is powered on
> always work perfectly?
Yes, always! The next runs of tftp sometimes works perfectly, sometimes 
I have timeouts, but it runs to end, and sometimes it did not even 
start, throwing the described error. But there are no crashes while 
running tfpd anymore :-)

FYI: I applied a series of patches from Heiko Schocher: [PATCH v5 0/5] 
mtd, ubi, ubifs: resync with Linux-3.14 on top of the patches for 
ci_udc. Now the USB Ethernet driver is completely broken. It does not 
even run a first time.
>> On 06/27/2014 11:37 PM, Jörg Krause wrote:
>> [snip]
Stephen Warren June 27, 2014, 11:37 p.m. UTC | #11 
On 06/27/2014 05:16 PM, Jörg Krause wrote:
> 
> On 06/27/2014 11:55 PM, Stephen Warren wrote:
>> On 06/27/2014 03:37 PM, Jörg Krause wrote:
>>> I added the last series of patches beginning from 2014-06-10 for testing
>>> purposes. The patches from 2014-05-29 were already applied.
>>>
>>> First series of patches:
>>>
>>>     Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>>     Applying: usb: ci_udc: fix freeing of ep0 req
>>>     Applying: usb: ci_udc: fix probe error cleanup
>>>     Applying: usb: ci_udc: clean up all allocations in unregister
>>>
>>> Calling tftp the first time after a reset runs fine,
>>
>> I thought the issue you reported was that the *first* time you run the
>> "tftp" command, it has issues such as timeouts? Did I misunderstand, or
>> did that issue somehow go away?
>
> That's right! This was the state before applying a series of patches
> after allow multiple buffer allocs per ep. Now, the first run of tftp
> runs without any errors.

Just to make sure I understand, here's what you saw:

1) tftp works fine to start with. No timeouts even on repeated invocation.

2) You applied "allow multiple buffer allocs per ep"

3) Now, you see tftp timeouts.

4) You applied "a series of patches *after* allow multiple buffer allocs
per ep"

5) Now, the first tftp command works fine, but repeated invocations fail
(intermittently).

And in (4) above the patch you applied that solved the problem was
"Applying: usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC"?

>>> But there is still a problem:
>>> I have to wait some seconds before I can run a second time tftp. This is
>>> the output from U-Boot:
>>>
>>>     => run update_rootfs
>>>     Updating rootfs ...
>>>     using ci_udc, OUT ep- IN ep- STATUS ep-
>>>     high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>     USB network up!
>>>     Using usb_ether device
>>>     [snip]
>>>
>>>     => run update_rootfs
>>>     Updating rootfs ...
>>>     using ci_udc, OUT ep- IN ep- STATUS ep-
>>>     high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>     ERROR: The remote end did not respond in time.
>>>     at drivers/usb/gadget/ether.c:2388/usb_eth_init()
>>>
>>> Wait some seconds ...
>>>
>>>     => run update_rootfs
>>>     Updating rootfs ...
>>>     using ci_udc, OUT ep- IN ep- STATUS ep-
>>>     high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>     USB network up!
>>>     Using usb_ether device
>>>     [snip]
>>
>> Hmm. That's odd. I didn't notice that, but I'll try retesting sometime.
>> What exactly does $update_rootfs contain? It might be useful to know
>> some details of your network topology (e.g. is the TFTP server on the
>> machine that the USB cable is plugged into or further away, and are the
>> machine and network lightly loaded) and rough sizes of the files you're
>> downloading.
>
> This is what update_rootfs is doing:
> 
>       "update_rootfs=echo Updating rootfs ...; " \
>         "if tftp ${rootfs_file}; then " \
>           "mtdparts default; " \
>           "nand erase.part rootfs; " \
>           "ubi part rootfs; " \
>           "ubi create rootfs; " \
>           "ubi write ${loadaddr} rootfs ${filesize}; " \

I wonder if there's some kind of memory corruption caused by the
mtdparts, nand, or ubi commands? I'm especially curious about this,
since your other email mentioned that some mtd/ubi patches cause
complete networking failures.

If you *just* run "tftp ${rootfs_file}" over and over, does that work?
If so, perhaps try running more and more of the commands from
$update_rootfs above until you find the one that causes problems.

>         "fi; " \
>         "\0" \
> 
> Filesize of rootfs.ubifs is about 13 MB.
> 
> The tftp server (tftp-hpa 5.2-4) is running on my notebook (running Arch
> Linux), where the device is plugged via USB cable. Ethernet is not used,
> but wireless network, which is used "normal" I would say.

OK, that's basically the same setup I used for testing, network/USB-wise.

Unfortunately, I don't have and NAND or ubifs to test with.
Jörg Krause June 28, 2014, 12:09 a.m. UTC | #12 
On 06/28/2014 01:37 AM, Stephen Warren wrote:
> On 06/27/2014 05:16 PM, Jörg Krause wrote:
>> On 06/27/2014 11:55 PM, Stephen Warren wrote:
>>> On 06/27/2014 03:37 PM, Jörg Krause wrote:
>>>> I added the last series of patches beginning from 2014-06-10 for testing
>>>> purposes. The patches from 2014-05-29 were already applied.
>>>>
>>>> First series of patches:
>>>>
>>>>      Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>>>      Applying: usb: ci_udc: fix freeing of ep0 req
>>>>      Applying: usb: ci_udc: fix probe error cleanup
>>>>      Applying: usb: ci_udc: clean up all allocations in unregister
>>>>
>>>> Calling tftp the first time after a reset runs fine,
>>> I thought the issue you reported was that the *first* time you run the
>>> "tftp" command, it has issues such as timeouts? Did I misunderstand, or
>>> did that issue somehow go away?
>> That's right! This was the state before applying a series of patches
>> after allow multiple buffer allocs per ep. Now, the first run of tftp
>> runs without any errors.
> Just to make sure I understand, here's what you saw:
>
> 1) tftp works fine to start with. No timeouts even on repeated invocation.
True.
>
> 2) You applied "allow multiple buffer allocs per ep"
I did a pull from the u-boot-imx branch. I am not sure which date it 
stop working.
>
> 3) Now, you see tftp timeouts.
At the beginning I had random timeouts even running update_rootfs the 
first time after a reset.
>
> 4) You applied "a series of patches *after* allow multiple buffer allocs
> per ep"
Yes. I applied these patches:

[U-Boot,1/4] usb: ci_udc: detect queued requests on ep0
[U-Boot,2/4] usb: ci_udc: use a single descriptor for ep0
[U-Boot,3/4] usb: ci_udc: pre-allocate ep0 req
[U-Boot,4/4] usb: ci_udc: complete ep0 direction handling

But the error still existed. I found out that setting #define CONFIG_SYS_CACHELINE_SIZE 32 in my config file helped. This is a quotation from my mail from 06/12/2014:

> I checked this and I found that ARCH_DMA_MINALIGN is set to 64, which 
> is not true for Freescale i.MX28. This processor has an ARM926EJ-S, 
> which has an cache line size of 32. In arch/arm/include/asm/cache.h 
> the macro ARCH_DMA_MINALIGN is defined as followed: #ifdef 
> CONFIG_SYS_CACHELINE_SIZE #define ARCH_DMA_MINALIGN    
> CONFIG_SYS_CACHELINE_SIZE #else #define ARCH_DMA_MINALIGN    64 #endif 
> And in /arch/arm/cpu/arm926ejs/cache.c as followed: #ifndef 
> CONFIG_SYS_CACHELINE_SIZE #define CONFIG_SYS_CACHELINE_SIZE    32 
> #endif arch/arm/include/asm/cache.h does not see the definition of 
> CONFIG_SYS_CACHELINE_SIZE in /arch/arm/cpu/arm926ejs/cache.c, so it's 
> a bad place to put it in there.


>
> 5) Now, the first tftp command works fine, but repeated invocations fail
> (intermittently).
Yes. The first tftp command almost always works fine. Sometimes I have a 
timeout in between, but it runs to the end. But the timeouts are really 
rare.
>
> And in (4) above the patch you applied that solved the problem was
> "Applying: usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC"?
This is also a quotation from my previous mail:

> I defined CONFIG_SYS_CACHELINE_SIZE to 32 in my config header file and 
> it works under the following circumstances: I have to disable the 
> macro CONFIG_USB_GADGET_DUALSPEED. But, and this is strange, it works 
> only the first time of a tftp download after a reset of the board. If 
> I try to use tftp a second time, I get the same timeout error as before.
>
> So, in short:
>
>     => reset
>     => run update_rootfs
>     [...]
>     done
>     => reset
>     => run update_rootfs
>     [...]
>     done
>
> works and
>
>     => reset
>     => run update_rootfs
>     [...]
>     done
>     => run update_rootfs
>     [...]
>     timeout sending packets to usb ethernet
>
> results in a timeout. Strange.
>
> Lastly, I changed CONFIG_SYS_CACHELINE_SIZE to 16 and this works for 
> me in normal mode an in dual speed mode.

So it worked, but there was already the error with running ftpd a second 
time. I am not so sure about the setting of the 
CONFIG_SYS_CACHELINE_SIZE to 16, because I did not used it anymore after 
some test runs.

>>>> But there is still a problem:
>>>> I have to wait some seconds before I can run a second time tftp. This is
>>>> the output from U-Boot:
>>>>
>>>>      => run update_rootfs
>>>>      Updating rootfs ...
>>>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>>      USB network up!
>>>>      Using usb_ether device
>>>>      [snip]
>>>>
>>>>      => run update_rootfs
>>>>      Updating rootfs ...
>>>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>>      ERROR: The remote end did not respond in time.
>>>>      at drivers/usb/gadget/ether.c:2388/usb_eth_init()
>>>>
>>>> Wait some seconds ...
>>>>
>>>>      => run update_rootfs
>>>>      Updating rootfs ...
>>>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>>      USB network up!
>>>>      Using usb_ether device
>>>>      [snip]
>>> Hmm. That's odd. I didn't notice that, but I'll try retesting sometime.
>>> What exactly does $update_rootfs contain? It might be useful to know
>>> some details of your network topology (e.g. is the TFTP server on the
>>> machine that the USB cable is plugged into or further away, and are the
>>> machine and network lightly loaded) and rough sizes of the files you're
>>> downloading.
>> This is what update_rootfs is doing:
>>
>>        "update_rootfs=echo Updating rootfs ...; " \
>>          "if tftp ${rootfs_file}; then " \
>>            "mtdparts default; " \
>>            "nand erase.part rootfs; " \
>>            "ubi part rootfs; " \
>>            "ubi create rootfs; " \
>>            "ubi write ${loadaddr} rootfs ${filesize}; " \
> I wonder if there's some kind of memory corruption caused by the
> mtdparts, nand, or ubi commands? I'm especially curious about this,
> since your other email mentioned that some mtd/ubi patches cause
> complete networking failures.
>
> If you *just* run "tftp ${rootfs_file}" over and over, does that work?
> If so, perhaps try running more and more of the commands from
> $update_rootfs above until you find the one that causes problems.

I will check this.
>
>>          "fi; " \
>>          "\0" \
>>
>> Filesize of rootfs.ubifs is about 13 MB.
>>
>> The tftp server (tftp-hpa 5.2-4) is running on my notebook (running Arch
>> Linux), where the device is plugged via USB cable. Ethernet is not used,
>> but wireless network, which is used "normal" I would say.
> OK, that's basically the same setup I used for testing, network/USB-wise.
>
> Unfortunately, I don't have and NAND or ubifs to test with.
Jörg Krause June 28, 2014, 1:34 a.m. UTC | #13 
On 06/28/2014 01:37 AM, Stephen Warren wrote:
> On 06/27/2014 05:16 PM, Jörg Krause wrote:
>> On 06/27/2014 11:55 PM, Stephen Warren wrote:
>>> On 06/27/2014 03:37 PM, Jörg Krause wrote:
>>>> I added the last series of patches beginning from 2014-06-10 for testing
>>>> purposes. The patches from 2014-05-29 were already applied.
>>>>
>>>> First series of patches:
>>>>
>>>>      Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>>>      Applying: usb: ci_udc: fix freeing of ep0 req
>>>>      Applying: usb: ci_udc: fix probe error cleanup
>>>>      Applying: usb: ci_udc: clean up all allocations in unregister
>>>>
>>>> Calling tftp the first time after a reset runs fine,
>>> I thought the issue you reported was that the *first* time you run the
>>> "tftp" command, it has issues such as timeouts? Did I misunderstand, or
>>> did that issue somehow go away?
>> That's right! This was the state before applying a series of patches
>> after allow multiple buffer allocs per ep. Now, the first run of tftp
>> runs without any errors.
> Just to make sure I understand, here's what you saw:
>
> 1) tftp works fine to start with. No timeouts even on repeated invocation.
I went back in time and now I can be more precise. Everything worked 
fine until commit usb: ci_udc: allow multiple buffer allocs per ep which 
introduces timeouts in almost all tftp file downloads. Even the first 
run of tfpt can end in a timeout.

>
> 2) You applied "allow multiple buffer allocs per ep"
Setting #define CONFIG_SYS_CACHELINE_SIZE 32 to my config file helped 
here. But still timeouts. First run almost always runs fine, only 
sometimes timeouts while receiving a packet, but always running to the 
end. Running tftp after this a second time and more fails with a ERROR: 
The remote end did not respond in time. at 
drivers/usb/gadget/ether.c:2388/usb_eth_init(), but sometimes it works.

Setting CONFIG_SYS_CACHELINE_SIZE 32 does not make it better (as I 
previously wrote it).

Removing CONFIG_USB_GADGET_DUALSPEED helps a little bit, but I am 
getting also errors after the second or third run.
>
> 3) Now, you see tftp timeouts.
>
> 4) You applied "a series of patches *after* allow multiple buffer allocs
> per ep"
Applying: usb: ci_udc: parse QTD before over-writing it
Applying: usb: ci_udc: detect queued requests on ep0
Applying: usb: ci_udc: use a single descriptor for ep0
Applying: usb: ci_udc: pre-allocate ep0 req
Applying: usb: ci_udc: complete ep0 direction handling
Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
Applying: usb: ci_udc: fix freeing of ep0 req
Applying: usb: ci_udc: fix probe error cleanup
Applying: usb: ci_udc: clean up all allocations in unregister
Applying: usb: ci_udc: terminate ep0 INs with a zlp when required
Applying: usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC
Applying: usb: ci_udc: fix typo in debug message
>
> 5) Now, the first tftp command works fine, but repeated invocations fail
> (intermittently).
Now tftp runs fine the first time (and sometimes more often) after a 
reboot, but after some tftp runs I am getting the ERROR: The remote end 
did not respond in time. at 
drivers/usb/gadget/ether.c:2388/usb_eth_init(), as mentioned above.
>
> And in (4) above the patch you applied that solved the problem was
> "Applying: usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC"?
>
>>>> But there is still a problem:
>>>> I have to wait some seconds before I can run a second time tftp. This is
>>>> the output from U-Boot:
>>>>
>>>>      => run update_rootfs
>>>>      Updating rootfs ...
>>>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>>      USB network up!
>>>>      Using usb_ether device
>>>>      [snip]
>>>>
>>>>      => run update_rootfs
>>>>      Updating rootfs ...
>>>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>>      ERROR: The remote end did not respond in time.
>>>>      at drivers/usb/gadget/ether.c:2388/usb_eth_init()
>>>>
>>>> Wait some seconds ...
>>>>
>>>>      => run update_rootfs
>>>>      Updating rootfs ...
>>>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>>>      USB network up!
>>>>      Using usb_ether device
>>>>      [snip]
>>> Hmm. That's odd. I didn't notice that, but I'll try retesting sometime.
>>> What exactly does $update_rootfs contain? It might be useful to know
>>> some details of your network topology (e.g. is the TFTP server on the
>>> machine that the USB cable is plugged into or further away, and are the
>>> machine and network lightly loaded) and rough sizes of the files you're
>>> downloading.
>> This is what update_rootfs is doing:
>>
>>        "update_rootfs=echo Updating rootfs ...; " \
>>          "if tftp ${rootfs_file}; then " \
>>            "mtdparts default; " \
>>            "nand erase.part rootfs; " \
>>            "ubi part rootfs; " \
>>            "ubi create rootfs; " \
>>            "ubi write ${loadaddr} rootfs ${filesize}; " \
> I wonder if there's some kind of memory corruption caused by the
> mtdparts, nand, or ubi commands? I'm especially curious about this,
> since your other email mentioned that some mtd/ubi patches cause
> complete networking failures.
>
> If you *just* run "tftp ${rootfs_file}" over and over, does that work?
> If so, perhaps try running more and more of the commands from
> $update_rootfs above until you find the one that causes problems.
For all tests I only run tftp rootfs.ubifs. The mtd patches from Heiko 
Schocher make some changes to the gadget driver. Maybe that's the cause, 
that it fails together.
>>          "fi; " \
>>          "\0" \
>>
>> Filesize of rootfs.ubifs is about 13 MB.
>>
>> The tftp server (tftp-hpa 5.2-4) is running on my notebook (running Arch
>> Linux), where the device is plugged via USB cable. Ethernet is not used,
>> but wireless network, which is used "normal" I would say.
> OK, that's basically the same setup I used for testing, network/USB-wise.
>
> Unfortunately, I don't have and NAND or ubifs to test with.
Jörg Krause June 28, 2014, 8:37 p.m. UTC | #14 
On 06/28/2014 09:59 PM, Marek Vasut wrote:

> I would actually be really glad if you could test plain u-boot-usb/master and
> see if your USB problem is present there. Also, it would be really nice to
> isolate the sequence of commands which triggers the bug, so we have a proper
> reproducible testcase.
>
> Anyway, this thread is not a place where I would like to discuss this. This
> should be discussed in the "original" thread, that is:
>
> Subj: [PATCH] usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC

I cloned u-boot-usb/master and tested it. I am running "tftpd 
rootfs.ubifs" several times. Here is the log (I shortened the loading 
process bar):

    HTLLCLLC

    U-Boot 2014.07-rc3-g18e0313 (Jun 28 2014 - 22:17:52)

    CPU:   Freescale i.MX28 rev1.2 at 454 MHz
    BOOT:  NAND, 3V3
    DRAM:  64 MiB
    NAND:  128 MiB
    In:    serial
    Out:   serial
    Err:   serial
    Net:   usb_ether [PRIME]
    Hit any key to stop autoboot:  0
    => tftp rootfs.ubifs
    using ci_udc, OUT ep- IN ep- STATUS ep-
    MAC 00:19:b8:00:00:02
    HOST MAC 00:19:b8:00:00:01
    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
    USB network up!
    Using usb_ether device
    TFTP from server 10.0.0.1; our IP address is 10.0.0.2
    Filename 'rootfs.ubifs'.
    Load address: 0x40008000
    Loading: [snip]
      ##############################################################T ###
          [snip]
          1.7 MiB/s
    done
    Bytes transferred = 13205504 (c98000 hex)
    => tftp rootfs.ubifs
    using ci_udc, OUT ep- IN ep- STATUS ep-
    MAC 00:19:b8:00:00:02
    HOST MAC 00:19:b8:00:00:01
    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
    USB network up!
    Using usb_ether device
    TFTP from server 10.0.0.1; our IP address is 10.0.0.2
    Filename 'rootfs.ubifs'.
    Load address: 0x40008000
    Loading:
    #################################################################
          [snip]
          5.6 MiB/s
    done
    Bytes transferred = 13205504 (c98000 hex)
    => tftp rootfs.ubifs
    using ci_udc, OUT ep- IN ep- STATUS ep-
    MAC 00:19:b8:00:00:02
    HOST MAC 00:19:b8:00:00:01
    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
    USB network up!
    Using usb_ether device
    TFTP from server 10.0.0.1; our IP address is 10.0.0.2
    Filename 'rootfs.ubifs'.
    Load address: 0x40008000
    Loading:
    #################################################################
          [snip]
          5.6 MiB/s
    done
    Bytes transferred = 13205504 (c98000 hex)
    => tftp rootfs.ubifs
    using ci_udc, OUT ep- IN ep- STATUS ep-
    MAC 00:19:b8:00:00:02
    HOST MAC 00:19:b8:00:00:01
    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
    ERROR: The remote end did not respond in time.
    at drivers/usb/gadget/ether.c:2388/usb_eth_init()
    =>

The error always appears after calling tftp the third time! The first 
two runs work fine, but the third one always fails. I run this test 
about 20 times.

Best regards
Jörg Krause
Marek Vasut June 28, 2014, 8:45 p.m. UTC | #15 
On Saturday, June 28, 2014 at 10:37:35 PM, Jörg Krause wrote:
> On 06/28/2014 09:59 PM, Marek Vasut wrote:
> > I would actually be really glad if you could test plain u-boot-usb/master
> > and see if your USB problem is present there. Also, it would be really
> > nice to isolate the sequence of commands which triggers the bug, so we
> > have a proper reproducible testcase.
> > 
> > Anyway, this thread is not a place where I would like to discuss this.
> > This should be discussed in the "original" thread, that is:
> > 
> > Subj: [PATCH] usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC
> 
> I cloned u-boot-usb/master and tested it. I am running "tftpd
> rootfs.ubifs" several times. Here is the log (I shortened the loading
> process bar):
[...]
>     => tftp rootfs.ubifs
>     using ci_udc, OUT ep- IN ep- STATUS ep-
>     MAC 00:19:b8:00:00:02
>     HOST MAC 00:19:b8:00:00:01
>     high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>     USB network up!
>     Using usb_ether device
>     TFTP from server 10.0.0.1; our IP address is 10.0.0.2
>     Filename 'rootfs.ubifs'.
>     Load address: 0x40008000
>     Loading: [snip]
>       ##############################################################T ###
>           [snip]
>           1.7 MiB/s

Here it's 1.7MB/s ...

>     done
>     Bytes transferred = 13205504 (c98000 hex)
>     => tftp rootfs.ubifs
>     using ci_udc, OUT ep- IN ep- STATUS ep-
>     MAC 00:19:b8:00:00:02
>     HOST MAC 00:19:b8:00:00:01
>     high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>     USB network up!
>     Using usb_ether device
>     TFTP from server 10.0.0.1; our IP address is 10.0.0.2
>     Filename 'rootfs.ubifs'.
>     Load address: 0x40008000
>     Loading:
>     #################################################################
>           [snip]
>           5.6 MiB/s

... and here it's 5.6 MB/s ... that's weird.

[...]

> The error always appears after calling tftp the third time! The first
> two runs work fine, but the third one always fails. I run this test
> about 20 times.

And finally, what is the contents of 'rootfs.ubifs' ?

Best regards,
Jörg Krause June 28, 2014, 8:53 p.m. UTC | #16 
On 06/28/2014 10:45 PM, Marek Vasut wrote:
> On Saturday, June 28, 2014 at 10:37:35 PM, Jörg Krause wrote:
>> On 06/28/2014 09:59 PM, Marek Vasut wrote:
>>> I would actually be really glad if you could test plain u-boot-usb/master
>>> and see if your USB problem is present there. Also, it would be really
>>> nice to isolate the sequence of commands which triggers the bug, so we
>>> have a proper reproducible testcase.
>>>
>>> Anyway, this thread is not a place where I would like to discuss this.
>>> This should be discussed in the "original" thread, that is:
>>>
>>> Subj: [PATCH] usb: ci_udc: fix interaction with CONFIG_USB_ETH_CDC
>> I cloned u-boot-usb/master and tested it. I am running "tftpd
>> rootfs.ubifs" several times. Here is the log (I shortened the loading
>> process bar):
> [...]
>>      => tftp rootfs.ubifs
>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>      MAC 00:19:b8:00:00:02
>>      HOST MAC 00:19:b8:00:00:01
>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>      USB network up!
>>      Using usb_ether device
>>      TFTP from server 10.0.0.1; our IP address is 10.0.0.2
>>      Filename 'rootfs.ubifs'.
>>      Load address: 0x40008000
>>      Loading: [snip]
>>        ##############################################################T ###
>>            [snip]
>>            1.7 MiB/s
> Here it's 1.7MB/s ...
Sometimes I get a timeout "#T " while loading, which slows down the 
transfer rate. It is not always the case.
>
>>      done
>>      Bytes transferred = 13205504 (c98000 hex)
>>      => tftp rootfs.ubifs
>>      using ci_udc, OUT ep- IN ep- STATUS ep-
>>      MAC 00:19:b8:00:00:02
>>      HOST MAC 00:19:b8:00:00:01
>>      high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
>>      USB network up!
>>      Using usb_ether device
>>      TFTP from server 10.0.0.1; our IP address is 10.0.0.2
>>      Filename 'rootfs.ubifs'.
>>      Load address: 0x40008000
>>      Loading:
>>      #################################################################
>>            [snip]
>>            5.6 MiB/s
> ... and here it's 5.6 MB/s ... that's weird.
>
> [...]
>
>> The error always appears after calling tftp the third time! The first
>> two runs work fine, but the third one always fails. I run this test
>> about 20 times.
> And finally, what is the contents of 'rootfs.ubifs' ?
It's an rootf ubifs filesystem. I also tested other files. One small 
u-boot.sb file with a filesize of 400 KB and a 62 MB compressed ubuntu 
core file (ubuntu-core-14.04-core.ext4.gz). For all I get the same result.
>
> Best regards,
Jörg Krause June 29, 2014, 8:33 p.m. UTC | #17 
On 06/28/2014 10:53 PM, Jörg Krause wrote:
>
> [snip]
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

I did some tests this weekend on u-boot-usb/master branch.

If I run "env default -a" and then "saveenv" after a reset, I get the 
same error as running three time "tftp file" in a row.
Log:

    U-Boot 2014.07-rc3-g18e0313-dirty (Jun 29 2014 - 21:56:02)

    CPU:   Freescale i.MX28 rev1.2 at 454 MHz
    BOOT:  NAND, 3V3
    DRAM:  64 MiB
    NAND:  128 MiB
    In:    serial
    Out:   serial
    Err:   serial
    Net:   usb_ether [PRIME]
    Hit any key to stop autoboot:  0
    => env default -a
    ## Resetting to default environment
    => saveenv
    Saving Environment to NAND...
    Erasing NAND...
    Erasing at 0x360000 -- 100% complete.
    Writing to NAND... OK
    => tftp rootfs.ubifs
    using ci_udc, OUT ep- IN ep- STATUS ep-
    MAC 00:19:b8:00:00:02
    HOST MAC 00:19:b8:00:00:01
    high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
    ERROR: The remote end did not respond in time.
    at drivers/usb/gadget/ether.c:2388/usb_eth_init()

"env default -a" removes stdin, stdout, stderr, and ver from the output 
of "printenv".

Looking at drivers/usb/gadget/ether.c:usb_eth_init I found the 
environment variable "cdc_connect_timeout". I played a little bit with 
the settings.

1) Using "setenv cdc_connect_timeout 1" from the command line: tftp runs 
more then three time in a row. Actually I can run tftp more than ten 
times in row and it produces no error. I tested the values 1, 3, and 15 
for cdc_connect_timeout.

2) Setting #define CONFIG_EXTRA_ENV_SETTINGS "cdc_connect_timeout=1\0" \ 
in my config header file. This does not help and produces the error on 
the fourth run of tfpd. Tested with values 1 and 3 for timeout.

Very, very strange.
Stephen Warren June 30, 2014, 4:02 p.m. UTC | #18 
On 06/27/2014 07:34 PM, Jörg Krause wrote:
> 
> On 06/28/2014 01:37 AM, Stephen Warren wrote:
>> On 06/27/2014 05:16 PM, Jörg Krause wrote:
>>> On 06/27/2014 11:55 PM, Stephen Warren wrote:
>>>> On 06/27/2014 03:37 PM, Jörg Krause wrote:
>>>>> I added the last series of patches beginning from 2014-06-10 for
>>>>> testing
>>>>> purposes. The patches from 2014-05-29 were already applied.
>>>>>
>>>>> First series of patches:
>>>>>
>>>>>      Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>>>>      Applying: usb: ci_udc: fix freeing of ep0 req
>>>>>      Applying: usb: ci_udc: fix probe error cleanup
>>>>>      Applying: usb: ci_udc: clean up all allocations in unregister
>>>>>
>>>>> Calling tftp the first time after a reset runs fine,
>>>> I thought the issue you reported was that the *first* time you run the
>>>> "tftp" command, it has issues such as timeouts? Did I misunderstand, or
>>>> did that issue somehow go away?
>>> That's right! This was the state before applying a series of patches
>>> after allow multiple buffer allocs per ep. Now, the first run of tftp
>>> runs without any errors.
>> Just to make sure I understand, here's what you saw:
>>
>> 1) tftp works fine to start with. No timeouts even on repeated
>> invocation.
> I went back in time and now I can be more precise. Everything worked
> fine until commit usb: ci_udc: allow multiple buffer allocs per ep which
> introduces timeouts in almost all tftp file downloads. Even the first
> run of tfpt can end in a timeout.
> 
>>
>> 2) You applied "allow multiple buffer allocs per ep"
> Setting #define CONFIG_SYS_CACHELINE_SIZE 32 to my config file helped
> here. But still timeouts. First run almost always runs fine, only
> sometimes timeouts while receiving a packet, but always running to the
> end. Running tftp after this a second time and more fails with a ERROR:
> The remote end did not respond in time. at
> drivers/usb/gadget/ether.c:2388/usb_eth_init(), but sometimes it works.
> 
> Setting CONFIG_SYS_CACHELINE_SIZE 32 does not make it better (as I
> previously wrote it).
> 
> Removing CONFIG_USB_GADGET_DUALSPEED helps a little bit, but I am
> getting also errors after the second or third run.

Sigh. There's been a lot of flip-flopping re: whether the cacheline size
affects the issue or not, and whether the first TFTP download always
works fine, or whether only the 3rd fails. This makes it very hard for
me to understand the issue that you're seeing. For instance, if even the
first TFTP download can fail (even if intermittently), then there's
clearly a problem with basic driver operation. However, if only the 2nd
or 3rd TFTP ever fails, then the problem is likely isolated to some part
of the cleanup/shutdown code. Given that your reports of the problem
keep flip-flopping about, it makes it hard to decide which part of the
code to look at.

For now, I'm going to simply assume that any TFTP download (1st, 2nd, or
100th) can fail intermittently, and that cache line size is irrelevant.

I'll look at the problematic commit you mentioned (2813006fecda "usb:
ci_udc: allow multiple buffer allocs per ep") and see if I can create a
series of patches that do the same thing, and have you bisect that. If I
can do that, I will ask you to test 100 times each: the commit before
the bad commit, then each of the commits in my series, always resetting
the board between each test and doing a single TFTP download. Then I'd
like to see the raw results from those tests.
Stephen Warren June 30, 2014, 7:55 p.m. UTC | #19 
On 06/30/2014 10:02 AM, Stephen Warren wrote:
> On 06/27/2014 07:34 PM, Jörg Krause wrote:
>>
>> On 06/28/2014 01:37 AM, Stephen Warren wrote:
>>> On 06/27/2014 05:16 PM, Jörg Krause wrote:
>>>> On 06/27/2014 11:55 PM, Stephen Warren wrote:
>>>>> On 06/27/2014 03:37 PM, Jörg Krause wrote:
>>>>>> I added the last series of patches beginning from 2014-06-10 for
>>>>>> testing
>>>>>> purposes. The patches from 2014-05-29 were already applied.
>>>>>>
>>>>>> First series of patches:
>>>>>>
>>>>>>      Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>>>>>      Applying: usb: ci_udc: fix freeing of ep0 req
>>>>>>      Applying: usb: ci_udc: fix probe error cleanup
>>>>>>      Applying: usb: ci_udc: clean up all allocations in unregister
>>>>>>
>>>>>> Calling tftp the first time after a reset runs fine,
>>>>> I thought the issue you reported was that the *first* time you run the
>>>>> "tftp" command, it has issues such as timeouts? Did I misunderstand, or
>>>>> did that issue somehow go away?
>>>>
>>>> That's right! This was the state before applying a series of patches
>>>> after allow multiple buffer allocs per ep. Now, the first run of tftp
>>>> runs without any errors.
>>>
>>> Just to make sure I understand, here's what you saw:
>>>
>>> 1) tftp works fine to start with. No timeouts even on repeated
>>> invocation.
>>
>> I went back in time and now I can be more precise. Everything worked
>> fine until commit usb: ci_udc: allow multiple buffer allocs per ep which
>> introduces timeouts in almost all tftp file downloads. Even the first
>> run of tfpt can end in a timeout.

I've pushed out some branches for you to test at:
git://github.com/swarren/u-boot.git

I looked back to see where the problematic commit 2813006fecda "usb:
ci_udc: allow multiple buffer allocs per ep" was first applied. I then
started with that same commit, and split up the problematic commit into
tiny pieces, and applied each piece in turn. The result is in branch
ci-udc-debug-base. If you could please test every commit in that branch:

> fabf0df9523a usb: ci_udc: remainder of "allow multiple buffer allocs"
> 373098f12a1d usb: ci_udc: dynamically alloc USB request objects
> 43fff2b11860 usb: ci_udc: dynamically alloc bounce buffer
> f21b9989bf83 usb: ci_udc: bounce only actual (rounded) packet len not buffer len for small pkts
> 09ba064f9a99 usb: ci_udc: debounce only actual packet len not buffer len
> 7e90ae0e4973 usb: ci_udc: use ci_req-> not ep-> a bit more
> 9663ff89c764 usb: ci_udc: pass ci_req not ep to bounce/debounce
> b9fdf9dd3f96 usb: ci_udc: move buffer fields from ep to req
> 82f35a839e31 usb: ci_udc: introduce struct ci_req
> c9396bc55069 usb: ci_udc: create "req" variable in bounce/unbounce
> 173d294b94cf Merge branch 'serial' of git://www.denx.de/git/u-boot-microblaze

... and tell me which commit causes the problem, that would be useful.
Since the problem is intermittent, please make sure you test each commit
many times (at least 10-20, preferably nearer 100). You can stop early
if you see the problem, but if you don't, please test many times to make
sure there is no problem.

You mentioned that your board support is not upstream. In order to test,
you should probably do: For each commit I mention above, check it out,
apply the minimal set of patches needed to support your board, and test.

Please don't apply any other patches while testing this series. Please
only test TFTP download, and not ubifs writes, etc.

I've also pushed out branch ci-udc-debug-tegra-dfu-working which has a
whole load of other commits I needed to test the split up patches. These
were needed to add board support for the board I'm currently using, and
to fix/enhance the core DFU code. Most of these commits are already
applied in u-boot.git/master, it's just that I cherry-picked them on top
of the old base commit so I would have something to test with.

BTW, I'm going on vacation from Jul 3rd-July 20th. I might answer some
limited email during this time, but I certainly won't have access to any
test HW.
Jörg Krause June 30, 2014, 8:55 p.m. UTC | #20 
On 06/30/2014 06:02 PM, Stephen Warren wrote:
> On 06/27/2014 07:34 PM, Jörg Krause wrote:
>> On 06/28/2014 01:37 AM, Stephen Warren wrote:
>>> On 06/27/2014 05:16 PM, Jörg Krause wrote:
>>>> On 06/27/2014 11:55 PM, Stephen Warren wrote:
>>>>> On 06/27/2014 03:37 PM, Jörg Krause wrote:
>>>>>> I added the last series of patches beginning from 2014-06-10 for
>>>>>> testing
>>>>>> purposes. The patches from 2014-05-29 were already applied.
>>>>>>
>>>>>> First series of patches:
>>>>>>
>>>>>>       Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>>>>>       Applying: usb: ci_udc: fix freeing of ep0 req
>>>>>>       Applying: usb: ci_udc: fix probe error cleanup
>>>>>>       Applying: usb: ci_udc: clean up all allocations in unregister
>>>>>>
>>>>>> Calling tftp the first time after a reset runs fine,
>>>>> I thought the issue you reported was that the *first* time you run the
>>>>> "tftp" command, it has issues such as timeouts? Did I misunderstand, or
>>>>> did that issue somehow go away?
>>>> That's right! This was the state before applying a series of patches
>>>> after allow multiple buffer allocs per ep. Now, the first run of tftp
>>>> runs without any errors.
>>> Just to make sure I understand, here's what you saw:
>>>
>>> 1) tftp works fine to start with. No timeouts even on repeated
>>> invocation.
>> I went back in time and now I can be more precise. Everything worked
>> fine until commit usb: ci_udc: allow multiple buffer allocs per ep which
>> introduces timeouts in almost all tftp file downloads. Even the first
>> run of tfpt can end in a timeout.
>>
>>> 2) You applied "allow multiple buffer allocs per ep"
>> Setting #define CONFIG_SYS_CACHELINE_SIZE 32 to my config file helped
>> here. But still timeouts. First run almost always runs fine, only
>> sometimes timeouts while receiving a packet, but always running to the
>> end. Running tftp after this a second time and more fails with a ERROR:
>> The remote end did not respond in time. at
>> drivers/usb/gadget/ether.c:2388/usb_eth_init(), but sometimes it works.
>>
>> Setting CONFIG_SYS_CACHELINE_SIZE 32 does not make it better (as I
>> previously wrote it).
Sorry, this is a typo. It should be CONFIG_SYS_CACHELINE_SIZE 16 (not 32).
>>
>> Removing CONFIG_USB_GADGET_DUALSPEED helps a little bit, but I am
>> getting also errors after the second or third run.
> Sigh. There's been a lot of flip-flopping re: whether the cacheline size
> affects the issue or not, and whether the first TFTP download always
> works fine, or whether only the 3rd fails. This makes it very hard for
> me to understand the issue that you're seeing. For instance, if even the
> first TFTP download can fail (even if intermittently), then there's
> clearly a problem with basic driver operation. However, if only the 2nd
> or 3rd TFTP ever fails, then the problem is likely isolated to some part
> of the cleanup/shutdown code. Given that your reports of the problem
> keep flip-flopping about, it makes it hard to decide which part of the
> code to look at.
I am very sorry if I confused you with my attempt to explain you what I 
am seeing. The "flip-flopping" comes from the different results and 
behaviour after applying a patch or a series of patches. And I also 
tried this and that and looked if it changes the behaviour. I must admit 
that this is not good testing practise.
> For now, I'm going to simply assume that any TFTP download (1st, 2nd, or
> 100th) can fail intermittently, and that cache line size is irrelevant.
>
> I'll look at the problematic commit you mentioned (2813006fecda "usb:
> ci_udc: allow multiple buffer allocs per ep") and see if I can create a
> series of patches that do the same thing, and have you bisect that. If I
> can do that, I will ask you to test 100 times each: the commit before
> the bad commit, then each of the commits in my series, always resetting
> the board between each test and doing a single TFTP download. Then I'd
> like to see the raw results from those tests.
I will do this and I hope it brings some clarifications.
Marek Vasut June 30, 2014, 9:15 p.m. UTC | #21 
On Monday, June 30, 2014 at 10:55:37 PM, Jörg Krause wrote:
[...]
> >>> 2) You applied "allow multiple buffer allocs per ep"
> >> 
> >> Setting #define CONFIG_SYS_CACHELINE_SIZE 32 to my config file helped
> >> here. But still timeouts. First run almost always runs fine, only
> >> sometimes timeouts while receiving a packet, but always running to the
> >> end. Running tftp after this a second time and more fails with a ERROR:
> >> The remote end did not respond in time. at
> >> drivers/usb/gadget/ether.c:2388/usb_eth_init(), but sometimes it works.
> >> 
> >> Setting CONFIG_SYS_CACHELINE_SIZE 32 does not make it better (as I
> >> previously wrote it).
> 
> Sorry, this is a typo. It should be CONFIG_SYS_CACHELINE_SIZE 16 (not 32).

MX28 has 32b-long cachelines. Setting this to 16 is nonsense.
[...]

Best regards,
Marek Vasut
Jörg Krause June 30, 2014, 9:43 p.m. UTC | #22 
On 06/30/2014 11:15 PM, Marek Vasut wrote:
> On Monday, June 30, 2014 at 10:55:37 PM, Jörg Krause wrote:
> [...]
>>>>> 2) You applied "allow multiple buffer allocs per ep"
>>>> Setting #define CONFIG_SYS_CACHELINE_SIZE 32 to my config file helped
>>>> here. But still timeouts. First run almost always runs fine, only
>>>> sometimes timeouts while receiving a packet, but always running to the
>>>> end. Running tftp after this a second time and more fails with a ERROR:
>>>> The remote end did not respond in time. at
>>>> drivers/usb/gadget/ether.c:2388/usb_eth_init(), but sometimes it works.
>>>>
>>>> Setting CONFIG_SYS_CACHELINE_SIZE 32 does not make it better (as I
>>>> previously wrote it).
>> Sorry, this is a typo. It should be CONFIG_SYS_CACHELINE_SIZE 16 (not 32).
> MX28 has 32b-long cachelines. Setting this to 16 is nonsense.
> [...]
Yes it is. But if I do not set cacheline size to 32 in my config header 
file it will be 64, as I stated in a previous mail.

    I checked this and I found that ARCH_DMA_MINALIGN is set to 64,
    which is not true for Freescale i.MX28. This processor has an
    ARM926EJ-S, which has an cache line size of 32. In
    arch/arm/include/asm/cache.h the macro ARCH_DMA_MINALIGN is defined
    as followed:
    #ifdef CONFIG_SYS_CACHELINE_SIZE
    #define ARCH_DMA_MINALIGN    CONFIG_SYS_CACHELINE_SIZE
    #else
    #define ARCH_DMA_MINALIGN    64
    #endif

    And in /arch/arm/cpu/arm926ejs/cache.c as followed:
    #ifndef CONFIG_SYS_CACHELINE_SIZE
    #define CONFIG_SYS_CACHELINE_SIZE    32
    #endif

    arch/arm/include/asm/cache.h does not see the definition of
    CONFIG_SYS_CACHELINE_SIZE in /arch/arm/cpu/arm926ejs/cache.c, so
    it's a bad place to put it in there.


I was just curious of the impact on the behaviour of the USB ethernet 
gadget driver.
>
> Best regards,
> Marek Vasut
Marek Vasut June 30, 2014, 9:50 p.m. UTC | #23 
On Monday, June 30, 2014 at 11:43:24 PM, Jörg Krause wrote:
> On 06/30/2014 11:15 PM, Marek Vasut wrote:
> > On Monday, June 30, 2014 at 10:55:37 PM, Jörg Krause wrote:
> > [...]
> > 
> >>>>> 2) You applied "allow multiple buffer allocs per ep"
> >>>> 
> >>>> Setting #define CONFIG_SYS_CACHELINE_SIZE 32 to my config file helped
> >>>> here. But still timeouts. First run almost always runs fine, only
> >>>> sometimes timeouts while receiving a packet, but always running to the
> >>>> end. Running tftp after this a second time and more fails with a
> >>>> ERROR: The remote end did not respond in time. at
> >>>> drivers/usb/gadget/ether.c:2388/usb_eth_init(), but sometimes it
> >>>> works.
> >>>> 
> >>>> Setting CONFIG_SYS_CACHELINE_SIZE 32 does not make it better (as I
> >>>> previously wrote it).
> >> 
> >> Sorry, this is a typo. It should be CONFIG_SYS_CACHELINE_SIZE 16 (not
> >> 32).
> > 
> > MX28 has 32b-long cachelines. Setting this to 16 is nonsense.
> > [...]
> 
> Yes it is. But if I do not set cacheline size to 32 in my config header
> file it will be 64, as I stated in a previous mail.
> 
>     I checked this and I found that ARCH_DMA_MINALIGN is set to 64,
>     which is not true for Freescale i.MX28. This processor has an
>     ARM926EJ-S, which has an cache line size of 32. In
>     arch/arm/include/asm/cache.h the macro ARCH_DMA_MINALIGN is defined
>     as followed:
>     #ifdef CONFIG_SYS_CACHELINE_SIZE
>     #define ARCH_DMA_MINALIGN    CONFIG_SYS_CACHELINE_SIZE
>     #else
>     #define ARCH_DMA_MINALIGN    64
>     #endif
> 
>     And in /arch/arm/cpu/arm926ejs/cache.c as followed:
>     #ifndef CONFIG_SYS_CACHELINE_SIZE
>     #define CONFIG_SYS_CACHELINE_SIZE    32
>     #endif
> 
>     arch/arm/include/asm/cache.h does not see the definition of
>     CONFIG_SYS_CACHELINE_SIZE in /arch/arm/cpu/arm926ejs/cache.c, so
>     it's a bad place to put it in there.
> 
> 
> I was just curious of the impact on the behaviour of the USB ethernet
> gadget driver.

Well okay, cacheline length and DMA alignment needs are two different things. 
Larger than necessary ARCH_DMA_MINALIGN cannot hurt, though there should be a 
patch adding CONFIG_SYS_CACHELINE_SIZE 32 for MXS into include/configs/mxs.h it 
seems.

Best regards,
Marek Vasut
Jörg Krause June 30, 2014, 10:44 p.m. UTC | #24 
On 06/30/2014 09:55 PM, Stephen Warren wrote:
> On 06/30/2014 10:02 AM, Stephen Warren wrote:
>> On 06/27/2014 07:34 PM, Jörg Krause wrote:
>>> On 06/28/2014 01:37 AM, Stephen Warren wrote:
>>>> On 06/27/2014 05:16 PM, Jörg Krause wrote:
>>>>> On 06/27/2014 11:55 PM, Stephen Warren wrote:
>>>>>> On 06/27/2014 03:37 PM, Jörg Krause wrote:
>>>>>>> I added the last series of patches beginning from 2014-06-10 for
>>>>>>> testing
>>>>>>> purposes. The patches from 2014-05-29 were already applied.
>>>>>>>
>>>>>>> First series of patches:
>>>>>>>
>>>>>>>       Applying: usb: ci_udc: call udc_disconnect() from ci_pullup()
>>>>>>>       Applying: usb: ci_udc: fix freeing of ep0 req
>>>>>>>       Applying: usb: ci_udc: fix probe error cleanup
>>>>>>>       Applying: usb: ci_udc: clean up all allocations in unregister
>>>>>>>
>>>>>>> Calling tftp the first time after a reset runs fine,
>>>>>> I thought the issue you reported was that the *first* time you run the
>>>>>> "tftp" command, it has issues such as timeouts? Did I misunderstand, or
>>>>>> did that issue somehow go away?
>>>>> That's right! This was the state before applying a series of patches
>>>>> after allow multiple buffer allocs per ep. Now, the first run of tftp
>>>>> runs without any errors.
>>>> Just to make sure I understand, here's what you saw:
>>>>
>>>> 1) tftp works fine to start with. No timeouts even on repeated
>>>> invocation.
>>> I went back in time and now I can be more precise. Everything worked
>>> fine until commit usb: ci_udc: allow multiple buffer allocs per ep which
>>> introduces timeouts in almost all tftp file downloads. Even the first
>>> run of tfpt can end in a timeout.
> I've pushed out some branches for you to test at:
> git://github.com/swarren/u-boot.git
>
> I looked back to see where the problematic commit 2813006fecda "usb:
> ci_udc: allow multiple buffer allocs per ep" was first applied. I then
> started with that same commit, and split up the problematic commit into
> tiny pieces, and applied each piece in turn. The result is in branch
> ci-udc-debug-base. If you could please test every commit in that branch:
>
>> fabf0df9523a usb: ci_udc: remainder of "allow multiple buffer allocs"
>> 373098f12a1d usb: ci_udc: dynamically alloc USB request objects
>> 43fff2b11860 usb: ci_udc: dynamically alloc bounce buffer
>> f21b9989bf83 usb: ci_udc: bounce only actual (rounded) packet len not buffer len for small pkts
>> 09ba064f9a99 usb: ci_udc: debounce only actual packet len not buffer len
>> 7e90ae0e4973 usb: ci_udc: use ci_req-> not ep-> a bit more
>> 9663ff89c764 usb: ci_udc: pass ci_req not ep to bounce/debounce
>> b9fdf9dd3f96 usb: ci_udc: move buffer fields from ep to req
>> 82f35a839e31 usb: ci_udc: introduce struct ci_req
>> c9396bc55069 usb: ci_udc: create "req" variable in bounce/unbounce
>> 173d294b94cf Merge branch 'serial' of git://www.denx.de/git/u-boot-microblaze
> ... and tell me which commit causes the problem, that would be useful.
> Since the problem is intermittent, please make sure you test each commit
> many times (at least 10-20, preferably nearer 100). You can stop early
> if you see the problem, but if you don't, please test many times to make
> sure there is no problem.
Commit 373098f12a1d usb: ci_udc: dynamically alloc USB request objects 
cannot be build.

The error is introduced with the latest commit fabf0df9523a usb: ci_udc: 
remainder of "allow multiple buffer allocs". I attached the output 
message for this commit.
>
> You mentioned that your board support is not upstream. In order to test,
> you should probably do: For each commit I mention above, check it out,
> apply the minimal set of patches needed to support your board, and test.
Done. Just added board support.
>
> Please don't apply any other patches while testing this series. Please
> only test TFTP download, and not ubifs writes, etc.
This is my test script:

    "test_usb=" \
         "setenv i 64; " \
         "while test ${i} -gt 0; do " \
           "echo ${i}; " \
           "tftp ${rootfs_file}; " \
           "setexpr i ${i} - 1; " \
         "done; " \
         "setenv i; " \
         "\0"

>
> I've also pushed out branch ci-udc-debug-tegra-dfu-working which has a
> whole load of other commits I needed to test the split up patches. These
> were needed to add board support for the board I'm currently using, and
> to fix/enhance the core DFU code. Most of these commits are already
> applied in u-boot.git/master, it's just that I cherry-picked them on top
> of the old base commit so I would have something to test with.
>
> BTW, I'm going on vacation from Jul 3rd-July 20th. I might answer some
> limited email during this time, but I certainly won't have access to any
> test HW.
=> reset
resetting ...
HTLLCLLC

U-Boot 2014.04-gfabf0df-dirty (Jul 01 2014 - 00:36:11)

CPU:   Freescale i.MX28 rev1.2 at 454 MHz
BOOT:  NAND, 3V3
DRAM:  64 MiB
NAND:  128 MiB
In:    serial
Out:   serial
Err:   serial
Net:   usb_ether [PRIME]
Warning: Your board does not use generic board. Please read
doc/README.generic-board and take action. Boards not
upgraded by the late 2014 may break or be removed.
Hit any key to stop autoboot:  0 
=> run test_usb 
64
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################################
	 5.8 MiB/s
done
Bytes transferred = 13205504 (c98000 hex)
63
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 ######T ###########################################################
	 #################################################################
	 #################timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet

Retry count exceeded; starting again
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################################
	 5.7 MiB/s
done
Bytes transferred = 13205504 (c98000 hex)
62
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
ERROR: The remote end did not respond in time.
at drivers/usb/gadget/ether.c:2388/usb_eth_init()
61
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################################
	 5.7 MiB/s
done
Bytes transferred = 13205504 (c98000 hex)
60
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: ############timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet

Retry count exceeded; starting again
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################################
	 5.7 MiB/s
done
Bytes transferred = 13205504 (c98000 hex)
5f
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
ERROR: The remote end did not respond in time.
at drivers/usb/gadget/ether.c:2388/usb_eth_init()
5e
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 ############################################################T #####
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################################
	 1.7 MiB/s
done
Bytes transferred = 13205504 (c98000 hex)
5d
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!
Using usb_ether device
TFTP from server 10.0.0.1; our IP address is 10.0.0.2
Filename 'rootfs.ubifs'.
Load address: 0x40008000
Loading: ################################################timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
T timeout sending packets to usb ethernet
timeout sending packets to usb ethernet

Retry count exceeded; starting again
using ci_udc, OUT ep- IN ep- STATUS ep-
MAC 00:19:b8:00:00:02
HOST MAC 00:19:b8:00:00:01
high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet
USB network up!

Abort
=>
diff mbox

Patch

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index b18bee43ad89..c3f6467b7db4 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -226,8 +226,11 @@  static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *req)
 	int num;
 
 	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
-	if (num == 0)
+	if (num == 0) {
+		if (!controller.ep0_req)
+			return;
 		controller.ep0_req = 0;
+	}
 
 	if (ci_req->b_buf)
 		free(ci_req->b_buf);
@@ -909,6 +912,9 @@  int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 {
 	udc_disconnect();
 
+	driver->unbind(&controller.gadget);
+	controller.driver = NULL;
+
 	ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req);
 	free(controller.items_mem);
 	free(controller.epts);