Re: [PATCH v25 01/33] xhci: add helper to stop endpoint and wait for completion

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

Hi Pierre,

On 8/26/2024 1:48 AM, Pierre-Louis Bossart wrote:
>
> On 8/23/24 22:00, Wesley Cheng wrote:
>> From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>>
>> Expose xhci_stop_endpoint_sync() which is a synchronous variant of
>> xhci_queue_stop_endpoint().  This is useful for client drivers that are
>> using the secondary interrupters, and need to stop/clean up the current
>> session.  The stop endpoint command handler will also take care of cleaning
>> up the ring.
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
>> ---
>>  drivers/usb/host/xhci.c | 39 +++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/host/xhci.h |  2 ++
>>  2 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 37eb37b0affa..3a051ed32907 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -2784,6 +2784,45 @@ static int xhci_reserve_bandwidth(struct xhci_hcd *xhci,
>>  	return -ENOMEM;
>>  }
>>  
>> +/*
>> + * Synchronous XHCI stop endpoint helper.  Issues the stop endpoint command and
>> + * waits for the command completion before returning.
>> + */
>> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int suspend,
>> +			    gfp_t gfp_flags)
>> +{
>> +	struct xhci_command *command;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	command = xhci_alloc_command(xhci, true, gfp_flags);
>> +	if (!command)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_irqsave(&xhci->lock, flags);
>> +	ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id,
>> +				       ep->ep_index, suspend);
>> +	if (ret < 0) {
>> +		spin_unlock_irqrestore(&xhci->lock, flags);
>> +		goto out;
>> +	}
>> +
>> +	xhci_ring_cmd_db(xhci);
>> +	spin_unlock_irqrestore(&xhci->lock, flags);
>> +
>> +	wait_for_completion(command->completion);
>> +
>> +	if (command->status == COMP_COMMAND_ABORTED ||
>> +	    command->status == COMP_COMMAND_RING_STOPPED) {
>> +		xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
> nit-pick: is this really a timeout? In that case you would have used
> wait_for_completion_timeout(), no?

With respects to the xHCI command implementation, every time a command is queued to the host controller, it arms timer (xhci->cmd_timer) that is used to handle the timeout conditions.  This is the reason for not using the _timeout() variant, as we can let the xHCI command timeout handler do the cleanup and stopping of the HCD. (marking as dead)  It will also ensure that any completion events are completed as part of the timeout handler as well (xhci_handle_command_timeout() --> xhci_abort_cmd_ring())

Thanks

Wesley Cheng

>> +		ret = -ETIME;
>> +	}
>> +out:
>> +	xhci_free_command(xhci, command);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(xhci_stop_endpoint_sync);
>>  
>>  /* Issue a configure endpoint command or evaluate context command
>>   * and wait for it to finish.
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 30415158ed3c..1c6126ed55b0 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1914,6 +1914,8 @@ void xhci_ring_doorbell_for_active_rings(struct xhci_hcd *xhci,
>>  void xhci_cleanup_command_queue(struct xhci_hcd *xhci);
>>  void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring);
>>  unsigned int count_trbs(u64 addr, u64 len);
>> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
>> +			    int suspend, gfp_t gfp_flags);
>>  
>>  /* xHCI roothub code */
>>  void xhci_set_link_state(struct xhci_hcd *xhci, struct xhci_port *port,