Thanks for reviewing. My responses are inline.
Greg has asked that we clean up this code internally before we
send out another patchset to the mailing list. I will address
the issues you pointed out, but it may be a while before you see
another patchset.
Thanks Again,
Sean
On Tue, Nov 11, 2014 at 10:54:30AM -0500, Alan Stern wrote:
> 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?
>
Our plan to support multiple MA devices is to have them all connected
to the same virtual host controller, so only 1 would be needed.
Would you prefer we have 1 host controller instance per MA device?
We are definitely open to suggestions on how this should be architected.
> > +/**
> > + * @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?
>
We will simplify the locking scheme before resubmitting.
I think it might be worthwhile to have a per-endpoint lock, see below.
> > + 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?
>
This works with our multi-lock scheme, but I will fix when we move to 1 lock.
> > +
> > + /* 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.
>
I will remove this check (along with any other unnecessary checks for things
guaranteed from usbcore).
> > + 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.
>
Will remove.
> > + /* 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.
>
Will fix this & make sure we hold our lock.
> > + 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!
>
If we get rid of these locks, endpoints can't run simultaneously.
MA USB IN endpoints have to copy data, which could take a while.
Couldn't this cause a bottleneck?
> > + /* 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.
>
Will remove.
> > + 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.
Will grep & remove all debugging messages before we release the next patchset.
>
> That's enough for now. Obviously there are a lot of issues in this
> driver which need to be fixed up.
We will try to address all the obvious issues before submitting again.
>
> Alan Stern
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
