I just posted the first two patches toward removing urb->status. It's
going to be a long road. :-)
An important step is to change the way URBs get linked and unlinked.
As suggested by David, I'm going to make the HCDs responsible for
adding and removing URBs from the endpoint's urb_list. The benefit is
that these activities will be protected by the HCD's private spinlock,
removing opportunities for odd sorts of races (e.g., usbcore thinks an
URB is linked and the HCD thinks it isn't).
This has led to a rather strange difficulty. As part of dequeuing an
URB, the HCD will have to verify that the URB is linked into the
endpoint's urb_list. However the endpoint is accessed via a pointer in
an array (udev->ep_in[] or udev->ep_out[]) indexed by the endpoint
number as stored in urb->pipe -- and when an endpoint is disabled the
pointer is set to NULL. This will make it impossible for
usb_hcd_endpoint_disable() to do its job! Once the endpoint has been
disabled, the unlink verification won't work.
I've got two proposed changes to help deal with this. The first is to
add an "enabled" flag to struct usb_host_endpoint. This simplifies the
testing a lot and is generally superior to the array entry approach
IMO.
The second is to change struct urb by adding a pointer to the
usb_host_endpoint. We have discussed this before; eventually urb->ep
should replace urb->pipe entirely. For now, until the drivers can be
changed we'll have to live with both fields. Still, it's a step in the
right direction.
Of particular interest is the new routine usb_control_urb_is_out().
The one piece of information present in a pipe but not an endpoint
descriptor is the direction of a control transfer. This routine
figures out the direction by looking at bRequestType and wLength in the
setup packet.
The patch below shows how all this will work. It adds some extra
things as well, utility routines for extracting an endpoint's transfer
type and number. The changes to usb_submit_urb() include a local
variable rename or two, so they aren't quite as bad as they may look.
Does anybody object to this?
Alan Stern
Index: usb-2.6/include/linux/usb.h
===================================================================
--- usb-2.6.orig/include/linux/usb.h
+++ usb-2.6/include/linux/usb.h
@@ -52,6 +52,7 @@ struct ep_device;
* @ep_dev: ep_device for sysfs info
* @extra: descriptors following this endpoint in the configuration
* @extralen: how many bytes of "extra" are valid
+ * @enabled: URBs may be submitted to this endpoint
*
* USB requests are always queued to a given endpoint, identified by a
* descriptor within an active interface in a given USB configuration.
@@ -64,6 +65,7 @@ struct usb_host_endpoint {
unsigned char *extra; /* Extra descriptors */
int extralen;
+ int enabled;
};
/* host-side wrapper for one interface setting's parsed descriptors */
@@ -553,6 +555,29 @@ static inline int usb_make_path (struct
/*-------------------------------------------------------------------------*/
/**
+ * usb_endpoint_num - get the endpoint's number
+ * @epd: endpoint to be checked
+ *
+ * Returns @epd's number: 0 to 15.
+ */
+static inline int usb_endpoint_num(const struct usb_endpoint_descriptor *epd)
+{
+ return epd->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
+}
+
+/**
+ * usb_endpoint_type - get the endpoint's transfer type
+ * @epd: endpoint to be checked
+ *
+ * Returns one of USB_ENDPOINT_XFER_{CONTROL, ISOC, BULK, INT} according
+ * to @epd's transfer type.
+ */
+static inline int usb_endpoint_type(const struct usb_endpoint_descriptor *epd)
+{
+ return epd->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
+}
+
+/**
* usb_endpoint_dir_in - check if the endpoint has IN direction
* @epd: endpoint to be checked
*
@@ -1019,6 +1044,8 @@ typedef void (*usb_complete_t)(struct ur
* @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
+ * @ep: Points to the endpoint's data structure. Will eventually
+ * replace @pipe.
* @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"
@@ -1194,6 +1221,7 @@ struct urb
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 */
+ struct usb_host_endpoint *ep; /* (out) pointer to endpoint struct */
unsigned int pipe; /* (in) pipe information */
int status; /* (return) non-ISO status */
unsigned int transfer_flags; /* (in) URB_SHORT_NOT_OK | ...*/
@@ -1334,6 +1362,25 @@ extern void usb_unanchor_urb(struct urb
extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
unsigned int timeout);
+/**
+ * usb_control_urb_is_out - determine the direction of a control URB
+ * @urb: pointer to the control URB to check
+ *
+ * Returns 1 if @urb describes an OUT transfer (host-to-device),
+ * otherwise 0.
+ *
+ * @urb->pipe is not consulted. Instead the direction is determined by
+ * the direction bit in the setup packet's bRequestType field together
+ * with the wLength value (0 length always implies OUT).
+ */
+static inline int usb_control_urb_is_out(struct urb *urb)
+{
+ struct usb_ctrlrequest *setup =
+ (struct usb_ctrlrequest *) urb->setup_packet;
+
+ return !(setup->bRequestType & USB_DIR_IN) || !setup->wLength;
+}
+
void *usb_buffer_alloc (struct usb_device *dev, size_t size,
gfp_t mem_flags, dma_addr_t *dma);
void usb_buffer_free (struct usb_device *dev, size_t size,
Index: usb-2.6/drivers/usb/core/urb.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/urb.c
+++ usb-2.6/drivers/usb/core/urb.c
@@ -277,9 +277,10 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb);
*/
int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
{
- int pipe, temp, max;
- struct usb_device *dev;
- int is_out;
+ int xfertype, max;
+ struct usb_device *dev;
+ struct usb_host_endpoint *ep;
+ int is_out;
if (!urb || urb->hcpriv || !urb->complete)
return -EINVAL;
@@ -291,30 +292,40 @@ int usb_submit_urb(struct urb *urb, gfp_
|| dev->state == USB_STATE_SUSPENDED)
return -EHOSTUNREACH;
+ /* For now, get the endpoint from the pipe. Eventually drivers
+ * will be required to set urb->ep directly and we can eliminate
+ * urb->pipe.
+ */
+ ep = (usb_pipein(urb->pipe) ? dev->ep_in : dev->ep_out)
+ [usb_pipeendpoint(urb->pipe)];
+ if (!ep)
+ return -ENOENT;
+
+ urb->ep = ep;
urb->status = -EINPROGRESS;
urb->actual_length = 0;
/* Lots of sanity checks, so HCDs can rely on clean data
* and don't need to duplicate tests
*/
- pipe = urb->pipe;
- temp = usb_pipetype(pipe);
- is_out = usb_pipeout(pipe);
+ xfertype = usb_endpoint_type(&ep->desc);
+ if (xfertype == USB_ENDPOINT_XFER_CONTROL) {
+ if (!urb->setup_packet)
+ return -ENOEXEC;
+ is_out = usb_control_urb_is_out(urb);
+ } else {
+ is_out = usb_endpoint_dir_out(&ep->desc);
+ }
- if (!usb_pipecontrol(pipe) && dev->state < USB_STATE_CONFIGURED)
+ if (xfertype != USB_ENDPOINT_XFER_CONTROL &&
+ dev->state < USB_STATE_CONFIGURED)
return -ENODEV;
- /* FIXME there should be a sharable lock protecting us against
- * config/altsetting changes and disconnects, kicking in here.
- * (here == before maxpacket, and eventually endpoint type,
- * checks get made.)
- */
-
- max = usb_maxpacket(dev, pipe, is_out);
+ max = le16_to_cpu(ep->desc.wMaxPacketSize);
if (max <= 0) {
dev_dbg(&dev->dev,
"bogus endpoint ep%d%s in %s (bad maxpacket %d)\n",
- usb_pipeendpoint(pipe), is_out ? "out" : "in",
+ usb_endpoint_num(&ep->desc), is_out ? "out" : "in",
__FUNCTION__, max);
return -EMSGSIZE;
}
@@ -323,7 +334,7 @@ int usb_submit_urb(struct urb *urb, gfp_
* but drivers only control those sizes for ISO.
* while we're checking, initialize return status.
*/
- if (temp == PIPE_ISOCHRONOUS) {
+ if (xfertype == USB_ENDPOINT_XFER_ISOC) {
int n, len;
/* "high bandwidth" mode, 1-3 packets/uframe? */
@@ -359,19 +370,19 @@ int usb_submit_urb(struct urb *urb, gfp_
/* enforce simple/standard policy */
allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_SETUP_DMA_MAP |
URB_NO_INTERRUPT);
- switch (temp) {
- case PIPE_BULK:
+ switch (xfertype) {
+ case USB_ENDPOINT_XFER_BULK:
if (is_out)
allowed |= URB_ZERO_PACKET;
/* FALLTHROUGH */
- case PIPE_CONTROL:
+ case USB_ENDPOINT_XFER_CONTROL:
allowed |= URB_NO_FSBR; /* only affects UHCI */
/* FALLTHROUGH */
default: /* all non-iso endpoints */
if (!is_out)
allowed |= URB_SHORT_NOT_OK;
break;
- case PIPE_ISOCHRONOUS:
+ case USB_ENDPOINT_XFER_ISOC:
allowed |= URB_ISO_ASAP;
break;
}
@@ -393,9 +404,9 @@ int usb_submit_urb(struct urb *urb, gfp_
* supports different values... this uses EHCI/UHCI defaults (and
* EHCI can use smaller non-default values).
*/
- switch (temp) {
- case PIPE_ISOCHRONOUS:
- case PIPE_INTERRUPT:
+ switch (xfertype) {
+ case USB_ENDPOINT_XFER_ISOC:
+ case USB_ENDPOINT_XFER_INT:
/* too small? */
if (urb->interval <= 0)
return -EINVAL;
@@ -405,29 +416,29 @@ int usb_submit_urb(struct urb *urb, gfp_
// NOTE usb handles 2^15
if (urb->interval > (1024 * 8))
urb->interval = 1024 * 8;
- temp = 1024 * 8;
+ max = 1024 * 8;
break;
case USB_SPEED_FULL: /* units are frames/msec */
case USB_SPEED_LOW:
- if (temp == PIPE_INTERRUPT) {
+ if (xfertype == USB_ENDPOINT_XFER_INT) {
if (urb->interval > 255)
return -EINVAL;
// NOTE ohci only handles up to 32
- temp = 128;
+ max = 128;
} else {
if (urb->interval > 1024)
urb->interval = 1024;
// NOTE usb and ohci handle up to 2^15
- temp = 1024;
+ max = 1024;
}
break;
default:
return -EINVAL;
}
/* power of two? */
- while (temp > urb->interval)
- temp >>= 1;
- urb->interval = temp;
+ while (max > urb->interval)
+ max >>= 1;
+ urb->interval = max;
}
return usb_hcd_submit_urb(urb, mem_flags);
Index: usb-2.6/drivers/usb/core/hcd.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hcd.c
+++ usb-2.6/drivers/usb/core/hcd.c
@@ -943,7 +943,6 @@ int usb_hcd_submit_urb (struct urb *urb,
{
int status;
struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
- struct usb_host_endpoint *ep;
unsigned long flags;
if (!hcd)
@@ -960,16 +959,14 @@ int usb_hcd_submit_urb (struct urb *urb,
// FIXME: verify that quiescing hc works right (RH cleans up)
spin_lock_irqsave(&hcd_urb_list_lock, flags);
- ep = (usb_pipein(urb->pipe) ? urb->dev->ep_in : urb->dev->ep_out)
- [usb_pipeendpoint(urb->pipe)];
- if (unlikely (!ep))
+ if (unlikely(!urb->ep->enabled))
status = -ENOENT;
else if (unlikely (urb->reject))
status = -EPERM;
else switch (hcd->state) {
case HC_STATE_RUNNING:
case HC_STATE_RESUMING:
- list_add_tail (&urb->urb_list, &ep->urb_list);
+ list_add_tail (&urb->urb_list, &urb->ep->urb_list);
status = 0;
break;
default:
@@ -1022,7 +1019,7 @@ int usb_hcd_submit_urb (struct urb *urb,
: DMA_TO_DEVICE);
}
- status = hcd->driver->urb_enqueue (hcd, ep, urb, mem_flags);
+ status = hcd->driver->urb_enqueue (hcd, urb->ep, urb, mem_flags);
done:
if (unlikely (status)) {
urb_unlink(hcd, urb);
@@ -1071,7 +1068,6 @@ unlink1 (struct usb_hcd *hcd, struct urb
*/
int usb_hcd_unlink_urb (struct urb *urb, int status)
{
- struct usb_host_endpoint *ep;
struct usb_hcd *hcd = NULL;
struct device *sys = NULL;
unsigned long flags;
@@ -1082,10 +1078,6 @@ int usb_hcd_unlink_urb (struct urb *urb,
return -EINVAL;
if (!urb->dev || !urb->dev->bus)
return -ENODEV;
- ep = (usb_pipein(urb->pipe) ? urb->dev->ep_in : urb->dev->ep_out)
- [usb_pipeendpoint(urb->pipe)];
- if (!ep)
- return -ENODEV;
/*
* we contend for urb->status with the hcd core,
@@ -1109,7 +1101,7 @@ int usb_hcd_unlink_urb (struct urb *urb,
}
/* insist the urb is still queued */
- list_for_each(tmp, &ep->urb_list) {
+ list_for_each(tmp, &urb->ep->urb_list) {
if (tmp == &urb->urb_list)
break;
}
Index: usb-2.6/drivers/usb/core/message.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/message.c
+++ usb-2.6/drivers/usb/core/message.c
@@ -998,8 +998,10 @@ void usb_disable_endpoint(struct usb_dev
ep = dev->ep_in[epnum];
dev->ep_in[epnum] = NULL;
}
- if (ep && dev->bus)
+ if (ep) {
+ ep->enabled = 0;
usb_hcd_endpoint_disable(dev, ep);
+ }
}
/**
@@ -1098,6 +1100,7 @@ usb_enable_endpoint(struct usb_device *d
usb_settoggle(dev, epnum, 0, 0);
dev->ep_in[epnum] = ep;
}
+ ep->enabled = 1;
}
/*
-------------------------------------------------------------------------
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
