Re: [patch 2.6.29+ 7/8] musb_host: refactor URB giveback

[Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>]

Hello, I wrote:

> > As musb_advance_schedule() is now the only remaning
> > caller of musb_giveback() (and the only valid context
> > of such call), just fold the latter into the former
> > and then rename __musb_giveback() into musb_giveback().

> > This is a net minor shrink.

> > Signed-off-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
> > Signed-off-by: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>

> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -295,9 +295,8 @@ start:
> >      }
> >  }
> >  
> > -/* caller owns controller lock, irqs are blocked */
> > -static void
> > -__musb_giveback(struct musb *musb, struct urb *urb, int status)
> > +/* Context: caller owns controller lock, IRQs are blocked */
> > +static void musb_giveback(struct musb *musb, struct urb *urb, int 
> > status)
> >  __releases(musb->lock)
> >  __acquires(musb->lock)
> >  {
> > @@ -350,14 +349,22 @@ static inline void musb_save_toggle(stru
> >      usb_settoggle(urb->dev, qh->epnum, !is_in, csr ? 1 : 0);
> >  }
> >  
> > -/* caller owns controller lock, irqs are blocked */
> > -static struct musb_qh *
> > -musb_giveback(struct musb_qh *qh, struct urb *urb, int status)
> > +/*
> > + * Advance this hardware endpoint's queue, completing the specified 
> > URB and
> > + * advancing to either the next URB queued to that qh, or else 
> > invalidating
> > + * that qh and advancing to the next qh scheduled after the current one.
> > + *
> > + * Context: caller owns controller lock, IRQs are blocked
> > + */
> > +static void musb_advance_schedule(struct musb *musb, struct urb *urb,
> > +                  struct musb_hw_ep *hw_ep, int is_in)
> >  {
> > +    struct musb_qh        *qh = musb_ep_get_qh(hw_ep, is_in);
> >      struct musb_hw_ep    *ep = qh->hw_ep;
> > -    struct musb        *musb = ep->musb;
> > -    int            is_in = usb_pipein(urb->pipe);
> >      int            ready = qh->is_ready;
> > +    int            status;
> > +
> > +    status = (urb->status == -EINPROGRESS) ? 0 : urb->status;
> >  
> >      /* save toggle eagerly, for paranoia */
> >      switch (qh->type) {
> > @@ -366,13 +373,13 @@ musb_giveback(struct musb_qh *qh, struct
> >          musb_save_toggle(qh, is_in, urb);
> >          break;
> >      case USB_ENDPOINT_XFER_ISOC:
> > -        if (status == 0 && urb->error_count)
> > +        if (urb->error_count)

>    Oops, this change looks wrong in the context of this patch. It was 
> done (initially for the older kernel) since musb_giveback() used to be 
> always called by musb_advance_schedule() with 'status' argument of 0. 
> Now it obviously isn't so. David, could you repost the patch without 
> this change?

   Looks like we'll have to go a longer way -- by me reposting the patch 
first... :-/

WBR, Sergei

--
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