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

[Bin Meng]

Hi Jason,

On Sat, Jul 25, 2020 at 5:51 AM Jason Wessel <jason.wessel at 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 at 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