Patchwork [-mm,1/3] USB: FHCI: Driver should be responsible for managing endpoint queues

login
register
mail settings
Submitter Anton Vorontsov
Date Dec. 24, 2008, 7:13 p.m.
Message ID <20081224191303.GA21815@oksana.dev.rtsoft.ru>
Download mbox | patch
Permalink /patch/15533/
State Superseded, archived
Delegated to: Kumar Gala
Headers show

Comments

Anton Vorontsov - Dec. 24, 2008, 7:13 p.m.
Follow these changes for the FHCI driver:

commit e9df41c5c5899259541dc928872cad4d07b82076
Author: Alan Stern <stern@rowland.harvard.edu>
Date:   Wed Aug 8 11:48:02 2007 -0400

USB: make HCDs responsible for managing endpoint queues

This patch (as954) implements a suggestion of David Brownell's.  Now
the host controller drivers are responsible for linking and unlinking
URBs to/from their endpoint queues.  This eliminates the possiblity of
strange situations where usbcore thinks an URB is linked but the HCD
thinks it isn't.  It also means HCDs no longer have to check for URBs
being dequeued before they were fully enqueued.

In addition to the core changes, this requires changing every host
controller driver and the root-hub URB handler.  For the most part the
required changes are fairly small; drivers have to call
usb_hcd_link_urb_to_ep() in their urb_enqueue method,
usb_hcd_check_unlink_urb() in their urb_dequeue method, and
usb_hcd_unlink_urb_from_ep() before giving URBs back.  A few HCDs make
matters more complicated by the way they split up the flow of control.

In addition some method interfaces get changed.  The endpoint argument
for urb_enqueue is now redundant so it is removed.  The unlink status
is required by usb_hcd_check_unlink_urb(), so it has been added to
urb_dequeue.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/usb/host/fhci-hcd.c |   24 ++++++++++++++++++++----
 drivers/usb/host/fhci-q.c   |    3 +++
 2 files changed, 23 insertions(+), 4 deletions(-)
Greg KH - Dec. 24, 2008, 7:59 p.m.
On Wed, Dec 24, 2008 at 10:13:03PM +0300, Anton Vorontsov wrote:
> Follow these changes for the FHCI driver:

Hm, would it just make more sense to respin the whole driver?  That way
it's never in the tree in a "broken" state?

thanks,

greg k-h
Anton Vorontsov - Dec. 24, 2008, 8:08 p.m.
On Wed, Dec 24, 2008 at 11:59:36AM -0800, Greg KH wrote:
> On Wed, Dec 24, 2008 at 10:13:03PM +0300, Anton Vorontsov wrote:
> > Follow these changes for the FHCI driver:
> 
> Hm, would it just make more sense to respin the whole driver?  That way
> it's never in the tree in a "broken" state?

It's for -mm tree. If I understand -mm workflow correctly, Andrew
will merge all the fixes into the final patch before sending it to
you in late -rc0 phase, when various -next git trees (on which this
driver depend) will be merged into the Linus tree.

Andrew, am I correct?


Thanks,
Andrew Morton - Dec. 24, 2008, 8:18 p.m.
On Wed, 24 Dec 2008 23:08:45 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> On Wed, Dec 24, 2008 at 11:59:36AM -0800, Greg KH wrote:
> > On Wed, Dec 24, 2008 at 10:13:03PM +0300, Anton Vorontsov wrote:
> > > Follow these changes for the FHCI driver:
> > 
> > Hm, would it just make more sense to respin the whole driver?  That way
> > it's never in the tree in a "broken" state?
> 
> It's for -mm tree. If I understand -mm workflow correctly, Andrew
> will merge all the fixes into the final patch before sending it to
> you in late -rc0 phase, when various -next git trees (on which this
> driver depend) will be merged into the Linus tree.
>
> Andrew, am I correct?

Yes, that's what I do.

I prefer to see the incremental patches once a patch has had review or
testing, so we can see which bits changed.  But if Greg intends to
merge this driver then he'd probably prefer a fresh new patch,
particularly as a) version 1 was only sent yesterday and b) he hasn't
merged version #1.
Greg KH - Dec. 24, 2008, 8:58 p.m.
On Wed, Dec 24, 2008 at 12:18:53PM -0800, Andrew Morton wrote:
> On Wed, 24 Dec 2008 23:08:45 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > On Wed, Dec 24, 2008 at 11:59:36AM -0800, Greg KH wrote:
> > > On Wed, Dec 24, 2008 at 10:13:03PM +0300, Anton Vorontsov wrote:
> > > > Follow these changes for the FHCI driver:
> > > 
> > > Hm, would it just make more sense to respin the whole driver?  That way
> > > it's never in the tree in a "broken" state?
> > 
> > It's for -mm tree. If I understand -mm workflow correctly, Andrew
> > will merge all the fixes into the final patch before sending it to
> > you in late -rc0 phase, when various -next git trees (on which this
> > driver depend) will be merged into the Linus tree.
> >
> > Andrew, am I correct?
> 
> Yes, that's what I do.
> 
> I prefer to see the incremental patches once a patch has had review or
> testing, so we can see which bits changed.  But if Greg intends to
> merge this driver then he'd probably prefer a fresh new patch,
> particularly as a) version 1 was only sent yesterday and b) he hasn't
> merged version #1.

I do intend to merge it, given that it's in a buildable format :)

So a merged version would be good for me.

thanks,

greg k-h
Anton Vorontsov - Dec. 24, 2008, 9:07 p.m.
On Wed, Dec 24, 2008 at 12:58:35PM -0800, Greg KH wrote:
> On Wed, Dec 24, 2008 at 12:18:53PM -0800, Andrew Morton wrote:
> > On Wed, 24 Dec 2008 23:08:45 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > 
> > > On Wed, Dec 24, 2008 at 11:59:36AM -0800, Greg KH wrote:
> > > > On Wed, Dec 24, 2008 at 10:13:03PM +0300, Anton Vorontsov wrote:
> > > > > Follow these changes for the FHCI driver:
> > > > 
> > > > Hm, would it just make more sense to respin the whole driver?  That way
> > > > it's never in the tree in a "broken" state?
> > > 
> > > It's for -mm tree. If I understand -mm workflow correctly, Andrew
> > > will merge all the fixes into the final patch before sending it to
> > > you in late -rc0 phase, when various -next git trees (on which this
> > > driver depend) will be merged into the Linus tree.
> > >
> > > Andrew, am I correct?
> > 
> > Yes, that's what I do.
> > 
> > I prefer to see the incremental patches once a patch has had review or
> > testing, so we can see which bits changed.  But if Greg intends to
> > merge this driver then he'd probably prefer a fresh new patch,
> > particularly as a) version 1 was only sent yesterday and b) he hasn't
> > merged version #1.
> 
> I do intend to merge it, given that it's in a buildable format :)

The point was that it's not buildable without linux-next queue
(mainly few patches from the powerpc-next git tree). So if you
apply it onto the pure linux-usb tree it won't build. Though it
will build in the -mm tree, as -mm bases on the linux-next.

That's exactly why I asked Andrew to merge it into the -mm tree
in the first place -- he would resend it to you at appropriate
time.


Thanks,
Alan Stern - Dec. 24, 2008, 10:54 p.m.
On Wed, 24 Dec 2008, Anton Vorontsov wrote:

> Follow these changes for the FHCI driver:
> 
> commit e9df41c5c5899259541dc928872cad4d07b82076
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date:   Wed Aug 8 11:48:02 2007 -0400
> 
> USB: make HCDs responsible for managing endpoint queues

On the whole this looks good.

> --- a/drivers/usb/host/fhci-q.c
> +++ b/drivers/usb/host/fhci-q.c
> @@ -200,6 +200,9 @@ void urb_complete_free(struct fhci_hcd *fhci, struct urb *urb)
>  		else
>  			urb->status = 0;
>  	}
> +
> +	usb_hcd_unlink_urb_from_ep(fhci_to_hcd(fhci), urb);
> +
>  	spin_unlock(&fhci->lock);
>  
>  	usb_hcd_giveback_urb(fhci_to_hcd(fhci), urb, urb->status);

For the future you might think about not using urb->status at all.  The 
intention is eventually to remove that field.

Alan Stern

Patch

diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
index e47b8e9..47ac33f 100644
--- a/drivers/usb/host/fhci-hcd.c
+++ b/drivers/usb/host/fhci-hcd.c
@@ -385,7 +385,9 @@  static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
 {
 	struct fhci_hcd *fhci = hcd_to_fhci(hcd);
 	u32 pipe = urb->pipe;
-	int i, size = 0;
+	int ret;
+	int i;
+	int size = 0;
 	struct urb_priv *urb_priv;
 	unsigned long flags;
 
@@ -434,6 +436,11 @@  static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
 	}
 
 	spin_lock_irqsave(&fhci->lock, flags);
+
+	ret = usb_hcd_link_urb_to_ep(hcd, urb);
+	if (ret)
+		goto err;
+
 	/* fill the private part of the URB */
 	urb_priv->num_of_tds = size;
 
@@ -443,9 +450,13 @@  static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
 	urb->hcpriv = urb_priv;
 
 	queue_urb(fhci, urb);
-
+err:
+	if (ret) {
+		kfree(urb_priv->tds);
+		kfree(urb_priv);
+	}
 	spin_unlock_irqrestore(&fhci->lock, flags);
-	return 0;
+	return ret;
 }
 
 /* dequeue FHCI URB */
@@ -453,6 +464,7 @@  static int fhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 {
 	struct fhci_hcd *fhci = hcd_to_fhci(hcd);
 	struct fhci_usb *usb = fhci->usb_lld;
+	int ret = -EINVAL;
 	unsigned long flags;
 
 	if (!urb || !urb->dev || !urb->dev->bus)
@@ -460,6 +472,10 @@  static int fhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 
 	spin_lock_irqsave(&fhci->lock, flags);
 
+	ret = usb_hcd_check_unlink_urb(hcd, urb, status);
+	if (ret)
+		goto out2;
+
 	if (usb->port_status != FHCI_PORT_DISABLED) {
 		struct urb_priv *urb_priv;
 
@@ -483,7 +499,7 @@  static int fhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 out2:
 	spin_unlock_irqrestore(&fhci->lock, flags);
 out:
-	return 0;
+	return ret;
 }
 
 static void fhci_endpoint_disable(struct usb_hcd *hcd,
diff --git a/drivers/usb/host/fhci-q.c b/drivers/usb/host/fhci-q.c
index 721d07d..1173318 100644
--- a/drivers/usb/host/fhci-q.c
+++ b/drivers/usb/host/fhci-q.c
@@ -200,6 +200,9 @@  void urb_complete_free(struct fhci_hcd *fhci, struct urb *urb)
 		else
 			urb->status = 0;
 	}
+
+	usb_hcd_unlink_urb_from_ep(fhci_to_hcd(fhci), urb);
+
 	spin_unlock(&fhci->lock);
 
 	usb_hcd_giveback_urb(fhci_to_hcd(fhci), urb, urb->status);