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@xxxxxxxxxxx>
> ---
> 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
