| Message ID | 20230913150058.38972-1-artur@conclusive.pl |
|---|---|
| State | Accepted |
| Commit | bc8e94c902d8b5a24d6b31963fc26058b54986ef |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | USB: gadget: atmel: fix transfer of queued requests | expand |
On 9/13/23 17:00, Artur Rojek wrote: > In the existing implementation, multiple requests queued up on an > endpoint are subject to getting evicted without transmission. > > For both control and bulk endpoints, their respective logic found in > usba_control_irq()/usba_ep_irq() guarantees that TX FIFO is empty before > data is sent out, and that request_complete() gets called once the > transaction has been finished. At this point however, if any additional > requests are found on the endpoint queue, they will be processed by > submit_next_request(), which makes no checks against the above > conditions, trashing data on a busy FIFO and neglecting completion > handlers. > > Fix the above issues by removing the calls to submit_next_request(), > and thus forcing the pending requests to be processed on the next pass > of the respective endpoint logic. While at it, remove a DBG message, as > that branch becomes part of regular flow. > > This restores mass storage mode operation on Microchip ATSAMA5D27 SoC. Updated subject prefix to lowercase usb: and applied to usb/next, thanks.
diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c index 7d51821497b4..e6b458d940d8 100644 --- a/drivers/usb/gadget/atmel_usba_udc.c +++ b/drivers/usb/gadget/atmel_usba_udc.c @@ -57,13 +57,9 @@ static void submit_request(struct usba_ep *ep, struct usba_request *req) req->submitted = 1; next_fifo_transaction(ep, req); - if (req->last_transaction) { - usba_ep_writel(ep, CTL_DIS, USBA_TX_PK_RDY); - usba_ep_writel(ep, CTL_ENB, USBA_TX_COMPLETE); - } else { + if (ep_is_control(ep)) usba_ep_writel(ep, CTL_DIS, USBA_TX_COMPLETE); - usba_ep_writel(ep, CTL_ENB, USBA_TX_PK_RDY); - } + usba_ep_writel(ep, CTL_ENB, USBA_TX_PK_RDY); } static void submit_next_request(struct usba_ep *ep) @@ -889,7 +885,6 @@ restart: if (req) { list_del_init(&req->queue); request_complete(ep, req, 0); - submit_next_request(ep); } usba_ep_writel(ep, CTL_DIS, USBA_TX_COMPLETE); ep->state = WAIT_FOR_SETUP; @@ -1036,7 +1031,6 @@ static void usba_ep_irq(struct usba_udc *udc, struct usba_ep *ep) DBG(DBG_BUS, "%s: TX PK ready\n", ep->ep.name); if (list_empty(&ep->queue)) { - DBG(DBG_INT, "ep_irq: queue empty\n"); usba_ep_writel(ep, CTL_DIS, USBA_TX_PK_RDY); return; } @@ -1050,7 +1044,6 @@ static void usba_ep_irq(struct usba_udc *udc, struct usba_ep *ep) if (req->last_transaction) { list_del_init(&req->queue); - submit_next_request(ep); request_complete(ep, req, 0); }
In the existing implementation, multiple requests queued up on an endpoint are subject to getting evicted without transmission. For both control and bulk endpoints, their respective logic found in usba_control_irq()/usba_ep_irq() guarantees that TX FIFO is empty before data is sent out, and that request_complete() gets called once the transaction has been finished. At this point however, if any additional requests are found on the endpoint queue, they will be processed by submit_next_request(), which makes no checks against the above conditions, trashing data on a busy FIFO and neglecting completion handlers. Fix the above issues by removing the calls to submit_next_request(), and thus forcing the pending requests to be processed on the next pass of the respective endpoint logic. While at it, remove a DBG message, as that branch becomes part of regular flow. This restores mass storage mode operation on Microchip ATSAMA5D27 SoC. Signed-off-by: Artur Rojek <artur@conclusive.pl> --- drivers/usb/gadget/atmel_usba_udc.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)