Skip to contentSkip to conversation

USB-CDC: called handle_interrupts inside usb_eth_send

PATCH New Stefano Babic 2 participants 5 messages Aug 11, 2010 – Aug 12, 2010 USB

lore
Share & tools
lore

Stefano Babic

author
#0 lore raw

PATCH 1/1 USB-CDC: called handle_interrupts inside usb_eth_send

The patch removes an endless loop in the usb_eth_send if the tx_complete is not called before going in the loop. The driver interrupt routine is called allowing the driver to check if the TX is completed.

Signed-off-by Stefano Babic <[email protected]>
---
 drivers/usb/gadget/ether.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)
drivers/usb/gadget/ether.c +9 -1
index 1481d76..9eb43d7 100644
@@ -1808,6 +1808,8 @@ static int usb_eth_send(struct eth_device* netdev, volatile void* packet, int le
 	struct usb_request	*req = NULL;
 
 	struct eth_dev *dev = &l_ethdev;
+	unsigned long ts;
+	unsigned long timeout = USB_CONNECT_TIMEOUT;
 	dprintf("%s:...\n",__func__);
 
 	req = dev->tx_req;

@@ -1837,9 +1839,15 @@ static int usb_eth_send(struct eth_device* netdev, volatile void* packet, int le
 
 	if (!retval)
 		dprintf("%s: packet queued\n",__func__);
+	ts = get_timer(0);
+	packet_sent=0;
 	while(!packet_sent)
 	{
-		packet_sent=0;
+		if (get_timer(ts) > timeout) {
+			printf ("timeout sending packets to usb ethernet\n");
+			return -1;
+		}
+		usb_gadget_handle_interrupts();
 	}
 
 	return 0;
Signature · 1 line
1.6.3.3
#1 lore raw

PATCH 1/1 USB-CDC: called handle_interrupts inside usb_eth_send

Hi Stefano,

2010/8/11 Stefano Babic <[email protected]>:

Quoted 14 lines

The patch removes an endless loop ?in the usb_eth_send if the tx_complete is not called before going in the loop. The driver interrupt routine is called allowing the driver to check if the TX is completed.

Signed-off-by Stefano Babic <[email protected]>

---
?drivers/usb/gadget/ether.c | ? 10 +++++++++- ?1 files changed, 9 insertions(+), 1 deletions(-)

drivers/usb/gadget/ether.c +0 -0
index 1481d76..9eb43d7 100644

drivers/usb/gadget/ether.c

Quoted 3 lines
drivers/usb/gadget/ether.c +2 -0
+ ? ? ? ts = get_timer(0);
 
+ ? ? ? packet_sent=0;

This line breaks the at91sam9261 code. Can you please fix this?

Rest of the patches do not seem to break anything and look okay to me. Thanks.

Kind regards,

Remy

author
#2 lore raw

PATCH 1/1 USB-CDC: called handle_interrupts inside usb_eth_send

Quoted 2 lines · Remy Bohmer

Hi Stefano,

Hi Remy,

Quoted 2 lines
Quoted 1 line

+ packet_sent=0;

This line breaks the at91sam9261 code. Can you please fix this?

This means to me that tx_complete() is not called for at91sam9261, as it supposed to be (or better, as I supposed..).

I see that packet_sent is reset only in the eth_init() function. Does it mean that usb_eth_send() does not wait for tx completion in the case of
at91sam9261 ?

I supposed (and I manage in this way..) that packet_sent is used as flag to detect when a single packet is really transmitted, and in this way is used when I call usb_handle_interrupts(). The callback stored into the USB request is tx_complete() that is the one responsible to set the flag. So the simple mechanism is that usb_eth_send clear the flag and wait until tx_complete sets it. It seems now to me that on at91sam9261 usb_eth_send() waits only for the first packet, and then it does not wait anymore. How can we be sure that the packet is really transmitted ?

Should I protect the line with some AT91 CONFIG or am I missing something ?

Best regards,
Stefano

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================
#3 lore raw

PATCH 1/1 USB-CDC: called handle_interrupts inside usb_eth_send

Hi Stefano,

2010/8/12 Stefano Babic <[email protected]>:

Quoted 7 lines
Quoted 2 lines · Remy Bohmer

Hi Stefano,

Hi Remy,

Quoted 1 line
Quoted 1 line

+ ? ? ? packet_sent=0;

Quoted 8 lines
Quoted 1 line

This line breaks the at91sam9261 code. Can you please fix this?

This means to me that tx_complete() is not called for at91sam9261, as it supposed to be (or better, as I supposed..).

I see that packet_sent is reset only in the eth_init() function. Does it mean that usb_eth_send() does not wait for tx completion in the case of ?at91sam9261 ?

Looking at this I agree that this packet_sent flag should be reset on every usb_eth_send().

Quoted 10 lines

I supposed (and I manage in this way..) that packet_sent is used as flag to detect when a single packet is really transmitted, and in this way is used when I call usb_handle_interrupts(). The callback stored into the USB request is tx_complete() that is the one responsible to set the flag. So the simple mechanism is that usb_eth_send clear the flag and wait until tx_complete sets it. It seems now to me that on at91sam9261 usb_eth_send() waits only for the first packet, and then it does not wait anymore. How can we be sure that the packet is really transmitted ?

Should I protect the line with some AT91 CONFIG or am I missing something ?

No, it should be AT91 specific...

Looking at the flow on at91 we see that this happens:
ether.c:usb_eth_send()

	gadget.h:usb_ep_queue()
		at91_udc.c:at91_ep_queue()
			at91_udc.c:write_fifo()
				at91_udc.c:done()
					req->req.complete()
						tx_complete()
							packet_send = 1

The packet is transmitted in the context of the usb_ep_queue() routine, not in the context of the interrupt handler. So, the timeout should incorporate usb_ep_queue also.

I think this is better:

       ts = get_timer(0);
       packet_sent=0;
       retval = usb_ep_queue (dev->in_ep, req, GFP_ATOMIC);

compared to:

       ts = get_timer(0);
       packet_sent=0;
       while(!packet_sent)

But I must mention that I have not yet tested it though...

Kind regards,

Remy

author
#4 lore raw

PATCH 1/1 USB-CDC: called handle_interrupts inside usb_eth_send

Quoted 2 lines · Remy Bohmer

Looking at this I agree that this packet_sent flag should be reset on every usb_eth_send().

Ok

Quoted 6 lines
Quoted 1 line

Should I protect the line with some AT91 CONFIG or am I missing something ?

No, it should be AT91 specific...

Looking at the flow on at91 we see that this happens:
ether.c:usb_eth_send()

Quoted 8 lines
	gadget.h:usb_ep_queue()
		at91_udc.c:at91_ep_queue()
			at91_udc.c:write_fifo()
				at91_udc.c:done()
					req->req.complete()
						tx_complete()
							packet_send = 1

Ok, understood.

Quoted 5 lines

The packet is transmitted in the context of the usb_ep_queue() routine, not in the context of the interrupt handler. So, the timeout should incorporate usb_ep_queue also.

I think this is better:

Quoted 9 lines
       ts = get_timer(0);
       packet_sent=0;
       retval = usb_ep_queue (dev->in_ep, req, GFP_ATOMIC);

compared to:

       ts = get_timer(0);
       packet_sent=0;
       while(!packet_sent)

Yes, agree, it should be enough to call usb_ep_queue after clearing the flag as you suggest.

Quoted 1 line

But I must mention that I have not yet tested it though...

It does not matter. I will repost the whole patch series with the comments we get up now. After that we can do some more tests to check if I break something...

Best regards,
Stefano

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================