[PATCH 5/9] xhci-ring.c: Add the poll_pend state to properly abort transactions
Jason Wessel
jason.wessel at windriver.com
Mon Aug 24 05:35:17 CEST 2020
On 8/20/20 3:26 AM, Bin Meng wrote:
> 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?
An example is when the keyboard poll happens just before loading an
a kernel image from the USB device. The poll sets up the TRB to listen
but the packet may arrive when a keyboard event occurs, and the
existing drivers will crash when the keyboard event is received
instead of the request it has issued synchronously.
It didn't seem to happen too often, but extended testing discovered
the problem.
>
>> 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?
>
I would say you are correct. Because the board I was using only
had a single controller, the issue is only fixed for the single controller case.
I can re-work the patches to account for the problem.
Jason.
>> +
>> /**
>> * 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
>
More information about the U-Boot
mailing list