iv/linux
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

USB: pxa27x_udc: Fix deadlocks on request queueing

Browse Source 
As reported by Antonio, there are cases where the ep->lock
can be taken twice, triggering a deadlock.
The typical sequence is :
 irq_handler
   \
    -> gadget.complete()
       \
        -> pxa27x_udc.pxa_ep_queue() : ep->lock is taken
           \
            -> gadget.complete()
               \
                -> pxa27x_udc.pxa_ep_queue() : ep->lock is taken
                                               ==> *deadlock*
The patch fixes this by :
 - releasing the lock each time gadget.complete() is called
 - adding a check in handle_ep() to detect a recursive call,
   in which case the function becomes on no-op.

The patch is still not good enough for ep0. For this unique
endpoint, another well thought over patch will be needed.

Reported-by: Antonio Ospite <ospite@studenti.unina.it>
Tested-by: Antonio Ospite <ospite@studenti.unina.it>
Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Eric Miao <eric.y.miao@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
This commit is contained in:
Robert Jarzmik 2010-01-27 18:38:03 +01:00 committed by Greg Kroah-Hartman
parent fb088e335d
commit 5e23e90f33
2 changed files with 85 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

114
drivers/usb/gadget/pxa27x_udc.c
View File

@ -742,13 +742,17 @@ static void ep_del_request(struct pxa_ep *ep, struct pxa27x_request *req)
* @ep: pxa physical endpoint * @ep: pxa physical endpoint
* @req: pxa request * @req: pxa request
* @status: usb request status sent to gadget API * @status: usb request status sent to gadget API
* @pflags: flags of previous spinlock_irq_save() or NULL if no lock held
* *
* Context: ep->lock held * Context: ep->lock held if flags not NULL, else ep->lock released
* *
* Retire a pxa27x usb request. Endpoint must be locked. * Retire a pxa27x usb request. Endpoint must be locked.
*/ */
static void req_done(struct pxa_ep *ep, struct pxa27x_request *req, int status) static void req_done(struct pxa_ep *ep, struct pxa27x_request *req, int status,
unsigned long *pflags)
{ {
unsigned long flags;
ep_del_request(ep, req); ep_del_request(ep, req);
if (likely(req->req.status == -EINPROGRESS)) if (likely(req->req.status == -EINPROGRESS))
req->req.status = status; req->req.status = status;
@ -760,38 +764,48 @@ static void req_done(struct pxa_ep *ep, struct pxa27x_request *req, int status)
&req->req, status, &req->req, status,
req->req.actual, req->req.length); req->req.actual, req->req.length);
if (pflags)
spin_unlock_irqrestore(&ep->lock, *pflags);
local_irq_save(flags);
req->req.complete(&req->udc_usb_ep->usb_ep, &req->req); req->req.complete(&req->udc_usb_ep->usb_ep, &req->req);
local_irq_restore(flags);
if (pflags)
spin_lock_irqsave(&ep->lock, *pflags);
} }
/** /**
* ep_end_out_req - Ends endpoint OUT request * ep_end_out_req - Ends endpoint OUT request
* @ep: physical endpoint * @ep: physical endpoint
* @req: pxa request * @req: pxa request
* @pflags: flags of previous spinlock_irq_save() or NULL if no lock held
* *
* Context: ep->lock held * Context: ep->lock held or released (see req_done())
* *
* Ends endpoint OUT request (completes usb request). * Ends endpoint OUT request (completes usb request).
*/ */
static void ep_end_out_req(struct pxa_ep *ep, struct pxa27x_request *req) static void ep_end_out_req(struct pxa_ep *ep, struct pxa27x_request *req,
unsigned long *pflags)
{ {
inc_ep_stats_reqs(ep, !USB_DIR_IN); inc_ep_stats_reqs(ep, !USB_DIR_IN);
req_done(ep, req, 0); req_done(ep, req, 0, pflags);
} }
/** /**
* ep0_end_out_req - Ends control endpoint OUT request (ends data stage) * ep0_end_out_req - Ends control endpoint OUT request (ends data stage)
* @ep: physical endpoint * @ep: physical endpoint
* @req: pxa request * @req: pxa request
* @pflags: flags of previous spinlock_irq_save() or NULL if no lock held
* *
* Context: ep->lock held * Context: ep->lock held or released (see req_done())
* *
* Ends control endpoint OUT request (completes usb request), and puts * Ends control endpoint OUT request (completes usb request), and puts
* control endpoint into idle state * control endpoint into idle state
*/ */
static void ep0_end_out_req(struct pxa_ep *ep, struct pxa27x_request *req) static void ep0_end_out_req(struct pxa_ep *ep, struct pxa27x_request *req,
unsigned long *pflags)
{ {
set_ep0state(ep->dev, OUT_STATUS_STAGE); set_ep0state(ep->dev, OUT_STATUS_STAGE);
ep_end_out_req(ep, req); ep_end_out_req(ep, req, pflags);
ep0_idle(ep->dev); ep0_idle(ep->dev);
} }
@ -799,31 +813,35 @@ static void ep0_end_out_req(struct pxa_ep *ep, struct pxa27x_request *req)
* ep_end_in_req - Ends endpoint IN request * ep_end_in_req - Ends endpoint IN request
* @ep: physical endpoint * @ep: physical endpoint
* @req: pxa request * @req: pxa request
* @pflags: flags of previous spinlock_irq_save() or NULL if no lock held
* *
* Context: ep->lock held * Context: ep->lock held or released (see req_done())
* *
* Ends endpoint IN request (completes usb request). * Ends endpoint IN request (completes usb request).
*/ */
static void ep_end_in_req(struct pxa_ep *ep, struct pxa27x_request *req) static void ep_end_in_req(struct pxa_ep *ep, struct pxa27x_request *req,
unsigned long *pflags)
{ {
inc_ep_stats_reqs(ep, USB_DIR_IN); inc_ep_stats_reqs(ep, USB_DIR_IN);
req_done(ep, req, 0); req_done(ep, req, 0, pflags);
} }
/** /**
* ep0_end_in_req - Ends control endpoint IN request (ends data stage) * ep0_end_in_req - Ends control endpoint IN request (ends data stage)
* @ep: physical endpoint * @ep: physical endpoint
* @req: pxa request * @req: pxa request
* @pflags: flags of previous spinlock_irq_save() or NULL if no lock held
* *
* Context: ep->lock held * Context: ep->lock held or released (see req_done())
* *
* Ends control endpoint IN request (completes usb request), and puts * Ends control endpoint IN request (completes usb request), and puts
* control endpoint into status state * control endpoint into status state
*/ */
static void ep0_end_in_req(struct pxa_ep *ep, struct pxa27x_request *req) static void ep0_end_in_req(struct pxa_ep *ep, struct pxa27x_request *req,
unsigned long *pflags)
{ {
set_ep0state(ep->dev, IN_STATUS_STAGE); set_ep0state(ep->dev, IN_STATUS_STAGE);
ep_end_in_req(ep, req); ep_end_in_req(ep, req, pflags);
} }
/** /**
@ -831,19 +849,22 @@ static void ep0_end_in_req(struct pxa_ep *ep, struct pxa27x_request *req)
* @ep: pxa endpoint * @ep: pxa endpoint
* @status: usb request status * @status: usb request status
* *
* Context: ep->lock held * Context: ep->lock released
* *
* Dequeues all requests on an endpoint. As a side effect, interrupts will be * Dequeues all requests on an endpoint. As a side effect, interrupts will be
* disabled on that endpoint (because no more requests). * disabled on that endpoint (because no more requests).
*/ */
static void nuke(struct pxa_ep *ep, int status) static void nuke(struct pxa_ep *ep, int status)
{ {
struct pxa27x_request *req; struct pxa27x_request *req;
unsigned long flags;
spin_lock_irqsave(&ep->lock, flags);
while (!list_empty(&ep->queue)) { while (!list_empty(&ep->queue)) {
req = list_entry(ep->queue.next, struct pxa27x_request, queue); req = list_entry(ep->queue.next, struct pxa27x_request, queue);
req_done(ep, req, status); req_done(ep, req, status, &flags);
} }
spin_unlock_irqrestore(&ep->lock, flags);
} }
/** /**
@ -1123,6 +1144,7 @@ static int pxa_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
int rc = 0; int rc = 0;
int is_first_req; int is_first_req;
unsigned length; unsigned length;
int recursion_detected;
req = container_of(_req, struct pxa27x_request, req); req = container_of(_req, struct pxa27x_request, req);
udc_usb_ep = container_of(_ep, struct udc_usb_ep, usb_ep); udc_usb_ep = container_of(_ep, struct udc_usb_ep, usb_ep);
@ -1152,6 +1174,7 @@ static int pxa_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
return -EMSGSIZE; return -EMSGSIZE;
spin_lock_irqsave(&ep->lock, flags); spin_lock_irqsave(&ep->lock, flags);
recursion_detected = ep->in_handle_ep;
is_first_req = list_empty(&ep->queue); is_first_req = list_empty(&ep->queue);
ep_dbg(ep, "queue req %p(first=%s), len %d buf %p\n", ep_dbg(ep, "queue req %p(first=%s), len %d buf %p\n",
@ -1161,12 +1184,12 @@ static int pxa_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
if (!ep->enabled) { if (!ep->enabled) {
_req->status = -ESHUTDOWN; _req->status = -ESHUTDOWN;
rc = -ESHUTDOWN; rc = -ESHUTDOWN;
goto out; goto out_locked;
} }
if (req->in_use) { if (req->in_use) {
ep_err(ep, "refusing to queue req %p (already queued)\n", req); ep_err(ep, "refusing to queue req %p (already queued)\n", req);
goto out; goto out_locked;
} }
length = _req->length; length = _req->length;
@ -1174,12 +1197,13 @@ static int pxa_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
_req->actual = 0; _req->actual = 0;
ep_add_request(ep, req); ep_add_request(ep, req);
spin_unlock_irqrestore(&ep->lock, flags);
if (is_ep0(ep)) { if (is_ep0(ep)) {
switch (dev->ep0state) { switch (dev->ep0state) {
case WAIT_ACK_SET_CONF_INTERF: case WAIT_ACK_SET_CONF_INTERF:
if (length == 0) { if (length == 0) {
ep_end_in_req(ep, req); ep_end_in_req(ep, req, NULL);
} else { } else {
ep_err(ep, "got a request of %d bytes while" ep_err(ep, "got a request of %d bytes while"
"in state WAIT_ACK_SET_CONF_INTERF\n", "in state WAIT_ACK_SET_CONF_INTERF\n",
@ -1192,12 +1216,12 @@ static int pxa_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
case IN_DATA_STAGE: case IN_DATA_STAGE:
if (!ep_is_full(ep)) if (!ep_is_full(ep))
if (write_ep0_fifo(ep, req)) if (write_ep0_fifo(ep, req))
ep0_end_in_req(ep, req); ep0_end_in_req(ep, req, NULL);
break; break;
case OUT_DATA_STAGE: case OUT_DATA_STAGE:
if ((length == 0) || !epout_has_pkt(ep)) if ((length == 0) || !epout_has_pkt(ep))
if (read_ep0_fifo(ep, req)) if (read_ep0_fifo(ep, req))
ep0_end_out_req(ep, req); ep0_end_out_req(ep, req, NULL);
break; break;
default: default:
ep_err(ep, "odd state %s to send me a request\n", ep_err(ep, "odd state %s to send me a request\n",
@ -1207,12 +1231,15 @@ static int pxa_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
break; break;
} }
} else { } else {
handle_ep(ep); if (!recursion_detected)
handle_ep(ep);
} }
out: out:
spin_unlock_irqrestore(&ep->lock, flags);
return rc; return rc;
out_locked:
spin_unlock_irqrestore(&ep->lock, flags);
goto out;
} }
/** /**
@ -1242,13 +1269,14 @@ static int pxa_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
/* make sure it's actually queued on this endpoint */ /* make sure it's actually queued on this endpoint */
list_for_each_entry(req, &ep->queue, queue) { list_for_each_entry(req, &ep->queue, queue) {
if (&req->req == _req) { if (&req->req == _req) {
req_done(ep, req, -ECONNRESET);
rc = 0; rc = 0;
break; break;
} }
} }
spin_unlock_irqrestore(&ep->lock, flags); spin_unlock_irqrestore(&ep->lock, flags);
if (!rc)
req_done(ep, req, -ECONNRESET, NULL);
return rc; return rc;
} }
@ -1445,7 +1473,6 @@ static int pxa_ep_disable(struct usb_ep *_ep)
{ {
struct pxa_ep *ep; struct pxa_ep *ep;
struct udc_usb_ep *udc_usb_ep; struct udc_usb_ep *udc_usb_ep;
unsigned long flags;
if (!_ep) if (!_ep)
return -EINVAL; return -EINVAL;
@ -1455,10 +1482,8 @@ static int pxa_ep_disable(struct usb_ep *_ep)
if (!ep || is_ep0(ep) || !list_empty(&ep->queue)) if (!ep || is_ep0(ep) || !list_empty(&ep->queue))
return -EINVAL; return -EINVAL;
spin_lock_irqsave(&ep->lock, flags);
ep->enabled = 0; ep->enabled = 0;
nuke(ep, -ESHUTDOWN); nuke(ep, -ESHUTDOWN);
spin_unlock_irqrestore(&ep->lock, flags);
pxa_ep_fifo_flush(_ep); pxa_ep_fifo_flush(_ep);
udc_usb_ep->pxa_ep = NULL; udc_usb_ep->pxa_ep = NULL;
@ -1907,8 +1932,10 @@ static void handle_ep0_ctrl_req(struct pxa_udc *udc,
} u; } u;
int i; int i;
int have_extrabytes = 0; int have_extrabytes = 0;
unsigned long flags;
nuke(ep, -EPROTO); nuke(ep, -EPROTO);
spin_lock_irqsave(&ep->lock, flags);
/* /*
* In the PXA320 manual, in the section about Back-to-Back setup * In the PXA320 manual, in the section about Back-to-Back setup
@ -1947,10 +1974,13 @@ static void handle_ep0_ctrl_req(struct pxa_udc *udc,
/* Tell UDC to enter Data Stage */ /* Tell UDC to enter Data Stage */
ep_write_UDCCSR(ep, UDCCSR0_SA | UDCCSR0_OPC); ep_write_UDCCSR(ep, UDCCSR0_SA | UDCCSR0_OPC);
spin_unlock_irqrestore(&ep->lock, flags);
i = udc->driver->setup(&udc->gadget, &u.r); i = udc->driver->setup(&udc->gadget, &u.r);
spin_lock_irqsave(&ep->lock, flags);
if (i < 0) if (i < 0)
goto stall; goto stall;
out: out:
spin_unlock_irqrestore(&ep->lock, flags);
return; return;
stall: stall:
ep_dbg(ep, "protocol STALL, udccsr0=%03x err %d\n", ep_dbg(ep, "protocol STALL, udccsr0=%03x err %d\n",
@ -2055,13 +2085,13 @@ static void handle_ep0(struct pxa_udc *udc, int fifo_irq, int opc_irq)
if (req && !ep_is_full(ep)) if (req && !ep_is_full(ep))
completed = write_ep0_fifo(ep, req); completed = write_ep0_fifo(ep, req);
if (completed) if (completed)
ep0_end_in_req(ep, req); ep0_end_in_req(ep, req, NULL);
break; break;
case OUT_DATA_STAGE: /* SET_DESCRIPTOR */ case OUT_DATA_STAGE: /* SET_DESCRIPTOR */
if (epout_has_pkt(ep) && req) if (epout_has_pkt(ep) && req)
completed = read_ep0_fifo(ep, req); completed = read_ep0_fifo(ep, req);
if (completed) if (completed)
ep0_end_out_req(ep, req); ep0_end_out_req(ep, req, NULL);
break; break;
case STALL: case STALL:
ep_write_UDCCSR(ep, UDCCSR0_FST); ep_write_UDCCSR(ep, UDCCSR0_FST);
@ -2091,7 +2121,7 @@ static void handle_ep0(struct pxa_udc *udc, int fifo_irq, int opc_irq)
* Tries to transfer all pending request data into the endpoint and/or * Tries to transfer all pending request data into the endpoint and/or
* transfer all pending data in the endpoint into usb requests. * transfer all pending data in the endpoint into usb requests.
* *
* Is always called when in_interrupt() or with ep->lock held. * Is always called when in_interrupt() and with ep->lock released.
*/ */
static void handle_ep(struct pxa_ep *ep) static void handle_ep(struct pxa_ep *ep)
{ {
@ -2100,10 +2130,17 @@ static void handle_ep(struct pxa_ep *ep)
u32 udccsr; u32 udccsr;
int is_in = ep->dir_in; int is_in = ep->dir_in;
int loop = 0; int loop = 0;
unsigned long flags;
spin_lock_irqsave(&ep->lock, flags);
if (ep->in_handle_ep)
goto recursion_detected;
ep->in_handle_ep = 1;
do { do {
completed = 0; completed = 0;
udccsr = udc_ep_readl(ep, UDCCSR); udccsr = udc_ep_readl(ep, UDCCSR);
if (likely(!list_empty(&ep->queue))) if (likely(!list_empty(&ep->queue)))
req = list_entry(ep->queue.next, req = list_entry(ep->queue.next,
struct pxa27x_request, queue); struct pxa27x_request, queue);
@ -2122,15 +2159,22 @@ static void handle_ep(struct pxa_ep *ep)
if (unlikely(is_in)) { if (unlikely(is_in)) {
if (likely(!ep_is_full(ep))) if (likely(!ep_is_full(ep)))
completed = write_fifo(ep, req); completed = write_fifo(ep, req);
if (completed)
ep_end_in_req(ep, req);
} else { } else {
if (likely(epout_has_pkt(ep))) if (likely(epout_has_pkt(ep)))
completed = read_fifo(ep, req); completed = read_fifo(ep, req);
if (completed) }
ep_end_out_req(ep, req);
if (completed) {
if (is_in)
ep_end_in_req(ep, req, &flags);
else
ep_end_out_req(ep, req, &flags);
} }
} while (completed); } while (completed);
ep->in_handle_ep = 0;
recursion_detected:
spin_unlock_irqrestore(&ep->lock, flags);
} }
/** /**

6
drivers/usb/gadget/pxa27x_udc.h
View File

@ -318,6 +318,11 @@ struct udc_usb_ep {
* @queue: requests queue * @queue: requests queue
* @lock: lock to pxa_ep data (queues and stats) * @lock: lock to pxa_ep data (queues and stats)
* @enabled: true when endpoint enabled (not stopped by gadget layer) * @enabled: true when endpoint enabled (not stopped by gadget layer)
* @in_handle_ep: number of recursions of handle_ep() function
* Prevents deadlocks or infinite recursions of types :
* irq->handle_ep()->req_done()->req.complete()->pxa_ep_queue()->handle_ep()
* or
* pxa_ep_queue()->handle_ep()->req_done()->req.complete()->pxa_ep_queue()
* @idx: endpoint index (1 => epA, 2 => epB, ..., 24 => epX) * @idx: endpoint index (1 => epA, 2 => epB, ..., 24 => epX)
* @name: endpoint name (for trace/debug purpose) * @name: endpoint name (for trace/debug purpose)
* @dir_in: 1 if IN endpoint, 0 if OUT endpoint * @dir_in: 1 if IN endpoint, 0 if OUT endpoint
@ -346,6 +351,7 @@ struct pxa_ep {
spinlock_t lock; /* Protects this structure */ spinlock_t lock; /* Protects this structure */
/* (queues, stats) */ /* (queues, stats) */
unsigned enabled:1; unsigned enabled:1;
unsigned in_handle_ep:1;
unsigned idx:5; unsigned idx:5;
char *name; char *name;