Re: [PATCH 5/9] xhci-ring.c: Add the poll_pend state to properly abort transactions

Hi Jason,

On Sat, Jul 25, 2020 at 5:51 AM Jason Wessel jason.wessel@windriver.com wrote:

xhci_trl_tx and xhchi_bulk_tx can be called synchronously by other drivers such as the usb storage or network, while the keyboard driver exclusively uses the polling mode.

Could you provide more details as to when this will fail?

And pending polling transactions must be aborted before switching modes to avoid corrupting the state of the controller.

Signed-off-by: Jason Wessel jason.wessel@windriver.com

drivers/usb/host/xhci-ring.c | 86 +++++++++++++++++++++++++++++------- 1 file changed, 70 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index b7b2e16410..d6339f4f0a 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -24,6 +24,12 @@

#include <usb/xhci.h>

+static void *last_bulk_tx_buf; +static struct usb_device *poll_last_udev; +int poll_last_ep_index; +static unsigned long bulk_tx_poll_ts; +static bool poll_pend;

Should these variables go into struct xhci_ctrl? What happens if 2 controllers are sending data?

/**

• Is this TRB a link TRB or was the last TRB the last TRB in this event ring
• segment? I.e. would the updated event TRB pointer step off the end of the

@@ -549,19 +555,8 @@ static void record_transfer_result(struct usb_device *udev, } }

-/**** Bulk and Control transfer methods ****/ -/**

• • Queues up the BULK Request
• • @param udev pointer to the USB device structure
• • @param pipe contains the DIR_IN or OUT , devnum
• • @param length length of the buffer
• • @param buffer buffer to be read/written based on the request
• • @param nonblock when true do not block waiting for response
• • @return returns 0 if successful else -1 on failure
• */

-int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,

•                   int length, void *buffer, bool nonblock)
  

+static int _xhci_bulk_tx_queue(struct usb_device *udev, unsigned long pipe,

•                          int length, void *buffer)
  

{ int num_trbs = 0; struct xhci_generic_trb *start_trb; @@ -575,7 +570,6 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, struct xhci_virt_device *virt_dev; struct xhci_ep_ctx *ep_ctx; struct xhci_ring *ring; /* EP transfer ring */

•   union xhci_trb *event;
  
    int running_total, trb_buff_len;
    unsigned int total_packet_count;
  

@@ -719,20 +713,73 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, } while (running_total < length);

    giveback_first_trb(udev, ep_index, start_cycle, start_trb);

•   return 0;
  

+}

+/**** Bulk and Control transfer methods ****/ +/**

• • Queues up the BULK Request
• • @param udev pointer to the USB device structure
• • @param pipe contains the DIR_IN or OUT , devnum
• • @param length length of the buffer
• • @param buffer buffer to be read/written based on the request
• • @param nonblock when true do not block waiting for response
• • @return returns 0 if successful else -1 on failure
• */

+int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,

•            int length, void *buffer, bool nonblock)
  

+{

•   u32 field;
  

•   int ret;
  

•   union xhci_trb *event;
  

•   struct xhci_ctrl *ctrl = xhci_get_ctrl(udev);
  

•   int ep_index = usb_pipe_ep_index(pipe);
  

•   if (poll_pend) {
  

•           /*
  

•            * Abort a pending poll operation if it should have
  

•            * timed out, or if this is a different buffer from a
  

•            * separate request
  

•            */
  

•           if (get_timer(bulk_tx_poll_ts) > XHCI_TIMEOUT ||
  

•               last_bulk_tx_buf != buffer || poll_last_udev != udev ||
  

•               ep_index != poll_last_ep_index) {
  

•                   abort_td(poll_last_udev, poll_last_ep_index);
  

•                   poll_last_udev->status = USB_ST_NAK_REC;  /* closest thing to a timeout */
  

•                   poll_last_udev->act_len = 0;
  

•                   poll_pend = false;
  

•           }
  

•   } /* No else here because poll_pend might have changed above */
  

•   if (!poll_pend) {
  

•           last_bulk_tx_buf = buffer;
  

•           ret = _xhci_bulk_tx_queue(udev, pipe, length, buffer);
  

•           if (ret)
  

•                   return ret;
  

•   }
    event = xhci_wait_for_event(ctrl, TRB_TRANSFER, nonblock);
    if (!event) {
  

•           if (nonblock)
  

•           if (nonblock) {
  

•                   if (!poll_pend) {
  

•                           /* Start the timer */
  

•                           bulk_tx_poll_ts = get_timer(0);
  

•                           poll_last_udev = udev;
  

•                           poll_last_ep_index = ep_index;
  

•                           poll_pend = true;
  

•                   }
                    return -EINVAL;
  

•           }
            debug("XHCI bulk transfer timed out, aborting...\n");
            abort_td(udev, ep_index);
            udev->status = USB_ST_NAK_REC;  /* closest thing to a timeout */
            udev->act_len = 0;
  

•           poll_pend = false;
            return -ETIMEDOUT;
    }
  

•   poll_pend = false;
    field = le32_to_cpu(event->trans_event.flags);
  
  

•   BUG_ON(TRB_TO_SLOT_ID(field) != slot_id);
  

•   BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id);
    BUG_ON(TRB_TO_EP_INDEX(field) != ep_index);
    BUG_ON(*(void **)(uintptr_t)le64_to_cpu(event->trans_event.buffer) -
            buffer > (size_t)length);
  

@@ -779,6 +826,13 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, le16_to_cpu(req->value), le16_to_cpu(req->value), le16_to_cpu(req->index));

•   if (poll_pend) {
  

•           abort_td(poll_last_udev, poll_last_ep_index);
  

•           poll_last_udev->status = USB_ST_NAK_REC;  /* closest thing to a timeout */
  

•           poll_last_udev->act_len = 0;
  

•           poll_pend = false;
  

•   }
  

•   ep_index = usb_pipe_ep_index(pipe);
  
    ep_ring = virt_dev->eps[ep_index].ring;
  

--

Regards, Bin