Re: [linux-usb-devel] anchors and their use

2007-05-25 Thread Oliver Neukum 
Am Freitag, 25. Mai 2007 04:30 schrieb Alan Stern:
 
 For a simple example driver, this is getting distressingly complicated.

Our API is growing. We've introduced pre/post_reset and autosuspend
within a recentlyish timeframe. How could a generic example avoid reflecting
that?

 Instead of worrying too much about error reporting, how about
 implementing exclusive-open semantics?  That would simplify things a
 lot.  Note that for simple drivers, exclusive-open is the rule rather
 than the exception.

It does not help. As yet I don't try to trace whose requests fail. Were
I to do that, the patch would become even larger. In that sense it does
implement exclusive open.

Apart from that a generic example must have proper error handling.
If we don't do it there we'll pay for it later fixing all derived drivers.

 The order of your new routines is strange: suspend, pre_reset, resume,
 post_reset.  It's more natural to do suspend, resume, pre_reset,
 post_reset.

Before an event - after an event.
I'll change it. Furthermore, I'll break up this patch into pieces
to make it simpler to review.

 There is no call to usb_pm_put_interface().

Oops. Thanks.

Regards
Oliver

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-24 Thread Oliver Neukum 
Am Dienstag, 22. Mai 2007 21:48 schrieb David Brownell:
 On Tuesday 22 May 2007, Alan Stern wrote:
 
  Timeouts are specified in jiffies, so your timeout argument
  should be long or unsigned long, not int.
 
 Timeouts are best specified in milliseconds though... that's been
 the policy inside the USB stack since someone's search-and-destroy
 mission a few years back.  Most people don't handle jiffies right.

Hi,

here's the use of anchors to implement a couple of methods in the
skeleton driver, which is thus brought up to standard. This patch does:

- correctly handle disconnect with an anchor
- correctly handle suspend with an anchor
- correctly handle pre_reset with an anchor

in order to have a correct handling of pre_reset it further:

- introduces error reporting in the write path
- reports resets to user space
- implements flush() to isolate consecutive calls to open()

Regards
Oliver
---

--- a/drivers/usb/usb-skeleton.c2007-05-22 14:50:34.0 +0200
+++ b/drivers/usb/usb-skeleton.c2007-05-24 08:47:29.0 +0200
@@ -34,10 +34,6 @@ static struct usb_device_id skel_table [
 };
 MODULE_DEVICE_TABLE(usb, skel_table);
 
-/* to prevent a race between open and disconnect */
-static DEFINE_MUTEX(skel_open_lock);
-
-
 /* Get a minor range for your devices from the usb maintainer */
 #define USB_SKEL_MINOR_BASE192
 
@@ -54,16 +50,20 @@ struct usb_skel {
struct usb_device   *udev;  /* the usb device for 
this device */
struct usb_interface*interface; /* the interface for 
this device */
struct semaphorelimit_sem;  /* limiting the number 
of writes in progress */
+   struct usb_anchor   submitted;  /* in case we need to 
retract our submissions */
unsigned char   *bulk_in_buffer;/* the buffer to 
receive data */
size_t  bulk_in_size;   /* the size of the 
receive buffer */
__u8bulk_in_endpointAddr;   /* the address of the 
bulk in endpoint */
__u8bulk_out_endpointAddr;  /* the address of the 
bulk out endpoint */
+   int errors; /* the last request 
tanked */
+   spinlock_t  err_lock;   /* lock for errors */
struct kref kref;
struct mutexio_mutex;   /* synchronize I/O with 
disconnect */
 };
 #define to_skel_dev(d) container_of(d, struct usb_skel, kref)
 
 static struct usb_driver skel_driver;
+static void skel_draw_down(struct usb_skel *dev);
 
 static void skel_delete(struct kref *kref)
 {
@@ -83,10 +83,8 @@ static int skel_open(struct inode *inode
 
subminor = iminor(inode);
 
-   mutex_lock(skel_open_lock);
interface = usb_find_interface(skel_driver, subminor);
if (!interface) {
-   mutex_unlock(skel_open_lock);
err (%s - error, can't find device for minor %d,
 __FUNCTION__, subminor);
retval = -ENODEV;
@@ -95,25 +93,26 @@ static int skel_open(struct inode *inode
 
dev = usb_get_intfdata(interface);
if (!dev) {
-   mutex_unlock(skel_open_lock);
retval = -ENODEV;
goto exit;
}
 
/* increment our usage count for the device */
kref_get(dev-kref);
-   /* now we can drop the lock */
-   mutex_unlock(skel_open_lock);
+   /* to guard against an ongoing reset */
+   mutex_lock(dev-io_mutex);
 
/* prevent the device from being autosuspended */
retval = usb_autopm_get_interface(interface);
if (retval) {
+   mutex_unlock(dev-io_mutex);
kref_put(dev-kref, skel_delete);
goto exit;
}
 
/* save our object in the file's private structure */
file-private_data = dev;
+   mutex_unlock(dev-io_mutex);
 
 exit:
return retval;
@@ -138,6 +137,30 @@ static int skel_release(struct inode *in
return 0;
 }
 
+static int skel_flush(struct file *file, fl_owner_t id)
+{
+   struct usb_skel *dev;
+   int res;
+
+   dev = (struct usb_skel *)file-private_data;
+   if (dev == NULL)
+   return -ENODEV;
+
+   /* wait for io to stop */
+   mutex_lock(dev-io_mutex);
+   skel_draw_down(dev);
+
+   /* read out errors, leave subsequent opens a clean slate */
+   spin_lock_irq(dev-err_lock);
+   res = dev-errors ? (dev-errors == -EPIPE ? -EPIPE : -EIO) : 0;
+   dev-errors = 0;
+   spin_unlock_irq(dev-err_lock);
+
+   mutex_unlock(dev-io_mutex);
+
+   return res;
+}
+
 static ssize_t skel_read(struct file *file, char *buffer, size_t count, loff_t 
*ppos)
 {
struct usb_skel *dev;
@@ -179,14 +202,17 @@ static void skel_write_bulk_callback(str
dev = (struct usb_skel *)urb-context;

Re: [linux-usb-devel] anchors and their use

2007-05-24 Thread Alan Stern 
On Thu, 24 May 2007, Oliver Neukum wrote:

 Hi,
 
 here's the use of anchors to implement a couple of methods in the
 skeleton driver, which is thus brought up to standard. This patch does:
 
 - correctly handle disconnect with an anchor
 - correctly handle suspend with an anchor
 - correctly handle pre_reset with an anchor
 
 in order to have a correct handling of pre_reset it further:
 
 - introduces error reporting in the write path
 - reports resets to user space
 - implements flush() to isolate consecutive calls to open()

For a simple example driver, this is getting distressingly complicated.
Instead of worrying too much about error reporting, how about
implementing exclusive-open semantics?  That would simplify things a
lot.  Note that for simple drivers, exclusive-open is the rule rather
than the exception.

The order of your new routines is strange: suspend, pre_reset, resume,
post_reset.  It's more natural to do suspend, resume, pre_reset,
post_reset.

There is no call to usb_pm_put_interface().

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-23 Thread Oliver Neukum 
Am Dienstag, 22. Mai 2007 17:12 schrieb Alan Stern:

 Very good.  I did notice two more things:
 
   Timeouts are specified in jiffies, so your timeout argument
   should be long or unsigned long, not int.
 
   There's a race if two threads call usb_unanchor_urb() for the
   same URB at the same time.  You need to recheck while holding 
   the anchor's spinlock that urb-anchor is still equal to 
   anchor.  You might also want to add
 
   WARN_ON(urb-anchor != NULL);
 
   to usb_anchor_urb(), just as a precaution.

OK, these are fixed and the timeout given in milliseconds, as David
recommended.

I looked into the inlining issue and the inlined version wins in terms
of size.

Regards
Oliver
-

--- a/include/linux/usb.h   2007-05-22 14:51:01.0 +0200
+++ b/include/linux/usb.h   2007-05-23 10:20:35.0 +0200
@@ -958,11 +958,26 @@ struct usb_iso_packet_descriptor {
 
 struct urb;
 
+struct usb_anchor {
+   struct list_head urb_list;
+   wait_queue_head_t wait;
+   spinlock_t lock;
+};
+
+static inline void init_usb_anchor(struct usb_anchor *anchor)
+{
+   INIT_LIST_HEAD(anchor-urb_list);
+   init_waitqueue_head(anchor-wait);
+   spin_lock_init(anchor-lock);
+}
+
 typedef void (*usb_complete_t)(struct urb *);
 
 /**
  * struct urb - USB Request Block
  * @urb_list: For use by current owner of the URB.
+ * @anchor_list: membership in the list of an anchor
+ * @anchor: to anchor URBs to a common mooring
  * @pipe: Holds endpoint number, direction, type, and more.
  * Create these values with the eight macros available;
  * usb_{snd,rcv}TYPEpipe(dev,endpoint), where the TYPE is ctrl
@@ -1135,6 +1150,8 @@ struct urb
/* public: documented fields in the urb that can be used by drivers */
struct list_head urb_list;  /* list head for use by the urb's
 * current owner */
+   struct list_head anchor_list;   /* the URB may be anchored by the 
driver */
+   struct usb_anchor *anchor;
struct usb_device *dev; /* (in) pointer to associated device */
unsigned int pipe;  /* (in) pipe information */
int status; /* (return) non-ISO status */
@@ -1270,6 +1287,11 @@ extern struct urb *usb_get_urb(struct ur
 extern int usb_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_unlink_urb(struct urb *urb);
 extern void usb_kill_urb(struct urb *urb);
+extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
+extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
+extern void usb_unanchor_urb(struct urb *urb);
+extern int usb_wait_anchor_empty_timeout
+(struct usb_anchor *anchor, unsigned long timeout);
 
 void *usb_buffer_alloc (struct usb_device *dev, size_t size,
gfp_t mem_flags, dma_addr_t *dma);
--- a/drivers/usb/core/hcd.c2007-05-22 14:50:32.0 +0200
+++ b/drivers/usb/core/hcd.c2007-05-22 15:36:17.0 +0200
@@ -1412,6 +1412,8 @@ void usb_hcd_giveback_urb (struct usb_hc
}
 
usbmon_urb_complete (hcd-self, urb);
+   usb_unanchor_urb(urb);
+
/* pass ownership to the completion handler */
urb-complete (urb);
atomic_dec (urb-use_count);
--- a/drivers/usb/core/urb.c2007-05-22 14:40:12.0 +0200
+++ b/drivers/usb/core/urb.c2007-05-23 10:20:01.0 +0200
@@ -4,6 +4,7 @@
 #include linux/slab.h
 #include linux/init.h
 #include linux/usb.h
+#include linux/wait.h
 #include hcd.h
 
 #define to_urb(d) container_of(d, struct urb, kref)
@@ -11,6 +12,7 @@
 static void urb_destroy(struct kref *kref)
 {
struct urb *urb = to_urb(kref);
+
kfree(urb);
 }
 
@@ -34,6 +36,7 @@ void usb_init_urb(struct urb *urb)
memset(urb, 0, sizeof(*urb));
kref_init(urb-kref);
spin_lock_init(urb-lock);
+   INIT_LIST_HEAD(urb-anchor_list);
}
 }
 
@@ -100,8 +103,60 @@ struct urb * usb_get_urb(struct urb *urb
kref_get(urb-kref);
return urb;
 }
-   
-   
+
+/**
+ * usb_anchor_urb - anchors an URB while it is processed
+ * @urb: pointer to the urb to anchor
+ * @anchor: pointer to the anchor
+ *
+ * This can be called to have access to URBs which are to be executed
+ * without bothering to track them
+ */
+
+void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(anchor-lock, flags);
+   usb_get_urb(urb);
+   list_add_tail(urb-anchor_list, anchor-urb_list);
+   urb-anchor = anchor;
+   spin_unlock_irqrestore(anchor-lock, flags);
+}
+
+/**
+ * usb_unanchor_urb - unanchors an URB
+ * @urb: pointer to the urb to anchor
+ *
+ * Call this to stop the system keeping track of this URB
+ */
+
+void usb_unanchor_urb(struct urb *urb)
+{
+   unsigned long flags;
+   struct usb_anchor *anchor;
+
+

Re: [linux-usb-devel] anchors and their use

2007-05-23 Thread Alan Stern 
On Wed, 23 May 2007, Oliver Neukum wrote:

 OK, these are fixed and the timeout given in milliseconds, as David
 recommended.
 
 I looked into the inlining issue and the inlined version wins in terms
 of size.


 @@ -1270,6 +1287,11 @@ extern struct urb *usb_get_urb(struct ur
  extern int usb_submit_urb(struct urb *urb, gfp_t mem_flags);
  extern int usb_unlink_urb(struct urb *urb);
  extern void usb_kill_urb(struct urb *urb);
 +extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
 +extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
 +extern void usb_unanchor_urb(struct urb *urb);
 +extern int usb_wait_anchor_empty_timeout
 +(struct usb_anchor *anchor, unsigned long timeout);

Suspicious line break.

 +int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor, unsigned long 
 timeout)
 +{
 + int res;
 +
 + res = wait_event_timeout(anchor-wait, list_empty(anchor-urb_list), 
 timeout * HZ / 1000);

What's wrong with msecs_to_jiffies()?

Ironically, it wants its argument to be an unsigned int!

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-23 Thread Oliver Neukum 
Am Mittwoch, 23. Mai 2007 17:39 schrieb Alan Stern:
 On Wed, 23 May 2007, Oliver Neukum wrote:
 
  OK, these are fixed and the timeout given in milliseconds, as David
  recommended.
  
  I looked into the inlining issue and the inlined version wins in terms
  of size.
 
 
  @@ -1270,6 +1287,11 @@ extern struct urb *usb_get_urb(struct ur
   extern int usb_submit_urb(struct urb *urb, gfp_t mem_flags);
   extern int usb_unlink_urb(struct urb *urb);
   extern void usb_kill_urb(struct urb *urb);
  +extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
  +extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
  +extern void usb_unanchor_urb(struct urb *urb);
  +extern int usb_wait_anchor_empty_timeout
  +(struct usb_anchor *anchor, unsigned long timeout);
 
 Suspicious line break.

This is what I get when I want to keep the 80 column limit.

  +int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor, unsigned long 
  timeout)
  +{
  +   int res;
  +
  +   res = wait_event_timeout(anchor-wait, list_empty(anchor-urb_list), 
  timeout * HZ / 1000);
 
 What's wrong with msecs_to_jiffies()?

Nothing. The defficiency is my mind and memory.

I'll make the change.

Regards
Oliver

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-23 Thread Alan Stern 
On Wed, 23 May 2007, Oliver Neukum wrote:

   +extern int usb_wait_anchor_empty_timeout
   +(struct usb_anchor *anchor, unsigned long timeout);
  
  Suspicious line break.
 
 This is what I get when I want to keep the 80 column limit.

I would write it like this:

extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor, 
unsigned long timeout);

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-23 Thread Oliver Neukum 
Here we go again.

Regards
Oliver
---

--- a/drivers/usb/core/hcd.c2007-05-22 14:50:32.0 +0200
+++ b/drivers/usb/core/hcd.c2007-05-22 15:36:17.0 +0200
@@ -1412,6 +1412,8 @@ void usb_hcd_giveback_urb (struct usb_hc
}
 
usbmon_urb_complete (hcd-self, urb);
+   usb_unanchor_urb(urb);
+
/* pass ownership to the completion handler */
urb-complete (urb);
atomic_dec (urb-use_count);
--- a/include/linux/usb.h   2007-05-22 14:51:01.0 +0200
+++ b/include/linux/usb.h   2007-05-23 18:49:02.0 +0200
@@ -958,11 +958,26 @@ struct usb_iso_packet_descriptor {
 
 struct urb;
 
+struct usb_anchor {
+   struct list_head urb_list;
+   wait_queue_head_t wait;
+   spinlock_t lock;
+};
+
+static inline void init_usb_anchor(struct usb_anchor *anchor)
+{
+   INIT_LIST_HEAD(anchor-urb_list);
+   init_waitqueue_head(anchor-wait);
+   spin_lock_init(anchor-lock);
+}
+
 typedef void (*usb_complete_t)(struct urb *);
 
 /**
  * struct urb - USB Request Block
  * @urb_list: For use by current owner of the URB.
+ * @anchor_list: membership in the list of an anchor
+ * @anchor: to anchor URBs to a common mooring
  * @pipe: Holds endpoint number, direction, type, and more.
  * Create these values with the eight macros available;
  * usb_{snd,rcv}TYPEpipe(dev,endpoint), where the TYPE is ctrl
@@ -1135,6 +1150,8 @@ struct urb
/* public: documented fields in the urb that can be used by drivers */
struct list_head urb_list;  /* list head for use by the urb's
 * current owner */
+   struct list_head anchor_list;   /* the URB may be anchored by the 
driver */
+   struct usb_anchor *anchor;
struct usb_device *dev; /* (in) pointer to associated device */
unsigned int pipe;  /* (in) pipe information */
int status; /* (return) non-ISO status */
@@ -1270,6 +1287,11 @@ extern struct urb *usb_get_urb(struct ur
 extern int usb_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_unlink_urb(struct urb *urb);
 extern void usb_kill_urb(struct urb *urb);
+extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
+extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
+extern void usb_unanchor_urb(struct urb *urb);
+extern int usb_wait_anchor_empty_timeout
+(struct usb_anchor *anchor, unsigned int timeout);
 
 void *usb_buffer_alloc (struct usb_device *dev, size_t size,
gfp_t mem_flags, dma_addr_t *dma);
--- a/drivers/usb/core/urb.c2007-05-22 14:40:12.0 +0200
+++ b/drivers/usb/core/urb.c2007-05-23 18:48:06.0 +0200
@@ -4,6 +4,7 @@
 #include linux/slab.h
 #include linux/init.h
 #include linux/usb.h
+#include linux/wait.h
 #include hcd.h
 
 #define to_urb(d) container_of(d, struct urb, kref)
@@ -11,6 +12,7 @@
 static void urb_destroy(struct kref *kref)
 {
struct urb *urb = to_urb(kref);
+
kfree(urb);
 }
 
@@ -34,6 +36,7 @@ void usb_init_urb(struct urb *urb)
memset(urb, 0, sizeof(*urb));
kref_init(urb-kref);
spin_lock_init(urb-lock);
+   INIT_LIST_HEAD(urb-anchor_list);
}
 }
 
@@ -100,8 +103,60 @@ struct urb * usb_get_urb(struct urb *urb
kref_get(urb-kref);
return urb;
 }
-   
-   
+
+/**
+ * usb_anchor_urb - anchors an URB while it is processed
+ * @urb: pointer to the urb to anchor
+ * @anchor: pointer to the anchor
+ *
+ * This can be called to have access to URBs which are to be executed
+ * without bothering to track them
+ */
+
+void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(anchor-lock, flags);
+   usb_get_urb(urb);
+   list_add_tail(urb-anchor_list, anchor-urb_list);
+   urb-anchor = anchor;
+   spin_unlock_irqrestore(anchor-lock, flags);
+}
+
+/**
+ * usb_unanchor_urb - unanchors an URB
+ * @urb: pointer to the urb to anchor
+ *
+ * Call this to stop the system keeping track of this URB
+ */
+
+void usb_unanchor_urb(struct urb *urb)
+{
+   unsigned long flags;
+   struct usb_anchor *anchor;
+
+   if (!urb)
+   return;
+
+   anchor = urb-anchor;
+   if (!anchor)
+   return;
+
+   spin_lock_irqsave(anchor-lock, flags);
+   if (unlikely(anchor != urb-anchor)) {
+   /* we've lost the race to another thread */
+   spin_unlock_irqrestore(anchor-lock, flags);
+   return;
+   }
+   urb-anchor = NULL;
+   list_del(urb-anchor_list);
+   spin_unlock_irqrestore(anchor-lock, flags);
+   usb_put_urb(urb);
+   if (list_empty(anchor-urb_list))
+   wake_up(anchor-wait);
+}
+
 /*---*/
 
 /**
@@ -477,12 +532,57 @@ void

Re: [linux-usb-devel] anchors and their use

2007-05-22 Thread Oliver Neukum 
Am Mittwoch, 16. Mai 2007 17:30 schrieb Alan Stern:

 This won't build because you forgot to export usb_unanchor_urb().
 
 You could make usb_init_anchor() non-inline.  I'm not sure where 
 tradeoff lies, but you might be over the limit.
 
 I don't like the way you combined the error handling in usb-skeleton 
 with the anchor stuff.  Better to keep the two things in separate 
 patches.

Hi,

I hope this version addresses your concerns

Regards
Oliver
-- 

--- a/include/linux/usb.h   2007-05-22 14:51:01.0 +0200
+++ b/include/linux/usb.h   2007-05-22 15:36:17.0 +0200
@@ -958,11 +958,26 @@ struct usb_iso_packet_descriptor {
 
 struct urb;
 
+struct usb_anchor {
+   struct list_head urb_list;
+   wait_queue_head_t wait;
+   spinlock_t lock;
+};
+
+static inline void init_usb_anchor(struct usb_anchor *anchor)
+{
+   INIT_LIST_HEAD(anchor-urb_list);
+   init_waitqueue_head(anchor-wait);
+   spin_lock_init(anchor-lock);
+}
+
 typedef void (*usb_complete_t)(struct urb *);
 
 /**
  * struct urb - USB Request Block
  * @urb_list: For use by current owner of the URB.
+ * @anchor_list: membership in the list of an anchor
+ * @anchor: to anchor URBs to a common mooring
  * @pipe: Holds endpoint number, direction, type, and more.
  * Create these values with the eight macros available;
  * usb_{snd,rcv}TYPEpipe(dev,endpoint), where the TYPE is ctrl
@@ -1135,6 +1150,8 @@ struct urb
/* public: documented fields in the urb that can be used by drivers */
struct list_head urb_list;  /* list head for use by the urb's
 * current owner */
+   struct list_head anchor_list;   /* the URB may be anchored by the 
driver */
+   struct usb_anchor *anchor;
struct usb_device *dev; /* (in) pointer to associated device */
unsigned int pipe;  /* (in) pipe information */
int status; /* (return) non-ISO status */
@@ -1270,6 +1287,10 @@ extern struct urb *usb_get_urb(struct ur
 extern int usb_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_unlink_urb(struct urb *urb);
 extern void usb_kill_urb(struct urb *urb);
+extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
+extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
+extern void usb_unanchor_urb(struct urb *urb);
+extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor, int 
timeout);
 
 void *usb_buffer_alloc (struct usb_device *dev, size_t size,
gfp_t mem_flags, dma_addr_t *dma);
--- a/drivers/usb/core/hcd.c2007-05-22 14:50:32.0 +0200
+++ b/drivers/usb/core/hcd.c2007-05-22 15:36:17.0 +0200
@@ -1412,6 +1412,8 @@ void usb_hcd_giveback_urb (struct usb_hc
}
 
usbmon_urb_complete (hcd-self, urb);
+   usb_unanchor_urb(urb);
+
/* pass ownership to the completion handler */
urb-complete (urb);
atomic_dec (urb-use_count);
--- a/drivers/usb/core/urb.c2007-05-22 14:40:12.0 +0200
+++ b/drivers/usb/core/urb.c2007-05-22 15:38:32.0 +0200
@@ -4,6 +4,7 @@
 #include linux/slab.h
 #include linux/init.h
 #include linux/usb.h
+#include linux/wait.h
 #include hcd.h
 
 #define to_urb(d) container_of(d, struct urb, kref)
@@ -11,6 +12,7 @@
 static void urb_destroy(struct kref *kref)
 {
struct urb *urb = to_urb(kref);
+
kfree(urb);
 }
 
@@ -34,6 +36,7 @@ void usb_init_urb(struct urb *urb)
memset(urb, 0, sizeof(*urb));
kref_init(urb-kref);
spin_lock_init(urb-lock);
+   INIT_LIST_HEAD(urb-anchor_list);
}
 }
 
@@ -100,8 +103,55 @@ struct urb * usb_get_urb(struct urb *urb
kref_get(urb-kref);
return urb;
 }
-   
-   
+
+/**
+ * usb_anchor_urb - anchors an URB while it is processed
+ * @urb: pointer to the urb to anchor
+ * @anchor: pointer to the anchor
+ *
+ * This can be called to have access to URBs which are to be executed
+ * without bothering to track them
+ */
+
+void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(anchor-lock, flags);
+   usb_get_urb(urb);
+   list_add_tail(urb-anchor_list, anchor-urb_list);
+   urb-anchor = anchor;
+   spin_unlock_irqrestore(anchor-lock, flags);
+}
+
+/**
+ * usb_unanchor_urb - unanchors an URB
+ * @urb: pointer to the urb to anchor
+ *
+ * Call this to stop the system keeping track of this URB
+ */
+
+void usb_unanchor_urb(struct urb *urb)
+{
+   unsigned long flags;
+   struct usb_anchor *anchor;
+
+   if (!urb)
+   return;
+
+   anchor = urb-anchor;
+   if (!anchor)
+   return;
+
+   spin_lock_irqsave(anchor-lock, flags);
+   urb-anchor = NULL;
+   list_del(urb-anchor_list);
+   spin_unlock_irqrestore(anchor-lock, flags);
+

Re: [linux-usb-devel] anchors and their use

2007-05-22 Thread Alan Stern 
On Tue, 22 May 2007, Oliver Neukum wrote:

 Am Mittwoch, 16. Mai 2007 17:30 schrieb Alan Stern:
 
  This won't build because you forgot to export usb_unanchor_urb().
  
  You could make usb_init_anchor() non-inline.  I'm not sure where 
  tradeoff lies, but you might be over the limit.
  
  I don't like the way you combined the error handling in usb-skeleton 
  with the anchor stuff.  Better to keep the two things in separate 
  patches.
 
 Hi,
 
 I hope this version addresses your concerns

Very good.  I did notice two more things:

Timeouts are specified in jiffies, so your timeout argument
should be long or unsigned long, not int.

There's a race if two threads call usb_unanchor_urb() for the
same URB at the same time.  You need to recheck while holding 
the anchor's spinlock that urb-anchor is still equal to 
anchor.  You might also want to add

WARN_ON(urb-anchor != NULL);

to usb_anchor_urb(), just as a precaution.

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-22 Thread David Brownell 
On Tuesday 22 May 2007, Alan Stern wrote:

   Timeouts are specified in jiffies, so your timeout argument
   should be long or unsigned long, not int.

Timeouts are best specified in milliseconds though... that's been
the policy inside the USB stack since someone's search-and-destroy
mission a few years back.  Most people don't handle jiffies right.

- Dave

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-22 Thread Oliver Neukum 
Am Dienstag, 22. Mai 2007 21:48 schrieb David Brownell:
 On Tuesday 22 May 2007, Alan Stern wrote:
 
    Timeouts are specified in jiffies, so your timeout argument
    should be long or unsigned long, not int.
 
 Timeouts are best specified in milliseconds though... that's been
 the policy inside the USB stack since someone's search-and-destroy
 mission a few years back.  Most people don't handle jiffies right.

Very well. I'll go to millisecond specified as unsigned long.

Regards
Oliver
-- 
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)
This signature is a legal requirement

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-21 Thread Alan Stern 
On Mon, 21 May 2007, Oliver Neukum wrote:

  I don't like the way you combined the error handling in usb-skeleton 
  with the anchor stuff.  Better to keep the two things in separate 
  patches.
 
 Problems with the packaging or the code?

Just the packaging.

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] anchors and their use

2007-05-20 Thread Oliver Neukum 
Am Mittwoch, 16. Mai 2007 17:30 schrieb Alan Stern:
 On Wed, 16 May 2007, Oliver Neukum wrote:
 
  Hi Alan, list,
  
  here's the patch to introduce anchors for use with fire-and-forget
  techniques and a modification for the skeleton driver to use them.
  This should make implementing pre/post_reset and suspend/resume
  much easier.
 
 This won't build because you forgot to export usb_unanchor_urb().

%$% (censored) I'll redo the patch. It seems like I diffed an old version.

 You could make usb_init_anchor() non-inline.  I'm not sure where 
 tradeoff lies, but you might be over the limit.

I'll do test compilations.
 
 I don't like the way you combined the error handling in usb-skeleton 
 with the anchor stuff.  Better to keep the two things in separate 
 patches.

Problems with the packaging or the code?
 
 There's no need for the version of skel_draw_down() which acquires the
 mutex.  You use it only in skel_flush(), so you might as well acquire
 the mutex in the caller.

OK.

Regards
Oliver

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[linux-usb-devel] anchors and their use

2007-05-16 Thread Oliver Neukum 
Hi Alan, list,

here's the patch to introduce anchors for use with fire-and-forget
techniques and a modification for the skeleton driver to use them.
This should make implementing pre/post_reset and suspend/resume
much easier.

Regards
Oliver
-- 

--- a/include/linux/usb.h   2007-05-04 10:50:18.0 +0200
+++ b/include/linux/usb.h   2007-05-08 12:47:27.0 +0200
@@ -958,11 +958,26 @@ struct usb_iso_packet_descriptor {
 
 struct urb;
 
+struct usb_anchor {
+   struct list_head urb_list;
+   wait_queue_head_t wait;
+   spinlock_t lock;
+};
+
+static inline void init_usb_anchor(struct usb_anchor *anchor)
+{
+   INIT_LIST_HEAD(anchor-urb_list);
+   init_waitqueue_head(anchor-wait);
+   spin_lock_init(anchor-lock);
+}
+
 typedef void (*usb_complete_t)(struct urb *);
 
 /**
  * struct urb - USB Request Block
  * @urb_list: For use by current owner of the URB.
+ * @anchor_list: membership in the list of an anchor
+ * @anchor: to anchor URBs to a common mooring
  * @pipe: Holds endpoint number, direction, type, and more.
  * Create these values with the eight macros available;
  * usb_{snd,rcv}TYPEpipe(dev,endpoint), where the TYPE is ctrl
@@ -1135,6 +1150,8 @@ struct urb
/* public: documented fields in the urb that can be used by drivers */
struct list_head urb_list;  /* list head for use by the urb's
 * current owner */
+   struct list_head anchor_list;   /* the URB may be anchored by the 
driver */
+   struct usb_anchor *anchor;
struct usb_device *dev; /* (in) pointer to associated device */
unsigned int pipe;  /* (in) pipe information */
int status; /* (return) non-ISO status */
@@ -1270,6 +1287,10 @@ extern struct urb *usb_get_urb(struct ur
 extern int usb_submit_urb(struct urb *urb, gfp_t mem_flags);
 extern int usb_unlink_urb(struct urb *urb);
 extern void usb_kill_urb(struct urb *urb);
+extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
+extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
+extern void usb_unanchor_urb(struct urb *urb);
+extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor, int 
timeout);
 
 void *usb_buffer_alloc (struct usb_device *dev, size_t size,
gfp_t mem_flags, dma_addr_t *dma);
--- a/drivers/usb/core/hcd.c2007-05-04 12:04:29.0 +0200
+++ b/drivers/usb/core/hcd.c2007-05-07 15:19:01.0 +0200
@@ -1408,6 +1408,8 @@ void usb_hcd_giveback_urb (struct usb_hc
}
 
usbmon_urb_complete (hcd-self, urb);
+   usb_unanchor_urb(urb);
+
/* pass ownership to the completion handler */
urb-complete (urb);
atomic_dec (urb-use_count);
--- a/drivers/usb/core/urb.c2007-05-04 10:53:14.0 +0200
+++ b/drivers/usb/core/urb.c2007-05-16 11:32:15.0 +0200
@@ -4,6 +4,7 @@
 #include linux/slab.h
 #include linux/init.h
 #include linux/usb.h
+#include linux/wait.h
 #include hcd.h
 
 #define to_urb(d) container_of(d, struct urb, kref)
@@ -11,6 +12,7 @@
 static void urb_destroy(struct kref *kref)
 {
struct urb *urb = to_urb(kref);
+
kfree(urb);
 }
 
@@ -34,6 +36,7 @@ void usb_init_urb(struct urb *urb)
memset(urb, 0, sizeof(*urb));
kref_init(urb-kref);
spin_lock_init(urb-lock);
+   INIT_LIST_HEAD(urb-anchor_list);
}
 }
 
@@ -100,8 +103,55 @@ struct urb * usb_get_urb(struct urb *urb
kref_get(urb-kref);
return urb;
 }
-   
-   
+
+/**
+ * usb_anchor_urb - anchors an URB while it is processed
+ * @urb: pointer to the urb to anchor
+ * @anchor: pointer to the anchor
+ *
+ * This can be called to have access to URBs which are to be executed
+ * without bothering to track them
+ */
+
+void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(anchor-lock, flags);
+   usb_get_urb(urb);
+   list_add_tail(urb-anchor_list, anchor-urb_list);
+   urb-anchor = anchor;
+   spin_unlock_irqrestore(anchor-lock, flags);
+}
+
+/**
+ * usb_unanchor_urb - unanchors an URB
+ * @urb: pointer to the urb to anchor
+ *
+ * Call this to stop the system keeping track of this URB
+ */
+
+void usb_unanchor_urb(struct urb *urb)
+{
+   unsigned long flags;
+   struct usb_anchor *anchor;
+
+   if (!urb)
+   return;
+
+   anchor = urb-anchor;
+   if (!anchor)
+   return;
+
+   spin_lock_irqsave(anchor-lock, flags);
+   urb-anchor = NULL;
+   list_del(urb-anchor_list);
+   spin_unlock_irqrestore(anchor-lock, flags);
+   usb_put_urb(urb);
+   if (list_empty(anchor-urb_list))
+   wake_up(anchor-wait);
+}
+
 /*---*/
 
 /**
@@ -477,12 +527,56 @@ void

Re: [linux-usb-devel] anchors and their use

2007-05-16 Thread Alan Stern 
On Wed, 16 May 2007, Oliver Neukum wrote:

 Hi Alan, list,
 
 here's the patch to introduce anchors for use with fire-and-forget
 techniques and a modification for the skeleton driver to use them.
 This should make implementing pre/post_reset and suspend/resume
 much easier.

This won't build because you forgot to export usb_unanchor_urb().

You could make usb_init_anchor() non-inline.  I'm not sure where 
tradeoff lies, but you might be over the limit.

I don't like the way you combined the error handling in usb-skeleton 
with the anchor stuff.  Better to keep the two things in separate 
patches.

There's no need for the version of skel_draw_down() which acquires the
mutex.  You use it only in skel_flush(), so you might as well acquire
the mutex in the caller.

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel