David Brownell wrote:
> As was discussed a few weeks back, this moves most of the
> sanity checks and input conditioning for the HCD framework's
> usb_submit_urb() support directly into usb_submit_urb(), so
> that all HCDs (not just those using the sharable HCD framework
> support) can rely on them.
>
Hi,
even though this seems to be a good thing to do I think
this puts a lot of overhead into the submition path especially for
embedded USB hosts. A 50MHz CPU will need a significant
amount of time to pass those checks. For example a MPC823
needs about 30 to 50 % of its CPU-time to manage its
USB host controller and the checks probably adds some other percents.
Also the checks are only good for bug finding. If all bugs have been
found during development then we don't need the checks during ordinary
runs.
Or a more pragmatic approach: Let us find the bugs on fast
CPU / low USB overhead systems and don't bother embedded HCDs
with those checks.
Another remark:
As far as I can remember it is legal to transfer more than
maxpacketsize bytes (= more than one packet) at
interrupt transfers.
So
> + case PIPE_INTERRUPT:
> + if (urb->transfer_buffer_length > max)
> return -EMSGSIZE;
seems to be wrong.
- Roman
> Please merge to Linus' tree.
>
> - Dave
>
>
> ------------------------------------------------------------------------
>
> --- ./drivers/usb-dist/core/hcd.c Mon Jun 3 11:11:11 2002
> +++ ./drivers/usb/core/hcd.c Thu Jun 6 19:32:13 2002
> @@ -1337,170 +1337,21 @@
> }
>
>
> -/* may be called in any context with a valid urb->dev usecount */
> -/* caller surrenders "ownership" of urb */
> -
> +/* may be called in any context with a valid urb->dev usecount
> + * caller surrenders "ownership" of urb
> + * expects usb_submit_urb() to have sanity checked and conditioned all
> + * inputs in the urb
> + */
> static int hcd_submit_urb (struct urb *urb, int mem_flags)
> {
> int status;
> - struct usb_hcd *hcd;
> - struct hcd_dev *dev;
> + struct usb_hcd *hcd = urb->dev->bus->hcpriv;
> + struct hcd_dev *dev = urb->dev->hcpriv;
> unsigned long flags;
> - int pipe, temp, max;
> -
> - if (!urb || urb->hcpriv || !urb->complete)
> - return -EINVAL;
> -
> - urb->status = -EINPROGRESS;
> - urb->actual_length = 0;
> - urb->bandwidth = 0;
> - INIT_LIST_HEAD (&urb->urb_list);
>
> - if (!urb->dev || !urb->dev->bus || urb->dev->devnum <= 0)
> - return -ENODEV;
> - hcd = urb->dev->bus->hcpriv;
> - dev = urb->dev->hcpriv;
> if (!hcd || !dev)
> return -ENODEV;
>
> - /* can't submit new urbs when quiescing, halted, ... */
> - if (hcd->state == USB_STATE_QUIESCING || !HCD_IS_RUNNING (hcd->state))
> - return -ESHUTDOWN;
> - pipe = urb->pipe;
> - temp = usb_pipetype (urb->pipe);
> - if (usb_endpoint_halted (urb->dev, usb_pipeendpoint (pipe),
> - usb_pipeout (pipe)))
> - return -EPIPE;
> -
> - /* FIXME there should be a sharable lock protecting us against
> - * config/altsetting changes and disconnects, kicking in here.
> - */
> -
> - /* Sanity check, so HCDs can rely on clean data */
> - max = usb_maxpacket (urb->dev, pipe, usb_pipeout (pipe));
> - if (max <= 0) {
> - err ("bogus endpoint (bad maxpacket)");
> - return -EINVAL;
> - }
> -
> - /* "high bandwidth" mode, 1-3 packets/uframe? */
> - if (urb->dev->speed == USB_SPEED_HIGH) {
> - int mult;
> - switch (temp) {
> - case PIPE_ISOCHRONOUS:
> - case PIPE_INTERRUPT:
> - mult = 1 + ((max >> 11) & 0x03);
> - max &= 0x03ff;
> - max *= mult;
> - }
> - }
> -
> - /* periodic transfers limit size per frame/uframe */
> - switch (temp) {
> - case PIPE_ISOCHRONOUS: {
> - int n, len;
> -
> - if (urb->number_of_packets <= 0)
> - return -EINVAL;
> - for (n = 0; n < urb->number_of_packets; n++) {
> - len = urb->iso_frame_desc [n].length;
> - if (len < 0 || len > max)
> - return -EINVAL;
> - }
> -
> - }
> - break;
> - case PIPE_INTERRUPT:
> - if (urb->transfer_buffer_length > max)
> - return -EINVAL;
> - }
> -
> - /* the I/O buffer must usually be mapped/unmapped */
> - if (urb->transfer_buffer_length < 0)
> - return -EINVAL;
> -
> -#ifdef DEBUG
> - /* stuff that drivers shouldn't do, but which shouldn't
> - * cause problems in HCDs if they get it wrong.
> - */
> - {
> - unsigned int orig_flags = urb->transfer_flags;
> - unsigned int allowed;
> -
> - /* enforce simple/standard policy */
> - allowed = USB_ASYNC_UNLINK; // affects later unlinks
> - allowed |= USB_NO_FSBR; // only affects UHCI
> - switch (temp) {
> - case PIPE_CONTROL:
> - allowed |= USB_DISABLE_SPD;
> - break;
> - case PIPE_BULK:
> - allowed |= USB_DISABLE_SPD | USB_QUEUE_BULK
> - | USB_ZERO_PACKET | URB_NO_INTERRUPT;
> - break;
> - case PIPE_INTERRUPT:
> - allowed |= USB_DISABLE_SPD;
> - break;
> - case PIPE_ISOCHRONOUS:
> - allowed |= USB_ISO_ASAP;
> - break;
> - }
> - urb->transfer_flags &= allowed;
> -
> - /* fail if submitter gave bogus flags */
> - if (urb->transfer_flags != orig_flags) {
> - err ("BOGUS urb flags, %x --> %x",
> - orig_flags, urb->transfer_flags);
> - return -EINVAL;
> - }
> - }
> -#endif
> - /*
> - * Force periodic transfer intervals to be legal values that are
> - * a power of two (so HCDs don't need to).
> - *
> - * FIXME want bus->{intr,iso}_sched_horizon values here. Each HC
> - * supports different values... this uses EHCI/UHCI defaults (and
> - * EHCI can use smaller non-default values).
> - */
> - switch (temp) {
> - case PIPE_ISOCHRONOUS:
> - case PIPE_INTERRUPT:
> - /* too small? */
> - if (urb->interval <= 0)
> - return -EINVAL;
> - /* too big? */
> - switch (urb->dev->speed) {
> - case USB_SPEED_HIGH: /* units are microframes */
> - // NOTE usb handles 2^15
> - if (urb->interval > (1024 * 8))
> - urb->interval = 1024 * 8;
> - temp = 1024 * 8;
> - break;
> - case USB_SPEED_FULL: /* units are frames/msec */
> - case USB_SPEED_LOW:
> - if (temp == PIPE_INTERRUPT) {
> - if (urb->interval > 255)
> - return -EINVAL;
> - // NOTE ohci only handles up to 32
> - temp = 128;
> - } else {
> - if (urb->interval > 1024)
> - urb->interval = 1024;
> - // NOTE usb and ohci handle up to 2^15
> - temp = 1024;
> - }
> - break;
> - default:
> - return -EINVAL;
> - }
> - /* power of two? */
> - while (temp > urb->interval)
> - temp >>= 1;
> - urb->interval = temp;
> - }
> -
> -
> /*
> * FIXME: make urb timeouts be generic, keeping the HCD cores
> * as simple as possible.
> @@ -1510,9 +1361,6 @@
> // hcd_monitor_hook(MONITOR_URB_SUBMIT, urb)
> // It would catch submission paths for all urbs.
>
> - /* increment urb's reference count, we now control it. */
> - urb = usb_get_urb(urb);
> -
> /*
> * Atomically queue the urb, first to our records, then to the HCD.
> * Access to urb->status is controlled by urb->lock ... changes on
> @@ -1534,18 +1382,15 @@
> if (status)
> return status;
>
> - /* temporarily up refcount while queueing it in the HCD,
> - * since we report some queuing/setup errors ourselves
> + /* increment urb's reference count as part of giving it to the HCD
> + * (which now controls it). HCD guarantees that it either returns
> + * an error or calls giveback(), but not both.
> */
> urb = usb_get_urb (urb);
> if (urb->dev == hcd->self.root_hub)
> status = rh_urb_enqueue (hcd, urb);
> else
> status = hcd->driver->urb_enqueue (hcd, urb, mem_flags);
> - /* urb->dev got nulled if hcd called giveback for us */
> - if (status && urb->dev)
> - urb_unlink (urb);
> - usb_put_urb (urb);
> return status;
> }
>
> --- ./drivers/usb-dist/core/urb.c Mon Jun 3 11:11:11 2002
> +++ ./drivers/usb/core/urb.c Thu Jun 6 19:32:13 2002
> @@ -182,15 +182,170 @@
> */
> int usb_submit_urb(struct urb *urb, int mem_flags)
> {
> + int pipe, temp, max;
> + struct usb_device *dev;
> + struct usb_operations *op;
> + int is_out;
>
> - if (urb && urb->dev && urb->dev->bus && urb->dev->bus->op) {
> - if (usb_maxpacket(urb->dev, urb->pipe, usb_pipeout(urb->pipe)) <= 0) {
> - err("%s: pipe %x has invalid size (<= 0)", __FUNCTION__,
>urb->pipe);
> + if (!urb || urb->hcpriv || !urb->complete)
> + return -EINVAL;
> + if (!(dev = urb->dev) || !dev->bus || dev->devnum <= 0)
> + return -ENODEV;
> + if (!(op = dev->bus->op) || !op->submit_urb)
> + return -ENODEV;
> +
> + urb->status = -EINPROGRESS;
> + urb->actual_length = 0;
> + urb->bandwidth = 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);
> +
> + /* (actually HCDs may need to duplicate this, endpoint might yet
> + * stall due to queued bulk/intr transactions that complete after
> + * we check)
> + */
> + if (usb_endpoint_halted (dev, usb_pipeendpoint (pipe), is_out))
> + return -EPIPE;
> +
> + /* 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);
> + if (max <= 0) {
> + dbg ("%s: bogus endpoint %d-%s on usb-%s-%s (bad maxpacket %d)",
> + __FUNCTION__,
> + usb_pipeendpoint (pipe), is_out ? "OUT" : "IN",
> + dev->bus->bus_name, dev->devpath,
> + max);
> + return -EMSGSIZE;
> + }
> +
> + /* "high bandwidth" mode, 1-3 packets/uframe? */
> + if (dev->speed == USB_SPEED_HIGH) {
> + int mult;
> + switch (temp) {
> + case PIPE_ISOCHRONOUS:
> + case PIPE_INTERRUPT:
> + mult = 1 + ((max >> 11) & 0x03);
> + max &= 0x03ff;
> + max *= mult;
> + }
> + }
> +
> + /* periodic transfers limit size per frame/uframe */
> + switch (temp) {
> + case PIPE_ISOCHRONOUS: {
> + int n, len;
> +
> + if (urb->number_of_packets <= 0)
> + return -EINVAL;
> + for (n = 0; n < urb->number_of_packets; n++) {
> + len = urb->iso_frame_desc [n].length;
> + if (len < 0 || len > max)
> + return -EMSGSIZE;
> + }
> +
> + }
> + break;
> + case PIPE_INTERRUPT:
> + if (urb->transfer_buffer_length > max)
> return -EMSGSIZE;
> + }
> +
> + /* the I/O buffer must be mapped/unmapped, except when length=0 */
> + if (urb->transfer_buffer_length < 0)
> + return -EMSGSIZE;
> +
> +#ifdef DEBUG
> + /* stuff that drivers shouldn't do, but which shouldn't
> + * cause problems in HCDs if they get it wrong.
> + */
> + {
> + unsigned int orig_flags = urb->transfer_flags;
> + unsigned int allowed;
> +
> + /* enforce simple/standard policy */
> + allowed = USB_ASYNC_UNLINK; // affects later unlinks
> + allowed |= USB_NO_FSBR; // only affects UHCI
> + switch (temp) {
> + case PIPE_CONTROL:
> + allowed |= USB_DISABLE_SPD;
> + break;
> + case PIPE_BULK:
> + allowed |= USB_DISABLE_SPD | USB_QUEUE_BULK
> + | USB_ZERO_PACKET | URB_NO_INTERRUPT;
> + break;
> + case PIPE_INTERRUPT:
> + allowed |= USB_DISABLE_SPD;
> + break;
> + case PIPE_ISOCHRONOUS:
> + allowed |= USB_ISO_ASAP;
> + break;
> + }
> + urb->transfer_flags &= allowed;
> +
> + /* fail if submitter gave bogus flags */
> + if (urb->transfer_flags != orig_flags) {
> + err ("BOGUS urb flags, %x --> %x",
> + orig_flags, urb->transfer_flags);
> + return -EINVAL;
> + }
> + }
> +#endif
> + /*
> + * Force periodic transfer intervals to be legal values that are
> + * a power of two (so HCDs don't need to).
> + *
> + * FIXME want bus->{intr,iso}_sched_horizon values here. Each HC
> + * supports different values... this uses EHCI/UHCI defaults (and
> + * EHCI can use smaller non-default values).
> + */
> + switch (temp) {
> + case PIPE_ISOCHRONOUS:
> + case PIPE_INTERRUPT:
> + /* too small? */
> + if (urb->interval <= 0)
> + return -EINVAL;
> + /* too big? */
> + switch (dev->speed) {
> + case USB_SPEED_HIGH: /* units are microframes */
> + // NOTE usb handles 2^15
> + if (urb->interval > (1024 * 8))
> + urb->interval = 1024 * 8;
> + temp = 1024 * 8;
> + break;
> + case USB_SPEED_FULL: /* units are frames/msec */
> + case USB_SPEED_LOW:
> + if (temp == PIPE_INTERRUPT) {
> + if (urb->interval > 255)
> + return -EINVAL;
> + // NOTE ohci only handles up to 32
> + temp = 128;
> + } else {
> + if (urb->interval > 1024)
> + urb->interval = 1024;
> + // NOTE usb and ohci handle up to 2^15
> + temp = 1024;
> + }
> + break;
> + default:
> + return -EINVAL;
> }
> - return urb->dev->bus->op->submit_urb(urb, mem_flags);
> + /* power of two? */
> + while (temp > urb->interval)
> + temp >>= 1;
> + urb->interval = temp;
> }
> - return -ENODEV;
> +
> + return op->submit_urb (urb, mem_flags);
> }
>
> /*-------------------------------------------------------------------*/
>
_______________________________________________________________
Don't miss the 2002 Sprint PCS Application Developer's Conference
August 25-28 in Las Vegas -- http://devcon.sprintpcs.com/adp/index.cfm
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
