[RFC PATCH 1/3] xhci: Add polling support for USB keyboards

[Matthias Brugger]

On 25/06/2020 17:10, Jason Wessel wrote:
> The xhci driver was causing intermittent 5 second delays from the USB
> keyboard polling hook.  Executing something like a "sleep 1" for
> example would sleep for 5 seconds, unless an event occurred on
> the USB bus to shorten the delay.
> 
> Modeled after the code in the DWC2 driver, a nonblock state was added
> to quickly return instead of blocking for up to 5 seconds waiting for
> an event before timing out.
> 
> Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
> ---
>  drivers/usb/host/xhci-ring.c | 24 +++++++++++++++---------
>  drivers/usb/host/xhci.c      | 10 +++++-----
>  include/usb/xhci.h           |  5 +++--
>  3 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 86aeaab412..1f7b3a8e0b 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -434,7 +434,8 @@ static int event_ready(struct xhci_ctrl *ctrl)
>   * @param expected	TRB type expected from Event TRB
>   * @return pointer to event trb
>   */
> -union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected)
> +union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected,
> +				    bool nonblock)
>  {
>  	trb_type type;
>  	unsigned long ts = get_timer(0);
> @@ -442,8 +443,11 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected)
>  	do {
>  		union xhci_trb *event = ctrl->event_ring->dequeue;
>  
> -		if (!event_ready(ctrl))
> +		if (!event_ready(ctrl)) {
> +			if (nonblock)
> +				return NULL;
>  			continue;
> +		}
>  
>  		type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->event_cmd.flags));
>  		if (type == expected)
> @@ -493,7 +497,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
>  
>  	xhci_queue_command(ctrl, NULL, udev->slot_id, ep_index, TRB_STOP_RING);
>  
> -	event = xhci_wait_for_event(ctrl, TRB_TRANSFER);
> +	event = xhci_wait_for_event(ctrl, TRB_TRANSFER, false);
>  	field = le32_to_cpu(event->trans_event.flags);
>  	BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id);
>  	BUG_ON(TRB_TO_EP_INDEX(field) != ep_index);
> @@ -501,7 +505,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
>  		!= COMP_STOP)));
>  	xhci_acknowledge_event(ctrl);
>  
> -	event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
> +	event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false);
>  	BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
>  		!= udev->slot_id || GET_COMP_CODE(le32_to_cpu(
>  		event->event_cmd.status)) != COMP_SUCCESS);
> @@ -509,7 +513,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
>  
>  	xhci_queue_command(ctrl, (void *)((uintptr_t)ring->enqueue |
>  		ring->cycle_state), udev->slot_id, ep_index, TRB_SET_DEQ);
> -	event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
> +	event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false);
>  	BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
>  		!= udev->slot_id || GET_COMP_CODE(le32_to_cpu(
>  		event->event_cmd.status)) != COMP_SUCCESS);
> @@ -555,7 +559,7 @@ static void record_transfer_result(struct usb_device *udev,
>   * @return returns 0 if successful else -1 on failure
>   */
>  int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,
> -			int length, void *buffer)
> +			int length, void *buffer, bool nonblock)
>  {
>  	int num_trbs = 0;
>  	struct xhci_generic_trb *start_trb;
> @@ -714,8 +718,10 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,
>  
>  	giveback_first_trb(udev, ep_index, start_cycle, start_trb);
>  
> -	event = xhci_wait_for_event(ctrl, TRB_TRANSFER);
> +	event = xhci_wait_for_event(ctrl, TRB_TRANSFER, nonblock);
>  	if (!event) {
> +		if (nonblock)
> +			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 */
> @@ -911,7 +917,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
>  
>  	giveback_first_trb(udev, ep_index, start_cycle, start_trb);
>  
> -	event = xhci_wait_for_event(ctrl, TRB_TRANSFER);
> +	event = xhci_wait_for_event(ctrl, TRB_TRANSFER, false);
>  	if (!event)
>  		goto abort;
>  	field = le32_to_cpu(event->trans_event.flags);
> @@ -929,7 +935,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
>  	if (GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len))
>  			== COMP_SHORT_TX) {
>  		/* Short data stage, clear up additional status stage event */
> -		event = xhci_wait_for_event(ctrl, TRB_TRANSFER);
> +		event = xhci_wait_for_event(ctrl, TRB_TRANSFER, false);
>  		if (!event)
>  			goto abort;
>  		BUG_ON(TRB_TO_SLOT_ID(field) != slot_id);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index ebd2954571..6f1748dee2 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -464,7 +464,7 @@ static int xhci_configure_endpoints(struct usb_device *udev, bool ctx_change)
>  	xhci_flush_cache((uintptr_t)in_ctx->bytes, in_ctx->size);
>  	xhci_queue_command(ctrl, in_ctx->bytes, udev->slot_id, 0,
>  			   ctx_change ? TRB_EVAL_CONTEXT : TRB_CONFIG_EP);
> -	event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
> +	event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false);
>  	BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
>  		!= udev->slot_id);
>  
> @@ -660,7 +660,7 @@ static int xhci_address_device(struct usb_device *udev, int root_portnr)
>  	ctrl_ctx->drop_flags = 0;
>  
>  	xhci_queue_command(ctrl, (void *)ctrl_ctx, slot_id, 0, TRB_ADDR_DEV);
> -	event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
> +	event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false);
>  	BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags)) != slot_id);
>  
>  	switch (GET_COMP_CODE(le32_to_cpu(event->event_cmd.status))) {
> @@ -735,7 +735,7 @@ static int _xhci_alloc_device(struct usb_device *udev)
>  	}
>  
>  	xhci_queue_command(ctrl, NULL, 0, 0, TRB_ENABLE_SLOT);
> -	event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
> +	event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false);
>  	BUG_ON(GET_COMP_CODE(le32_to_cpu(event->event_cmd.status))
>  		!= COMP_SUCCESS);
>  
> @@ -1138,7 +1138,7 @@ static int _xhci_submit_int_msg(struct usb_device *udev, unsigned long pipe,

the nonblock parameter is not documented in the comment. Would you mind to fix
this in the next round?

Regards,
Matthias

>  	 * (at most) one TD. A TD (comprised of sg list entries) can
>  	 * take several service intervals to transmit.
>  	 */
> -	return xhci_bulk_tx(udev, pipe, length, buffer);
> +	return xhci_bulk_tx(udev, pipe, length, buffer, nonblock);
>  }
>  
>  /**
> @@ -1158,7 +1158,7 @@ static int _xhci_submit_bulk_msg(struct usb_device *udev, unsigned long pipe,
>  		return -EINVAL;
>  	}
>  
> -	return xhci_bulk_tx(udev, pipe, length, buffer);
> +	return xhci_bulk_tx(udev, pipe, length, buffer, false);
>  }
>  
>  /**
> diff --git a/include/usb/xhci.h b/include/usb/xhci.h
> index 20e4a21066..875ddb2cd5 100644
> --- a/include/usb/xhci.h
> +++ b/include/usb/xhci.h
> @@ -1255,9 +1255,10 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
>  void xhci_queue_command(struct xhci_ctrl *ctrl, u8 *ptr,
>  			u32 slot_id, u32 ep_index, trb_type cmd);
>  void xhci_acknowledge_event(struct xhci_ctrl *ctrl);
> -union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected);
> +union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected, 
> +				    bool nonblock);
>  int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,
> -		 int length, void *buffer);
> +		 int length, void *buffer, bool nonblock);
>  int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
>  		 struct devrequest *req, int length, void *buffer);
>  int xhci_check_maxpacket(struct usb_device *udev);
>