On Mon, 10 Nov 2014, Stephanie Wallick wrote:
> +static struct mausb_hcd mhcd;
Only one statically-allocated structure? What if somebody wants to
have more than one of these things in their system?
> +/**
> + * @maurb: Media agnostic structure with URB to release.
> + * @status: Status for URB that is getting released.
> + *
> + * Removes an URB from the queue, deletes the media agnostic information in
> + * the urb, and gives the URB back to the HCD. Caller must be holding the
> + * driver's spinlock.
> + */
> +void mausb_unlink_giveback_urb(struct mausb_urb *maurb, int status)
> +{
> + struct urb *urb;
> + struct usb_hcd *hcd;
> + struct api_context *ctx = NULL;
> + unsigned long irq_flags;
> +
> + hcd = mausb_hcd_to_usb_hcd(&mhcd);
> +
> + spin_lock_irqsave(&mhcd.giveback_lock, irq_flags);
Why do you need multiple spinlocks? Isn't one lock sufficient?
> + if (!maurb) {
> + mausb_err(&mhcd, "%s: no maurb\n", __func__);
> + spin_unlock_irqrestore(&mhcd.giveback_lock, irq_flags);
> + return;
> + } else {
> + urb = maurb->urb;
> + ctx = urb->context;
> + }
> +
> + if (!urb) {
> + mausb_err(&mhcd, "%s: no urb\n", __func__);
> + mausb_internal_drop_maurb(maurb, &mhcd);
> + spin_unlock_irqrestore(&mhcd.giveback_lock, irq_flags);
> + return;
> + }
> +
> + mausb_dbg(&mhcd, "%s: returning urb with status %i\n", __func__, status);
> +
> + usb_hcd_unlink_urb_from_ep(hcd, urb);
> + usb_hcd_giveback_urb(hcd, urb, status);
You must not call this function while holding any spinlocks. What happens
if the URB's completion routine tries to resubmit?
> +
> + /* remove the mausb-specific data */
> + mausb_internal_drop_maurb(maurb, &mhcd);
> +
> + spin_unlock_irqrestore(&mhcd.giveback_lock, irq_flags);
> +}
> +
> +/**
> + * Adds an URB to the endpoint queue then calls the URB handler. URB is wrapped
> + * in media agnostic structure before being enqueued.
> + */
> +static int mausb_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
> + gfp_t memflags)
> +{
> + int ret = 0;
> + struct mausb_urb *maurb;
> + struct mausb_host_ep *ep;
> + unsigned long irq_flags;
> +
> + if (!hcd || !urb) {
> + pr_err("%s: no %s\n", __func__, (hcd ? "urb" : "USB hcd"));
> + }
This can never happen. The USB core guarantees it; you don't need
to check.
> + ep = usb_to_ma_endpoint(urb->ep);
> +
> + if (!ep) {
> + mausb_err(&mhcd, "%s: no endpoint\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (urb->status != -EINPROGRESS) {
> + mausb_err(&mhcd, "%s: urb already unlinked, status is %i\n",
> + __func__, urb->status);
> + return urb->status;
> + }
You also don't need to check this.
> + /* If the endpoint isn't activated, we can't enqueue anything. */
> + if (MAUSB_EP_HANDLE_UNASSIGNED == ep->ep_handle_state) {
> + mausb_err(&mhcd, "%s: endpoint handle unassigned\n", __func__);
> + return -EPIPE;
> + }
> +
> + if (USB_SPEED_FULL != urb->dev->speed) /* suppress checks */
> + ep->max_pkt = usb_endpoint_maxp(&urb->ep->desc);
What happens to full-speed devices? Don't they have maxpacket values?
> +
> + /* initialize the maurb */
> + maurb = mausb_alloc_maurb(ep, memflags);
> + if (!maurb) {
> + mausb_err(&mhcd, "could not allocate memory for MA USB urb\n");
> + return -ENOMEM;
> + }
> +
> + /* set maurb member values */
> + maurb->urb = urb;
> + urb->hcpriv = maurb;
> +
> + /* submit urb to hcd and add to endpoint queue */
> + ret = usb_hcd_link_urb_to_ep(hcd, urb);
Read the kerneldoc for this function. You must hold your private
spinlock when you call it.
> + if (ret < 0) {
> + mausb_err(&mhcd, "urb enqueue failed: error %d\n", ret);
> + usb_hcd_unlink_urb_from_ep(hcd, urb);
> + return ret;
> + }
> +
> + /* get usb device and increment reference counter */
> + if (!mhcd.udev) {
> + mhcd.udev = urb->dev;
> + usb_get_dev(mhcd.udev);
> + }
What happens if more than one device is in use at a time?
> +
> + /* add urb to queue list */
> + spin_lock_irqsave(&ep->ep_lock, irq_flags);
> + list_add_tail(&maurb->urb_list, &ep->urb_list);
> + spin_unlock_irqrestore(&ep->ep_lock, irq_flags);
Yet another class of spinlocks!
> + /* add urb to ma hcd urb list */
> + spin_lock_irqsave(&mhcd.urb_list_lock, irq_flags);
And another! You really shouldn't need more than one lock.
> + list_add_tail(&maurb->ma_hcd_urb_list, &mhcd.enqueue_urb_list);
> + spin_unlock_irqrestore(&mhcd.urb_list_lock, irq_flags);
> +
> + /* send to MA transfer process */
> + wake_up(&mhcd.waitq);
> +
> + return ret;
> +}
> +
> +/**
> + * Dequeues an URB.
> + */
> +static int mausb_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> +{
> + int ret = 0;
> + struct mausb_host_ep *ep = usb_to_ma_endpoint(urb->ep);
> + struct mausb_urb *maurb = usb_urb_to_mausb_urb(urb);
> + unsigned long irq_flags;
> +
> + /* For debugging - we want to know who initiated URB dequeue. */
> + dump_stack();
Debugging things like this should be removed before a patch is submitted.
That's enough for now. Obviously there are a lot of issues in this
driver which need to be fixed up.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
