On Fri, Mar 01, 2013 at 03:42:23PM +0100, Michael Grzeschik wrote:
> There is no need to call ep_queue unlocked inside the own driver. We
> move its functionionality into an unlocked version.
> 
> This patch removes potential unlocked timeslots inside
> isr_setup_status_phase and isr_get_status_response, in which the lock
> got released just before acquired again inside usb_ep_queue.

You mean before or after?

For other parts, it is ok, you can add:
Reviewed-by: Peter Chen <peter.c...@freescale.com>
> 
> Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <m...@pengutronix.de>
> ---
> Changes since v1:
>  - changed patch description as mentioned by alexander
> 
>  drivers/usb/chipidea/udc.c |  113 
> ++++++++++++++++++++++++--------------------
>  1 file changed, 63 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 2f45bba..22d37d8 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -668,6 +668,66 @@ static void isr_get_status_complete(struct usb_ep *ep, 
> struct usb_request *req)
>  }
>  
>  /**
> + * _ep_queue: queues (submits) an I/O request to an endpoint
> + *
> + * Caller must hold lock
> + */
> +static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
> +                 gfp_t __maybe_unused gfp_flags)
> +{
> +     struct ci13xxx_ep  *mEp  = container_of(ep,  struct ci13xxx_ep, ep);
> +     struct ci13xxx_req *mReq = container_of(req, struct ci13xxx_req, req);
> +     struct ci13xxx *ci = mEp->ci;
> +     int retval = 0;
> +
> +     if (ep == NULL || req == NULL || mEp->ep.desc == NULL)
> +             return -EINVAL;
> +
> +     if (mEp->type == USB_ENDPOINT_XFER_CONTROL) {
> +             if (req->length)
> +                     mEp = (ci->ep0_dir == RX) ?
> +                            ci->ep0out : ci->ep0in;
> +             if (!list_empty(&mEp->qh.queue)) {
> +                     _ep_nuke(mEp);
> +                     retval = -EOVERFLOW;
> +                     dev_warn(mEp->ci->dev, "endpoint ctrl %X nuked\n",
> +                              _usb_addr(mEp));
> +             }
> +     }
> +
> +     /* first nuke then test link, e.g. previous status has not sent */
> +     if (!list_empty(&mReq->queue)) {
> +             retval = -EBUSY;
> +             dev_err(mEp->ci->dev, "request already in queue\n");
> +             goto done;
> +     }
> +
> +     if (req->length > 4 * CI13XXX_PAGE_SIZE) {
> +             req->length = 4 * CI13XXX_PAGE_SIZE;
> +             retval = -EMSGSIZE;
> +             dev_warn(mEp->ci->dev, "request length truncated\n");
> +     }
> +
> +     dbg_queue(_usb_addr(mEp), req, retval);
> +
> +     /* push request */
> +     mReq->req.status = -EINPROGRESS;
> +     mReq->req.actual = 0;
> +
> +     retval = _hardware_enqueue(mEp, mReq);
> +
> +     if (retval == -EALREADY) {
> +             dbg_event(_usb_addr(mEp), "QUEUE", retval);
> +             retval = 0;
> +     }
> +     if (!retval)
> +             list_add_tail(&mReq->queue, &mEp->qh.queue);
> +
> + done:
> +     return retval;
> +}
> +
> +/**
>   * isr_get_status_response: get_status request response
>   * @ci: ci struct
>   * @setup: setup request packet
> @@ -714,9 +774,7 @@ __acquires(mEp->lock)
>       }
>       /* else do nothing; reserved for future use */
>  
> -     spin_unlock(mEp->lock);
> -     retval = usb_ep_queue(&mEp->ep, req, gfp_flags);
> -     spin_lock(mEp->lock);
> +     retval = _ep_queue(&mEp->ep, req, gfp_flags);
>       if (retval)
>               goto err_free_buf;
>  
> @@ -763,8 +821,6 @@ isr_setup_status_complete(struct usb_ep *ep, struct 
> usb_request *req)
>   * This function returns an error code
>   */
>  static int isr_setup_status_phase(struct ci13xxx *ci)
> -__releases(mEp->lock)
> -__acquires(mEp->lock)
>  {
>       int retval;
>       struct ci13xxx_ep *mEp;
> @@ -773,9 +829,7 @@ __acquires(mEp->lock)
>       ci->status->context = ci;
>       ci->status->complete = isr_setup_status_complete;
>  
> -     spin_unlock(mEp->lock);
> -     retval = usb_ep_queue(&mEp->ep, ci->status, GFP_ATOMIC);
> -     spin_lock(mEp->lock);
> +     retval = _ep_queue(&mEp->ep, ci->status, GFP_ATOMIC);
>  
>       return retval;
>  }
> @@ -1172,8 +1226,6 @@ static int ep_queue(struct usb_ep *ep, struct 
> usb_request *req,
>                   gfp_t __maybe_unused gfp_flags)
>  {
>       struct ci13xxx_ep  *mEp  = container_of(ep,  struct ci13xxx_ep, ep);
> -     struct ci13xxx_req *mReq = container_of(req, struct ci13xxx_req, req);
> -     struct ci13xxx *ci = mEp->ci;
>       int retval = 0;
>       unsigned long flags;
>  
> @@ -1182,47 +1234,8 @@ static int ep_queue(struct usb_ep *ep, struct 
> usb_request *req,
>  
>       spin_lock_irqsave(mEp->lock, flags);
>  
> -     if (mEp->type == USB_ENDPOINT_XFER_CONTROL) {
> -             if (req->length)
> -                     mEp = (ci->ep0_dir == RX) ?
> -                            ci->ep0out : ci->ep0in;
> -             if (!list_empty(&mEp->qh.queue)) {
> -                     _ep_nuke(mEp);
> -                     retval = -EOVERFLOW;
> -                     dev_warn(mEp->ci->dev, "endpoint ctrl %X nuked\n",
> -                              _usb_addr(mEp));
> -             }
> -     }
> +     retval = _ep_queue(ep, req, gfp_flags);
>  
> -     /* first nuke then test link, e.g. previous status has not sent */
> -     if (!list_empty(&mReq->queue)) {
> -             retval = -EBUSY;
> -             dev_err(mEp->ci->dev, "request already in queue\n");
> -             goto done;
> -     }
> -
> -     if (req->length > 4 * CI13XXX_PAGE_SIZE) {
> -             req->length = 4 * CI13XXX_PAGE_SIZE;
> -             retval = -EMSGSIZE;
> -             dev_warn(mEp->ci->dev, "request length truncated\n");
> -     }
> -
> -     dbg_queue(_usb_addr(mEp), req, retval);
> -
> -     /* push request */
> -     mReq->req.status = -EINPROGRESS;
> -     mReq->req.actual = 0;
> -
> -     retval = _hardware_enqueue(mEp, mReq);
> -
> -     if (retval == -EALREADY) {
> -             dbg_event(_usb_addr(mEp), "QUEUE", retval);
> -             retval = 0;
> -     }
> -     if (!retval)
> -             list_add_tail(&mReq->queue, &mEp->qh.queue);
> -
> - done:
>       spin_unlock_irqrestore(mEp->lock, flags);
>       return retval;
>  }
> -- 
> 1.7.10.4
> 
> 

-- 

Best Regards,
Peter Chen

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to