Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver

[Alan Stern]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/