Re: [PATCH v3 2/4] usb: host: add xhci hooks for USB offload

[Jung Daehwan]

On Mon, Mar 21, 2022 at 07:00:55PM +0200, Mathias Nyman wrote:
> On 21.3.2022 10.59, Daehwan Jung wrote:
> > To enable supporting for USB offload, define "offload" in usb controller
> > node of device tree. "offload" value can be used to determine which type
> > of offload was been enabled in the SoC.
> > 
> > For example:
> > 
> > &usbdrd_dwc3 {
> > 	...
> > 	/* support usb offloading, 0: disabled, 1: audio */
> > 	offload = <1>;
> > 	...
> > };
> > 
> > There are several vendor_ops introduced by this patch:
> > 
> > struct xhci_vendor_ops - function callbacks for vendor specific operations
> > {
> > 	@vendor_init:
> > 		- called for vendor init process during xhci-plat-hcd
> > 		  probe.
> > 	@vendor_cleanup:
> > 		- called for vendor cleanup process during xhci-plat-hcd
> > 		  remove.
> 
> The vendor_init() and vendor_cleanup() aren't really useful.
> you are calling them from platform_probe() and platform_remove(),
> just modify the probe and remove functions of the xhci-exynos driver directly.
> 
>

OK. I agree with you and I'm going to modify it on next submission.

> > 	@is_usb_offload_enabled:
> > 		- called to check if usb offload enabled.
> 
> Looks like this is being used more like a quirk bit.
> I think we can get rid of this as well
> 

Yes. I don't need it if I could get a quirk bit.

> > 	@alloc_dcbaa:
> > 		- called when allocating vendor specific dcbaa during
> > 		  memory initializtion.
> > 	@free_dcbaa:
> > 		- called to free vendor specific dcbaa when cleanup the
> > 		  memory.
> > 	@alloc_transfer_ring:
> > 		- called when vendor specific transfer ring allocation is required
> > 	@free_transfer_ring:
> > 		- called to free vendor specific transfer ring
> > 	@sync_dev_ctx:
> > 		- called when synchronization for device context is required
> > }
> > 
> > The xhci hooks with prefix "xhci_vendor_" on the ops in xhci_vendor_ops.
> > For example, vendor_init ops will be invoked by xhci_vendor_init() hook,
> > is_usb_offload_enabled ops will be invoked by
> > xhci_vendor_is_usb_offload_enabled(), and so on.
> > 
> > Signed-off-by: Daehwan Jung <dh10.jung@xxxxxxxxxxx>
> > Signed-off-by: J. Avila <elavila@xxxxxxxxxx>
> > Signed-off-by: Puma Hsu <pumahsu@xxxxxxxxxx>
> > Signed-off-by: Howard Yen <howardyen@xxxxxxxxxx>
> > ---
> >  drivers/usb/host/xhci-hub.c  |   5 ++
> >  drivers/usb/host/xhci-mem.c  | 131 +++++++++++++++++++++++++++++++----
> >  drivers/usb/host/xhci-plat.c |  43 +++++++++++-
> >  drivers/usb/host/xhci-plat.h |   7 ++
> >  drivers/usb/host/xhci.c      |  80 ++++++++++++++++++++-
> >  drivers/usb/host/xhci.h      |  46 ++++++++++++
> >  6 files changed, 295 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> > index 56546aaa93c7..aea72ffce820 100644
> > --- a/drivers/usb/host/xhci-hub.c
> > +++ b/drivers/usb/host/xhci-hub.c
> > @@ -535,8 +535,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> >  	    cmd->status == COMP_COMMAND_RING_STOPPED) {
> >  		xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
> >  		ret = -ETIME;
> > +		goto cmd_cleanup;
> >  	}
> >  
> > +	ret = xhci_vendor_sync_dev_ctx(xhci, slot_id);
> > +	if (ret)
> > +		xhci_warn(xhci, "Sync device context failed, ret=%d\n", ret);
> > +
> >  cmd_cleanup:
> >  	xhci_free_command(xhci, cmd);
> >  	return ret;
> > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> > index 82b9f90c0f27..5ee0ffb676d3 100644
> > --- a/drivers/usb/host/xhci-mem.c
> > +++ b/drivers/usb/host/xhci-mem.c
> > @@ -365,6 +365,54 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,
> >  	return 0;
> >  }
> >  
> > +static void xhci_vendor_free_container_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx)
> > +{
> > +	struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
> > +
> > +	if (ops && ops->free_container_ctx)
> > +		ops->free_container_ctx(xhci, ctx);
> > +}
> 
> 
> Looks suspicious, we always need to free container contexts, why this much checking?
> 
> 

Calling funcion could cause a problem if some functions haven't been mapped
on ops yet. Others below are same.

> > +
> > +static void xhci_vendor_alloc_container_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx,
> > +					    int type, gfp_t flags)
> > +{
> > +	struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
> > +
> > +	if (ops && ops->alloc_container_ctx)
> > +		ops->alloc_container_ctx(xhci, ctx, type, flags);
> > +}
> 
> same, there should always be a function to allocate container context..
> 
> > +
> > +static struct xhci_ring *xhci_vendor_alloc_transfer_ring(struct xhci_hcd *xhci,
> > +		u32 endpoint_type, enum xhci_ring_type ring_type,
> > +		unsigned int max_packet, gfp_t mem_flags)
> > +{
> > +	struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
> > +
> > +	if (ops && ops->alloc_transfer_ring)
> > +		return ops->alloc_transfer_ring(xhci, endpoint_type, ring_type,
> > +				max_packet, mem_flags);
> > +	return 0;
> 
> same, looks like a lot of extra code.
> 
> > +}
> > +
> > +void xhci_vendor_free_transfer_ring(struct xhci_hcd *xhci,
> > +		struct xhci_ring *ring, unsigned int ep_index)
> > +{
> > +	struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
> > +
> > +	if (ops && ops->free_transfer_ring)
> > +		ops->free_transfer_ring(xhci, ring, ep_index);
> > +}
> > +
> 
> same.
> 
> > +bool xhci_vendor_is_usb_offload_enabled(struct xhci_hcd *xhci,
> > +		struct xhci_virt_device *virt_dev, unsigned int ep_index)
> > +{
> > +	struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
> > +
> > +	if (ops && ops->is_usb_offload_enabled)
> > +		return ops->is_usb_offload_enabled(xhci, virt_dev, ep_index);
> > +	return false;
> > +}
> > +
> >  /*
> >   * Create a new ring with zero or more segments.
> >   *
> > @@ -417,7 +465,11 @@ void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
> >  		struct xhci_virt_device *virt_dev,
> >  		unsigned int ep_index)
> >  {
> > -	xhci_ring_free(xhci, virt_dev->eps[ep_index].ring);
> > +	if (xhci_vendor_is_usb_offload_enabled(xhci, virt_dev, ep_index))
> > +		xhci_vendor_free_transfer_ring(xhci, virt_dev->eps[ep_index].ring, ep_index);
> > +	else
> > +		xhci_ring_free(xhci, virt_dev->eps[ep_index].ring);
> > +
> >  	virt_dev->eps[ep_index].ring = NULL;
> >  }
> 
> Ok, I see.
> So idea is to override some functions that allocate and free DMA memory.
> Your vendor_ops structure filled with function callbacks could be a 
> mem_ops structure that allows your driver to directly override those
> functions.
> 
> For example here we would only call
> 
> xhci->mem_ops->ring_free(...);
> This would set to xhci_ring_free() by default, but your xhci-exonys driver could
> set it to xhci_exonys_free_ring(), which would do any needed is_offload_enabled()
> checks and custom freeing.
> 
> Same goes for most most of the other functions in this patch
> 

Exactly. I'm appreciate for your understanding my patch.

> If possible see if it's enough to override the existing callbacks in
> struct hc_driver instead of creating new function pointers.
> 
> -Mathias
>

I've used override for reset, start, add_endpoint, address_device, bus_suspend,
and bus_resume in hc_driver. I could use reset_bandwidth more but I don't
think override is better in this case because allocation / free of ep ring are
always done with xhci hooks on others.

Best Regards,
Jung Daehwan