[v3,02/28] usb: xhci: Add XHCI APIs to support USB offloading

  Some use cases, such as USB audio offloading, will allow for a DSP to take
over issuing USB transfers to the host controller.  In order for the DSP to
submit transfers for a particular endpoint, and to handle its events, the
client driver will need to query for some parameters allocated by XHCI.

- XHCI secondary interrupter event ring address
- XHCI transfer ring address (for a particular EP)
- Stop endpoint command API

Once the resources are handed off to the DSP, the offload begins, and the
main processor can enter idle.  When stopped, since there are no URBs
submitted from the main processor, the client will just issue a stop
endpoint command to halt any pending transfers.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/host/xhci.c       | 130 ++++++++++++++++++++++++++++++++++
 include/linux/usb/xhci-intr.h |   8 +++
 2 files changed, 138 insertions(+)
  

On Wed, Mar 08, 2023 at 03:57:25PM -0800, Wesley Cheng wrote:
> Some use cases, such as USB audio offloading, will allow for a DSP to take
> over issuing USB transfers to the host controller.  In order for the DSP to
> submit transfers for a particular endpoint, and to handle its events, the
> client driver will need to query for some parameters allocated by XHCI.
> 
> - XHCI secondary interrupter event ring address
> - XHCI transfer ring address (for a particular EP)
> - Stop endpoint command API
> 
> Once the resources are handed off to the DSP, the offload begins, and the
> main processor can enter idle.  When stopped, since there are no URBs
> submitted from the main processor, the client will just issue a stop
> endpoint command to halt any pending transfers.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/host/xhci.c       | 130 ++++++++++++++++++++++++++++++++++
>  include/linux/usb/xhci-intr.h |   8 +++
>  2 files changed, 138 insertions(+)

Please use checkpatch.pl on your patches before sending them out :(

Some other minor comments:

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 88435b9cd66e..5c6b3d8f834c 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1603,6 +1603,136 @@ static int xhci_check_args(struct usb_hcd *hcd, struct usb_device *udev,
>  	return 1;
>  }
>  
> +int xhci_stop_endpoint(struct usb_device *udev,
> +			struct usb_host_endpoint *ep)

That all can be on one line, right?

And no documentation for a global function?

> +{
> +	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	unsigned int ep_index;
> +	struct xhci_virt_device *virt_dev;
> +	struct xhci_command *cmd;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	ret = xhci_check_args(hcd, udev, ep, 1, true, __func__);
> +	if (ret <= 0)
> +		return ret;
> +
> +	cmd = xhci_alloc_command(xhci, true, GFP_NOIO);
> +	if (!cmd)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&xhci->lock, flags);
> +	virt_dev = xhci->devs[udev->slot_id];
> +	if (!virt_dev) {
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	ep_index = xhci_get_endpoint_index(&ep->desc);
> +	if (virt_dev->eps[ep_index].ring &&
> +			virt_dev->eps[ep_index].ring->dequeue) {
> +		ret = xhci_queue_stop_endpoint(xhci, cmd, udev->slot_id,
> +				ep_index, 0);
> +		if (ret)
> +			goto err;
> +
> +		xhci_ring_cmd_db(xhci);
> +		spin_unlock_irqrestore(&xhci->lock, flags);
> +
> +		/* Wait for stop endpoint command to finish */
> +		wait_for_completion(cmd->completion);
> +
> +		if (cmd->status == COMP_COMMAND_ABORTED ||
> +				cmd->status == COMP_STOPPED) {
> +			xhci_warn(xhci,
> +				"stop endpoint command timeout for ep%d%s\n",
> +				usb_endpoint_num(&ep->desc),
> +				usb_endpoint_dir_in(&ep->desc) ? "in" : "out");
> +			ret = -ETIME;
> +				}
> +		goto free_cmd;
> +	}
> +
> +err:
> +	spin_unlock_irqrestore(&xhci->lock, flags);
> +free_cmd:
> +	xhci_free_command(xhci, cmd);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(xhci_stop_endpoint);
> +
> +/* Retrieve the transfer ring base address for a specific endpoint. */

At least some comment, but not much for a global function.

> +phys_addr_t xhci_get_xfer_resource(struct usb_device *udev,
> +					struct usb_host_endpoint *ep, dma_addr_t *dma)
> +{
> +	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +	struct device *dev = hcd->self.sysdev;
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	struct sg_table sgt;
> +	phys_addr_t pa;
> +	int ret;
> +	unsigned int ep_index;
> +	struct xhci_virt_device *virt_dev;
> +	unsigned long flags;
> +
> +	if (!HCD_RH_RUNNING(hcd))
> +		return 0;

Isn't 0 a valid address?


> +
> +	ret = xhci_check_args(hcd, udev, ep, 1, true, __func__);
> +	if (ret <= 0) {
> +		xhci_err(xhci, "%s: invalid args\n", __func__);
> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&xhci->lock, flags);
> +
> +	virt_dev = xhci->devs[udev->slot_id];
> +	ep_index = xhci_get_endpoint_index(&ep->desc);
> +
> +	if (virt_dev->eps[ep_index].ring &&
> +		virt_dev->eps[ep_index].ring->first_seg) {
> +
> +		dma_get_sgtable(dev, &sgt,
> +			virt_dev->eps[ep_index].ring->first_seg->trbs,
> +			virt_dev->eps[ep_index].ring->first_seg->dma,
> +			TRB_SEGMENT_SIZE);
> +
> +		*dma = virt_dev->eps[ep_index].ring->first_seg->dma;
> +
> +		pa = page_to_phys(sg_page(sgt.sgl));
> +		sg_free_table(&sgt);
> +		spin_unlock_irqrestore(&xhci->lock, flags);
> +
> +		return pa;
> +	}
> +	spin_unlock_irqrestore(&xhci->lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xhci_get_xfer_resource);
> +
> +phys_addr_t xhci_get_ir_resource(struct usb_device *udev, struct xhci_interrupter *ir)

kerneldoc for global functions?

> +{
> +	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +	struct device *dev = hcd->self.sysdev;
> +	struct sg_table sgt;
> +	phys_addr_t pa;
> +
> +	if (!ir)
> +		return 0;

How can ir ever be NULL?  You control the callers, just don't do that.

> +
> +	dma_get_sgtable(dev, &sgt, ir->event_ring->first_seg->trbs,
> +		ir->event_ring->first_seg->dma, TRB_SEGMENT_SIZE);
> +
> +	pa = page_to_phys(sg_page(sgt.sgl));
> +	sg_free_table(&sgt);
> +
> +	return pa;
> +}
> +EXPORT_SYMBOL_GPL(xhci_get_ir_resource);
> +
>  static int xhci_configure_endpoint(struct xhci_hcd *xhci,
>  		struct usb_device *udev, struct xhci_command *command,
>  		bool ctx_change, bool must_succeed);
> diff --git a/include/linux/usb/xhci-intr.h b/include/linux/usb/xhci-intr.h
> index 738b0f0481a6..d42cc9a1e698 100644
> --- a/include/linux/usb/xhci-intr.h
> +++ b/include/linux/usb/xhci-intr.h
> @@ -80,7 +80,15 @@ struct xhci_interrupter {
>  	u64	s3_erst_dequeue;
>  };
>  
> +/* Secondary interrupter */
>  struct xhci_interrupter *
>  xhci_create_secondary_interrupter(struct usb_hcd *hcd, int intr_num);
>  void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrupter *ir);
> +
> +/* Offload */
> +int xhci_stop_endpoint(struct usb_device *udev,
> +			struct usb_host_endpoint *ep);
> +phys_addr_t xhci_get_xfer_resource(struct usb_device *udev,
> +					struct usb_host_endpoint *ep, dma_addr_t *dma);
> +phys_addr_t xhci_get_ir_resource(struct usb_device *udev, struct xhci_interrupter *ir);

Why are these functions unique to offload?

thanks,

greg k-h
  

Hi Greg,

On 3/8/2023 10:38 PM, Greg KH wrote:
> On Wed, Mar 08, 2023 at 03:57:25PM -0800, Wesley Cheng wrote:
>> Some use cases, such as USB audio offloading, will allow for a DSP to take
>> over issuing USB transfers to the host controller.  In order for the DSP to
>> submit transfers for a particular endpoint, and to handle its events, the
>> client driver will need to query for some parameters allocated by XHCI.
>>
>> - XHCI secondary interrupter event ring address
>> - XHCI transfer ring address (for a particular EP)
>> - Stop endpoint command API
>>
>> Once the resources are handed off to the DSP, the offload begins, and the
>> main processor can enter idle.  When stopped, since there are no URBs
>> submitted from the main processor, the client will just issue a stop
>> endpoint command to halt any pending transfers.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   drivers/usb/host/xhci.c       | 130 ++++++++++++++++++++++++++++++++++
>>   include/linux/usb/xhci-intr.h |   8 +++
>>   2 files changed, 138 insertions(+)
> 
> Please use checkpatch.pl on your patches before sending them out :(
> 
> Some other minor comments:
> 

Thanks for taking the time to review these!

Hmm, I did run checkpatch, and cleaned up the warnings it did give. 
However, I think something changed with regards to the tools on my host 
env.  Will address those and make sure it runs properly next time.

Will fix the minor changes you mentioned, and focus on the general 
questions you had.

>> diff --git a/include/linux/usb/xhci-intr.h b/include/linux/usb/xhci-intr.h
>> index 738b0f0481a6..d42cc9a1e698 100644
>> --- a/include/linux/usb/xhci-intr.h
>> +++ b/include/linux/usb/xhci-intr.h
>> @@ -80,7 +80,15 @@ struct xhci_interrupter {
>>   	u64	s3_erst_dequeue;
>>   };
>>   
>> +/* Secondary interrupter */
>>   struct xhci_interrupter *
>>   xhci_create_secondary_interrupter(struct usb_hcd *hcd, int intr_num);
>>   void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrupter *ir);
>> +
>> +/* Offload */
>> +int xhci_stop_endpoint(struct usb_device *udev,
>> +			struct usb_host_endpoint *ep);
>> +phys_addr_t xhci_get_xfer_resource(struct usb_device *udev,
>> +					struct usb_host_endpoint *ep, dma_addr_t *dma);
>> +phys_addr_t xhci_get_ir_resource(struct usb_device *udev, struct xhci_interrupter *ir);
> 
> Why are these functions unique to offload?
> 

Wanted to separate the set of APIs used for creating a secondary 
interrupter versus offload related ones.  In general, the APIs under the 
secondary interrupter portion can be used for other things other than 
offloading.  As Mathias pointed out, they had a use case where they 
wanted to utilize the secondary interrupter to actually route and 
receive interrupts on the secondary ring, not to suppress them. (which 
is opposite of what the offload concept is doing)

Now for the offload section, those are specific to that feature, because 
we need to pass certain memory information about what was allocated by 
XHCI to the entity that we are offloading the IO operations to.  Hence, 
why they are APIs which fetch the transfer ring and event ring 
addresses.  In addition, we do have the stop EP as well, since in the 
offload case, since the main processor doesn't submit TDs (transfer 
descriptors) then it isn't aware there are transfers in progress.  When 
the endpoint is released, then the offload driver needs to be the one 
that halts the EP.

As you mentioned, I will add documentation to better describe these.

Thanks
Wesley Cheng
  

On 09.03.2023 01:57, Wesley Cheng wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Some use cases, such as USB audio offloading, will allow for a DSP to take
> over issuing USB transfers to the host controller.  In order for the DSP to
> submit transfers for a particular endpoint, and to handle its events, the
> client driver will need to query for some parameters allocated by XHCI.
> 
> - XHCI secondary interrupter event ring address
> - XHCI transfer ring address (for a particular EP)
> - Stop endpoint command API
> 
> Once the resources are handed off to the DSP, the offload begins, and the
> main processor can enter idle.  When stopped, since there are no URBs
> submitted from the main processor, the client will just issue a stop
> endpoint command to halt any pending transfers.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/host/xhci.c       | 130 ++++++++++++++++++++++++++++++++++
>  include/linux/usb/xhci-intr.h |   8 +++
>  2 files changed, 138 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 88435b9cd66e..5c6b3d8f834c 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1603,6 +1603,136 @@ static int xhci_check_args(struct usb_hcd *hcd, struct usb_device *udev,
>         return 1;
>  }
> 
> +int xhci_stop_endpoint(struct usb_device *udev,
> +                       struct usb_host_endpoint *ep)
> +{
> +       struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +       unsigned int ep_index;
> +       struct xhci_virt_device *virt_dev;
> +       struct xhci_command *cmd;
> +       unsigned long flags;
> +       int ret = 0;

No need to initialize it.

> +
> +       ret = xhci_check_args(hcd, udev, ep, 1, true, __func__);
> +       if (ret <= 0)
> +               return ret;
> +
> +       cmd = xhci_alloc_command(xhci, true, GFP_NOIO);
> +       if (!cmd)
> +               return -ENOMEM;
> +
> +       spin_lock_irqsave(&xhci->lock, flags);
> +       virt_dev = xhci->devs[udev->slot_id];
> +       if (!virt_dev) {
> +               ret = -ENODEV;
> +               goto err;
> +       }
> +
> +       ep_index = xhci_get_endpoint_index(&ep->desc);
> +       if (virt_dev->eps[ep_index].ring &&
> +                       virt_dev->eps[ep_index].ring->dequeue) {
> +               ret = xhci_queue_stop_endpoint(xhci, cmd, udev->slot_id,
> +                               ep_index, 0);
> +               if (ret)
> +                       goto err;
> +
> +               xhci_ring_cmd_db(xhci);
> +               spin_unlock_irqrestore(&xhci->lock, flags);
> +
> +               /* Wait for stop endpoint command to finish */
> +               wait_for_completion(cmd->completion);
> +
> +               if (cmd->status == COMP_COMMAND_ABORTED ||
> +                               cmd->status == COMP_STOPPED) {

                      ^ usually here go 2nd condition

> +                       xhci_warn(xhci,
> +                               "stop endpoint command timeout for ep%d%s\n",
> +                               usb_endpoint_num(&ep->desc),
> +                               usb_endpoint_dir_in(&ep->desc) ? "in" : "out");
> +                       ret = -ETIME;
> +                               }

} should be aligned with if

[ ... ]