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

[Wesley Cheng]

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

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