Re: [linux-usb-devel] cleanup of synchronous API

2003-03-20 Thread Alan Stern 
On Thu, 20 Mar 2003, Oliver Neukum wrote:

  were doing it, I'd not use the synchronous unlink call;
  ought not to matter, but we can be more sure than that.
 
 That's dangerous. We must have absolute confidence about whether
 the message has gone over the wire or not.
 Suppose it's a reset. We'd have a device at address zero without knowing
 it. So this seems to be the easiest way.

Correct me if this is wrong, but it's not so easy to tell whether or not a
message has gone over the wire.  With UHCI, it would be necessary to
unlink it and then wait for the start of the next frame to be certain.  
And even then, for bulk OUT messages you face the possibility that the
message was received correctly by the device but the ACK handshake was
lost, so the host doesn't _know_ that the message was received.

In this case, it's very easy to use an asynchronous unlink rather than a
synchronous one.  All you have to do is wait for the struct completion
again after the unlink call, with no timeout.  Given that this whole
matter revolves around updating the synchronous API interface, it's
probably best not to use one synchronous call within the implementation of
another.

Alan Stern




---
This SF.net email is sponsored by: Tablet PC.  
Does your code think in ink? You could win a Tablet PC. 
Get a free Tablet PC hat just for playing. What are you waiting for? 
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr5043en
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] cleanup of synchronous API

2003-03-20 Thread Oliver Neukum 
Am Donnerstag, 20. März 2003 17:03 schrieb Alan Stern:
 On Thu, 20 Mar 2003, Oliver Neukum wrote:
   were doing it, I'd not use the synchronous unlink call;
   ought not to matter, but we can be more sure than that.
 
  That's dangerous. We must have absolute confidence about whether
  the message has gone over the wire or not.
  Suppose it's a reset. We'd have a device at address zero without knowing
  it. So this seems to be the easiest way.

 Correct me if this is wrong, but it's not so easy to tell whether or not a
 message has gone over the wire.  With UHCI, it would be necessary to
 unlink it and then wait for the start of the next frame to be certain.
 And even then, for bulk OUT messages you face the possibility that the
 message was received correctly by the device but the ACK handshake was
 lost, so the host doesn't _know_ that the message was received.

Is this the case for control mesages as well?
I am not familiar with the controlers at hardware level, I must admit.

 In this case, it's very easy to use an asynchronous unlink rather than a
 synchronous one.  All you have to do is wait for the struct completion
 again after the unlink call, with no timeout.  Given that this whole
 matter revolves around updating the synchronous API interface, it's
 probably best not to use one synchronous call within the implementation of
 another.

Why? It seems obvious to me to use synchronous calls in a synchronous call.

Regards
Oliver



---
This SF.net email is sponsored by: Tablet PC.
Does your code think in ink? You could win a Tablet PC.
Get a free Tablet PC hat just for playing. What are you waiting for?
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr5043en
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] cleanup of synchronous API

2003-03-20 Thread David Brownell 
Here's an updated version of Oliver's patch, changing:

 - uses struct completion for simplicity

 - the wait_event_timeout() logic is now like the other macros
   which can return before the event happens:  returns a value
 - clarity for MAX_SCHEDULE_TIMEOUT scenarios, by using the
   standard timeouts are long calling convention internally
 - uses only the async primitives

 - restores a warning when the request times out.

 - can use dev_dbg() calls in drivers/usb/core/message.c

Still needs more testing (notably the timeout paths!) and cleanup,
but I thought it'd be good to send this one around.
- Dave

--- 1.22/drivers/usb/core/message.c Wed Mar  5 07:24:34 2003
+++ edited/drivers/usb/core/message.c   Wed Mar 19 23:18:46 2003
@@ -2,6 +2,14 @@
  * message.c - synchronous message handling
  */
 
+#include linux/config.h
+
+#ifdef CONFIG_USB_DEBUG
+   #define DEBUG
+#else
+   #undef DEBUG
+#endif
+
 #include linux/pci.h /* for scatterlist macros */
 #include linux/usb.h
 #include linux/module.h
@@ -11,78 +19,57 @@
 
 #include hcd.h   /* for usbcore internals */
 
-struct usb_api_data {
-   wait_queue_head_t wqh;
-   int done;
-};
-
-static void usb_api_blocking_completion(struct urb *urb, struct pt_regs *regs)
+static void done (struct urb *urb, struct pt_regs *regs)
 {
-   struct usb_api_data *awd = (struct usb_api_data *)urb-context;
-
-   awd-done = 1;
-   wmb();
-   wake_up(awd-wqh);
+   dev_dbg (urb-dev-dev, urb %p done\n, urb);
+   complete ((struct completion *)urb-context);
 }
 
 // Starts urb and waits for completion or timeout
-static int usb_start_wait_urb(struct urb *urb, int timeout, int* actual_length)
+static int usb_start_wait_urb(struct urb *urb, long timeout, int *actual_length)
 { 
-   DECLARE_WAITQUEUE(wait, current);
-   struct usb_api_data awd;
+   DECLARE_COMPLETION (wait);
int status;
 
-   init_waitqueue_head(awd.wqh);  
-   awd.done = 0;
+   might_sleep();
 
-   set_current_state(TASK_UNINTERRUPTIBLE);
-   add_wait_queue(awd.wqh, wait);
-
-   urb-context = awd;
-   status = usb_submit_urb(urb, GFP_ATOMIC);
+   urb-context = wait;
+   urb-complete = done;
+   urb-transfer_flags |= URB_ASYNC_UNLINK;
+   status = usb_submit_urb (urb, GFP_NOIO);
if (status) {
// something went wrong
-   usb_free_urb(urb);
-   set_current_state(TASK_RUNNING);
-   remove_wait_queue(awd.wqh, wait);
return status;
}
 
-   while (timeout  !awd.done)
-   {
-   timeout = schedule_timeout(timeout);
-   set_current_state(TASK_UNINTERRUPTIBLE);
-   rmb();
-   }
-
-   set_current_state(TASK_RUNNING);
-   remove_wait_queue(awd.wqh, wait);
-
-   if (!timeout  !awd.done) {
-   if (urb-status != -EINPROGRESS) {  /* No callback?!! */
-   printk(KERN_ERR usb: raced timeout, 
-   pipe 0x%x status %d time left %d\n,
-   urb-pipe, urb-status, timeout);
-   status = urb-status;
-   } else {
-   warn(usb_control/bulk_msg: timeout);
-   usb_unlink_urb(urb);  // remove urb safely
-   status = -ETIMEDOUT;
+   /* cancel request if we time out */
+   if (wait_event_timeout (wait.wait, wait.done, timeout) == 0) {
+   if ((status = usb_unlink_urb (urb)) == -EINPROGRESS)
+   wait_event (wait.wait, wait.done);
+   /* patch status unless we raced a real completion */
+   if (urb-status == -ECONNRESET) {
+   dev_warn (urb-dev-dev,
+   timeout urb for ep%d%s (len %d/%d)\n,
+   usb_pipeendpoint (urb-pipe),
+   usb_pipein (urb-pipe) ? in : out,
+   urb-actual_length,
+   urb-transfer_buffer_length);
+   urb-status = -ETIME;
}
-   } else
-   status = urb-status;
+   }
+   status = urb-status;
 
if (actual_length)
*actual_length = urb-actual_length;
-
-   usb_free_urb(urb);
-   return status;
+   
+   return status;
 }
 
 /*---*/
 // returns status (negative) or length (positive)
 int usb_internal_control_msg(struct usb_device *usb_dev, unsigned int pipe, 
-   struct usb_ctrlrequest *cmd,  void *data, int len, int 
timeout)
+   struct usb_ctrlrequest *cmd,  void *data, int len,
+   int timeout /* 0 means MAX_SCHEDULE_TIMEOUT */ )
 {
struct urb *urb;
int retv;
@@ -93,9 +80,12 @@
return -ENOMEM;

Re: [linux-usb-devel] cleanup of synchronous API

2003-03-19 Thread Pete Zaitcev 
 From: Oliver Neukum [EMAIL PROTECTED]
 Date: Wed, 19 Mar 2003 21:44:10 +0100

 +#define __wait_event_timeout(wq, condition, ret) \
 + if (condition)  \
 + break;  \

I find it deeply disturbing to see this sort of trick.
For one thing, it constrains you to a macro implementation.
Why don't you take it to linux-kernel?

-- Pete


---
This SF.net email is sponsored by: Does your code think in ink? 
You could win a Tablet PC. Get a free Tablet PC hat just for playing. 
What are you waiting for?
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr5043en
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] cleanup of synchronous API

2003-03-19 Thread Oliver Neukum 
Am Mittwoch, 19. März 2003 22:00 schrieb Pete Zaitcev:
  From: Oliver Neukum [EMAIL PROTECTED]
  Date: Wed, 19 Mar 2003 21:44:10 +0100
 
  +#define __wait_event_timeout(wq, condition, ret)   \
  +   if (condition)  \
  +   break;  \

 I find it deeply disturbing to see this sort of trick.
 For one thing, it constrains you to a macro implementation.
 Why don't you take it to linux-kernel?

I don't understand this remark. Could you elaborate?
What's wrong about adding the fourth macro of this kind to the
macros already present there? I asked on linux-kernel and nobody
so far has answered.
As these are the new macros in 2.5 exactly for such purposes
shouldn't such macros be used?
What exactly is the problem you see here?

Regards
Oliver



---
This SF.net email is sponsored by: Does your code think in ink?
You could win a Tablet PC. Get a free Tablet PC hat just for playing.
What are you waiting for?
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr5043en
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] cleanup of synchronous API

2003-03-19 Thread Pete Zaitcev 
 From: Oliver Neukum [EMAIL PROTECTED]
 Date: Wed, 19 Mar 2003 22:13:07 +0100

   From: Oliver Neukum [EMAIL PROTECTED]
   Date: Wed, 19 Mar 2003 21:44:10 +0100
  
   +#define __wait_event_timeout(wq, condition, ret) \
   + if (condition)  \
   + break;  \
 
  I find it deeply disturbing to see this sort of trick.
  For one thing, it constrains you to a macro implementation.
  Why don't you take it to linux-kernel?
 
 I don't understand this remark. Could you elaborate?
 What's wrong about adding the fourth macro of this kind to the
 macros already present there? I asked on linux-kernel and nobody
 so far has answered.

Oh well, whatever. Indeed, it's all there already.

-- Pete


---
This SF.net email is sponsored by: Does your code think in ink? 
You could win a Tablet PC. Get a free Tablet PC hat just for playing. 
What are you waiting for?
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr5043en
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] cleanup of synchronous API

2003-03-19 Thread David Brownell 
Oliver Neukum wrote:
Hi Dave,

does this remove your doubts about race conditions in the synchronous API?
It's a lot closer ... I'll try it.  Yes, it's odd that there
was no pre-existing wait_event_timeout(), and that's the
call we need to make that implementation behave right.
Some further cleanup:  abolish struct usb_api_data, it's
exactly the same as struct completion.  Then likely that
completion callback can just call complete(). And why are
you initializing the waitqueue head twice?  Also:  if I
were doing it, I'd not use the synchronous unlink call;
ought not to matter, but we can be more sure than that.
Curiously enough, I seem to have found a way to reproduce
the previous raced timeout message -- which is IMO proof
that my doubts were well deserved.  (Specifically the ones
about the implementation being broken; API is another issue.)
Basically, with the system under heavy USB loads (two EHCI
devices putting 20+ MByte/sec loads, plus the device side
logic of one of those -- net 70+ MByte/sec!) and interrupt
load to match ... system latencies were high enough that I
couldn't enumerate a new device.  I got the raced message,
then first khubd and then the rest of the USB stack locked
up.  There was another wierdness going on too, but clearly
the synchronization replaced by your patch was also broken.
- Dave







---
This SF.net email is sponsored by: Does your code think in ink? 
You could win a Tablet PC. Get a free Tablet PC hat just for playing. 
What are you waiting for?
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr5043en
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] cleanup of synchronous API

2003-03-19 Thread Oliver Neukum 
Am Mittwoch, 19. März 2003 23:39 schrieb David Brownell:
 Oliver Neukum wrote:
  Hi Dave,
 
  does this remove your doubts about race conditions in the synchronous
  API?

 It's a lot closer ... I'll try it.  Yes, it's odd that there
 was no pre-existing wait_event_timeout(), and that's the
 call we need to make that implementation behave right.

I thought about that. I still have no clue as to the reason.

 Some further cleanup:  abolish struct usb_api_data, it's
 exactly the same as struct completion.  Then likely that
 completion callback can just call complete(). And why are
 you initializing the waitqueue head twice?  Also:  if I

Do I? Where? Cerebral insufficiency, most likely. It's past midnight
here.

 were doing it, I'd not use the synchronous unlink call;
 ought not to matter, but we can be more sure than that.

That's dangerous. We must have absolute confidence about whether
the message has gone over the wire or not.
Suppose it's a reset. We'd have a device at address zero without knowing
it. So this seems to be the easiest way.

 Curiously enough, I seem to have found a way to reproduce
 the previous raced timeout message -- which is IMO proof
 that my doubts were well deserved.  (Specifically the ones
 about the implementation being broken; API is another issue.)

 Basically, with the system under heavy USB loads (two EHCI
 devices putting 20+ MByte/sec loads, plus the device side
 logic of one of those -- net 70+ MByte/sec!) and interrupt
 load to match ... system latencies were high enough that I
 couldn't enumerate a new device.  I got the raced message,
 then first khubd and then the rest of the USB stack locked
 up.  There was another wierdness going on too, but clearly
 the synchronization replaced by your patch was also broken.

This is odd. How was the system load? Doesn't it have to be insanely
high for a kernel thread not running for several seconds after being
woken up?
Or is this an IO scheduling bug? Control messages should have priority
over bulk, shouldn't they?

Regards
Oliver



---
This SF.net email is sponsored by: Does your code think in ink?
You could win a Tablet PC. Get a free Tablet PC hat just for playing.
What are you waiting for?
http://ads.sourceforge.net/cgi-bin/redirect.pl?micr5043en
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel