Re: [PATCH v2 3/3] usb: host: add the xhci offload hooks implementations

From: Greg KH
Date: Thu Nov 24 2022 - 04:08:12 EST


On Thu, Nov 24, 2022 at 02:47:22PM +0800, Albert Wang wrote:
> > > +/*
> > > + * This is the driver call to co-processor for offload operations.
> > > + */
> > > +int offload_driver_call(enum usb_offload_msg msg, void *ptr)
> > > +{
> > > + enum usb_offload_msg offload_msg;
> > > + void *argptr;
> > > +
> > > + offload_msg = msg;
> > > + argptr = ptr;
> >
> > Don't just silence compiler warnings for no reason.
> >
> > Again, this does not actually do anything at all. So how can we accept
> > this code?
> >
>
> This is the driver call to our co-processor which is a specific
> hardware, so I don't submit it
> and make it silent here.

"specific hardware" is what Linux is all about! Please submit your
actual drivers for this hardware, otherwise there is no way we can even
review properly this type of code, let alone accept it.

You all know this in great detail, I've been saying this for many years
now. It is very frustrating on my end to constantly have to reject this
type of change all the time.

What would you do if you were on the reviewer's side? Would you accept
this type of submission after constantly saying "I will only accept this
if you do X" and you get another patch that does NOT do "X"?

> We define and use those hook apis in the common xhci driver to offload
> some memory
> manipulation to a co-processor. So these apis will be executed to
> allocate or free memory,
> like dcbaa, transfer ring, in the co-processor memory space when
> offlooffload_driver_callad enabled. The file,
> xhci-offload-impl.c, shows how we use those xHCI hook apis for the
> offload implementation.
>
> Here is the flow diagram:
> xHCI common driver xHCI offload implement driver
> co-processor driver
> hooks
> offload_driver_call()
> ----------------------------
> ----------------------------------------
> --------------------------------------------------------------
> offload_init usb_audio_offload_init
> offload_cleanup usb_audio_offload_cleanup
> is_offload_enabled is_offload_enabled
> alloc_dcbaa alloc_dcbaa
> offload_driver_call(SET_DCBAA_PTR, &dcbaa_ptr);
>
> offload_driver_call(SETUP_DONE, NULL);
> free_dcbaa free_dcbaa
> alloc_transfer_ring alloc_transfer_ring
> offload_driver_call(SET_ISOC_TR_INFO, &tr_info);
> free_transfer_ring free_transfer_ring
> usb_offload_skip_urb offload_skip_urb


This does not make any sense, sorry. Perhaps your lines got wrapped
incorrectly?

thanks,

greg k-h