Date:   Thu, 9 Mar 2023 07:38:01 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Wesley Cheng <quic_wcheng@quicinc.com>
Cc:     srinivas.kandagatla@linaro.org, mathias.nyman@intel.com,
        perex@perex.cz, broonie@kernel.org, lgirdwood@gmail.com,
        krzysztof.kozlowski+dt@linaro.org, agross@kernel.org,
        Thinh.Nguyen@synopsys.com, bgoswami@quicinc.com,
        andersson@kernel.org, robh+dt@kernel.org, tiwai@suse.com,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-usb@vger.kernel.org, quic_jackp@quicinc.com,
        quic_plai@quicinc.com
Subject: Re: [PATCH v3 02/28] usb: xhci: Add XHCI APIs to support USB
 offloading
Message-ID: <ZAl+ydxOCoGXIj1Y@kroah.com>
References: <20230308235751.495-1-quic_wcheng@quicinc.com>
 <20230308235751.495-3-quic_wcheng@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230308235751.495-3-quic_wcheng@quicinc.com>
Precedence: bulk

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