| Message ID | 20221110080006.3563429-1-albertccwang@google.com |
|---|---|
| Headers | show |
| Series | add xhci hooks for USB offload | expand |
On Thu, Nov 10, 2022 at 04:00:06PM +0800, Albert Wang wrote: > Add the offload hooks implementations which are used in the xHCI driver > for vendor offload function, and some functions will call to > co-processor driver for further offload operations. Where is the users for these hooks? We can't add code that doesn't have users as stated before. > Signed-off-by: Albert Wang <albertccwang@google.com> > Signed-off-by: Howard Yen <howardyen@google.com> > --- > Changes in v2: > - New in v2 > > drivers/usb/host/xhci-offload-impl.c | 492 +++++++++++++++++++++++++++ > 1 file changed, 492 insertions(+) > create mode 100644 drivers/usb/host/xhci-offload-impl.c > > diff --git a/drivers/usb/host/xhci-offload-impl.c b/drivers/usb/host/xhci-offload-impl.c > new file mode 100644 > index 000000000000..90e546d63fbe > --- /dev/null > +++ b/drivers/usb/host/xhci-offload-impl.c > @@ -0,0 +1,492 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2020 Google Corp. I don't think it's 2020 anymore :) > + * > + * Author: > + * Howard.Yen <howardyen@google.com> > + */ > + > +#include <linux/dmapool.h> > +#include <linux/dma-mapping.h> > +#include <linux/of.h> > +#include <linux/of_reserved_mem.h> > +#include <linux/pm_wakeup.h> > +#include <linux/slab.h> > +#include <linux/usb.h> > +#include <linux/workqueue.h> > +#include <linux/usb/hcd.h> > + > +#include "xhci.h" > +#include "xhci-plat.h" > + > +enum usb_offload_op_mode { > + USB_OFFLOAD_STOP, > + USB_OFFLOAD_DRAM > +}; > + > +enum usb_state { > + USB_DISCONNECTED, > + USB_CONNECTED > +}; > + > +enum usb_offload_msg { > + SET_DCBAA_PTR, > + SETUP_DONE, > + SET_ISOC_TR_INFO, > + SYNC_CONN_STAT, > + SET_OFFLOAD_STATE > +}; > + > +struct conn_stat_args { > + u16 bus_id; > + u16 dev_num; > + u16 slot_id; > + u32 conn_stat; > +}; > + > +struct get_isoc_tr_info_args { > + u16 ep_id; > + u16 dir; > + u32 type; > + u32 num_segs; > + u32 seg_ptr; > + u32 max_packet; > + u32 deq_ptr; > + u32 enq_ptr; > + u32 cycle_state; > + u32 num_trbs_free; > +}; > + > +struct xhci_offload_data { > + struct xhci_hcd *xhci; > + > + bool usb_accessory_enabled; > + bool usb_audio_offload; > + bool dt_direct_usb_access; > + bool offload_state; > + > + enum usb_offload_op_mode op_mode; > +}; > + > +static struct xhci_offload_data *offload_data; > +struct xhci_offload_data *xhci_get_offload_data(void) > +{ > + return offload_data; > +} > + > +/* > + * Determine if an USB device is a compatible devices: > + * True: Devices are audio class and they contain ISOC endpoint > + * False: Devices are not audio class or they're audio class but no ISOC endpoint or > + * they have at least one interface is video class > + */ > +static bool is_compatible_with_usb_audio_offload(struct usb_device *udev) > +{ > + struct usb_endpoint_descriptor *epd; > + struct usb_host_config *config; > + struct usb_host_interface *alt; > + struct usb_interface_cache *intfc; > + int i, j, k; > + bool is_audio = false; > + > + config = udev->config; > + for (i = 0; i < config->desc.bNumInterfaces; i++) { > + intfc = config->intf_cache[i]; > + for (j = 0; j < intfc->num_altsetting; j++) { > + alt = &intfc->altsetting[j]; > + > + if (alt->desc.bInterfaceClass == USB_CLASS_VIDEO) { > + is_audio = false; > + goto out; > + } > + > + if (alt->desc.bInterfaceClass == USB_CLASS_AUDIO) { > + for (k = 0; k < alt->desc.bNumEndpoints; k++) { > + epd = &alt->endpoint[k].desc; > + if (usb_endpoint_xfer_isoc(epd)) { > + is_audio = true; > + break; > + } > + } > + } > + } > + } > + > +out: > + return is_audio; > +} > + > +/* > + * check the usb device including the video class: > + * True: Devices contain video class > + * False: Device doesn't contain video class > + */ > +static bool is_usb_video_device(struct usb_device *udev) > +{ > + struct usb_host_config *config; > + struct usb_host_interface *alt; > + struct usb_interface_cache *intfc; > + int i, j; > + bool is_video = false; > + > + if (!udev || !udev->config) > + return is_video; > + > + config = udev->config; > + > + for (i = 0; i < config->desc.bNumInterfaces; i++) { > + intfc = config->intf_cache[i]; > + for (j = 0; j < intfc->num_altsetting; j++) { > + alt = &intfc->altsetting[j]; > + > + if (alt->desc.bInterfaceClass == USB_CLASS_VIDEO) { > + is_video = true; > + goto out; > + } > + } > + } > + > +out: > + return is_video; > +} > + > +/* > + * 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? > + > + return 0; > +} > + > +static int xhci_sync_conn_stat(unsigned int bus_id, unsigned int dev_num, unsigned int slot_id, > + unsigned int conn_stat) > +{ > + struct conn_stat_args conn_args; > + > + conn_args.bus_id = bus_id; > + conn_args.dev_num = dev_num; > + conn_args.slot_id = slot_id; > + conn_args.conn_stat = conn_stat; > + > + return offload_driver_call(SYNC_CONN_STAT, &conn_args); > +} > + > +static int usb_host_mode_state_notify(enum usb_state usb_state) > +{ > + return xhci_sync_conn_stat(0, 0, 0, usb_state); > +} > + > +static int xhci_udev_notify(struct notifier_block *self, unsigned long action, > + void *dev) > +{ > + struct usb_device *udev = dev; > + struct xhci_offload_data *offload_data = xhci_get_offload_data(); > + > + switch (action) { > + case USB_DEVICE_ADD: > + if (is_compatible_with_usb_audio_offload(udev)) { > + dev_dbg(&udev->dev, "Compatible with usb audio offload\n"); > + if (offload_data->op_mode == USB_OFFLOAD_DRAM) { > + xhci_sync_conn_stat(udev->bus->busnum, udev->devnum, udev->slot_id, > + USB_CONNECTED); > + } > + } > + offload_data->usb_accessory_enabled = false; > + break; > + case USB_DEVICE_REMOVE: > + if (is_compatible_with_usb_audio_offload(udev) && > + (offload_data->op_mode == USB_OFFLOAD_DRAM)) { > + xhci_sync_conn_stat(udev->bus->busnum, udev->devnum, udev->slot_id, > + USB_DISCONNECTED); > + } > + offload_data->usb_accessory_enabled = false; > + break; > + } > + > + return NOTIFY_OK; > +} > + > +static struct notifier_block xhci_udev_nb = { > + .notifier_call = xhci_udev_notify, > +}; > + > +static int usb_audio_offload_init(struct xhci_hcd *xhci) > +{ > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > + struct xhci_offload_data *offload_data = xhci_get_offload_data(); > + int ret; > + u32 out_val; > + > + offload_data = kzalloc(sizeof(struct xhci_offload_data), GFP_KERNEL); > + if (!offload_data) > + return -ENOMEM; > + > + if (!of_property_read_u32(dev->of_node, "offload", &out_val)) > + offload_data->usb_audio_offload = (out_val == 1) ? true : false; > + > + ret = of_reserved_mem_device_init(dev); > + if (ret) { > + dev_err(dev, "Could not get reserved memory\n"); > + kfree(offload_data); > + return ret; > + } > + > + offload_data->dt_direct_usb_access = > + of_property_read_bool(dev->of_node, "direct-usb-access") ? true : false; > + if (!offload_data->dt_direct_usb_access) > + dev_warn(dev, "Direct USB access is not supported\n"); > + > + offload_data->offload_state = true; > + > + usb_register_notify(&xhci_udev_nb); > + offload_data->op_mode = USB_OFFLOAD_DRAM; > + offload_data->xhci = xhci; > + > + return 0; > +} > + > +static void usb_audio_offload_cleanup(struct xhci_hcd *xhci) > +{ > + struct xhci_offload_data *offload_data = xhci_get_offload_data(); > + > + offload_data->usb_audio_offload = false; > + offload_data->op_mode = USB_OFFLOAD_STOP; > + offload_data->xhci = NULL; > + > + usb_unregister_notify(&xhci_udev_nb); > + > + /* Notification for xhci driver removing */ > + usb_host_mode_state_notify(USB_DISCONNECTED); > + > + kfree(offload_data); > + offload_data = NULL; Why are you setting a stack variable to NULL? Anyway, this looks much better overall than the previous submissions, but we need a real user of this code otherwise it can not be accepted. Please submit the driver that uses this api as part of your next submission. thanks, greg k-h
On Thu, Nov 10, 2022 at 4:17 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Nov 10, 2022 at 04:00:06PM +0800, Albert Wang wrote: > > Add the offload hooks implementations which are used in the xHCI driver > > for vendor offload function, and some functions will call to > > co-processor driver for further offload operations. > > Where is the users for these hooks? We can't add code that doesn't have > users as stated before. > > > Signed-off-by: Albert Wang <albertccwang@google.com> > > Signed-off-by: Howard Yen <howardyen@google.com> > > --- > > Changes in v2: > > - New in v2 > > > > drivers/usb/host/xhci-offload-impl.c | 492 +++++++++++++++++++++++++++ > > 1 file changed, 492 insertions(+) > > create mode 100644 drivers/usb/host/xhci-offload-impl.c > > > > diff --git a/drivers/usb/host/xhci-offload-impl.c b/drivers/usb/host/xhci-offload-impl.c > > new file mode 100644 > > index 000000000000..90e546d63fbe > > --- /dev/null > > +++ b/drivers/usb/host/xhci-offload-impl.c > > @@ -0,0 +1,492 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2020 Google Corp. > > I don't think it's 2020 anymore :) Thanks, I will correct it. > > > + * > > + * Author: > > + * Howard.Yen <howardyen@google.com> > > + */ > > + > > +#include <linux/dmapool.h> > > +#include <linux/dma-mapping.h> > > +#include <linux/of.h> > > +#include <linux/of_reserved_mem.h> > > +#include <linux/pm_wakeup.h> > > +#include <linux/slab.h> > > +#include <linux/usb.h> > > +#include <linux/workqueue.h> > > +#include <linux/usb/hcd.h> > > + > > +#include "xhci.h" > > +#include "xhci-plat.h" > > + > > +enum usb_offload_op_mode { > > + USB_OFFLOAD_STOP, > > + USB_OFFLOAD_DRAM > > +}; > > + > > +enum usb_state { > > + USB_DISCONNECTED, > > + USB_CONNECTED > > +}; > > + > > +enum usb_offload_msg { > > + SET_DCBAA_PTR, > > + SETUP_DONE, > > + SET_ISOC_TR_INFO, > > + SYNC_CONN_STAT, > > + SET_OFFLOAD_STATE > > +}; > > + > > +struct conn_stat_args { > > + u16 bus_id; > > + u16 dev_num; > > + u16 slot_id; > > + u32 conn_stat; > > +}; > > + > > +struct get_isoc_tr_info_args { > > + u16 ep_id; > > + u16 dir; > > + u32 type; > > + u32 num_segs; > > + u32 seg_ptr; > > + u32 max_packet; > > + u32 deq_ptr; > > + u32 enq_ptr; > > + u32 cycle_state; > > + u32 num_trbs_free; > > +}; > > + > > +struct xhci_offload_data { > > + struct xhci_hcd *xhci; > > + > > + bool usb_accessory_enabled; > > + bool usb_audio_offload; > > + bool dt_direct_usb_access; > > + bool offload_state; > > + > > + enum usb_offload_op_mode op_mode; > > +}; > > + > > +static struct xhci_offload_data *offload_data; > > +struct xhci_offload_data *xhci_get_offload_data(void) > > +{ > > + return offload_data; > > +} > > + > > +/* > > + * Determine if an USB device is a compatible devices: > > + * True: Devices are audio class and they contain ISOC endpoint > > + * False: Devices are not audio class or they're audio class but no ISOC endpoint or > > + * they have at least one interface is video class > > + */ > > +static bool is_compatible_with_usb_audio_offload(struct usb_device *udev) > > +{ > > + struct usb_endpoint_descriptor *epd; > > + struct usb_host_config *config; > > + struct usb_host_interface *alt; > > + struct usb_interface_cache *intfc; > > + int i, j, k; > > + bool is_audio = false; > > + > > + config = udev->config; > > + for (i = 0; i < config->desc.bNumInterfaces; i++) { > > + intfc = config->intf_cache[i]; > > + for (j = 0; j < intfc->num_altsetting; j++) { > > + alt = &intfc->altsetting[j]; > > + > > + if (alt->desc.bInterfaceClass == USB_CLASS_VIDEO) { > > + is_audio = false; > > + goto out; > > + } > > + > > + if (alt->desc.bInterfaceClass == USB_CLASS_AUDIO) { > > + for (k = 0; k < alt->desc.bNumEndpoints; k++) { > > + epd = &alt->endpoint[k].desc; > > + if (usb_endpoint_xfer_isoc(epd)) { > > + is_audio = true; > > + break; > > + } > > + } > > + } > > + } > > + } > > + > > +out: > > + return is_audio; > > +} > > + > > +/* > > + * check the usb device including the video class: > > + * True: Devices contain video class > > + * False: Device doesn't contain video class > > + */ > > +static bool is_usb_video_device(struct usb_device *udev) > > +{ > > + struct usb_host_config *config; > > + struct usb_host_interface *alt; > > + struct usb_interface_cache *intfc; > > + int i, j; > > + bool is_video = false; > > + > > + if (!udev || !udev->config) > > + return is_video; > > + > > + config = udev->config; > > + > > + for (i = 0; i < config->desc.bNumInterfaces; i++) { > > + intfc = config->intf_cache[i]; > > + for (j = 0; j < intfc->num_altsetting; j++) { > > + alt = &intfc->altsetting[j]; > > + > > + if (alt->desc.bInterfaceClass == USB_CLASS_VIDEO) { > > + is_video = true; > > + goto out; > > + } > > + } > > + } > > + > > +out: > > + return is_video; > > +} > > + > > +/* > > + * 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. > > + > > + return 0; > > +} > > + > > +static int xhci_sync_conn_stat(unsigned int bus_id, unsigned int dev_num, unsigned int slot_id, > > + unsigned int conn_stat) > > +{ > > + struct conn_stat_args conn_args; > > + > > + conn_args.bus_id = bus_id; > > + conn_args.dev_num = dev_num; > > + conn_args.slot_id = slot_id; > > + conn_args.conn_stat = conn_stat; > > + > > + return offload_driver_call(SYNC_CONN_STAT, &conn_args); > > +} > > + > > +static int usb_host_mode_state_notify(enum usb_state usb_state) > > +{ > > + return xhci_sync_conn_stat(0, 0, 0, usb_state); > > +} > > + > > +static int xhci_udev_notify(struct notifier_block *self, unsigned long action, > > + void *dev) > > +{ > > + struct usb_device *udev = dev; > > + struct xhci_offload_data *offload_data = xhci_get_offload_data(); > > + > > + switch (action) { > > + case USB_DEVICE_ADD: > > + if (is_compatible_with_usb_audio_offload(udev)) { > > + dev_dbg(&udev->dev, "Compatible with usb audio offload\n"); > > + if (offload_data->op_mode == USB_OFFLOAD_DRAM) { > > + xhci_sync_conn_stat(udev->bus->busnum, udev->devnum, udev->slot_id, > > + USB_CONNECTED); > > + } > > + } > > + offload_data->usb_accessory_enabled = false; > > + break; > > + case USB_DEVICE_REMOVE: > > + if (is_compatible_with_usb_audio_offload(udev) && > > + (offload_data->op_mode == USB_OFFLOAD_DRAM)) { > > + xhci_sync_conn_stat(udev->bus->busnum, udev->devnum, udev->slot_id, > > + USB_DISCONNECTED); > > + } > > + offload_data->usb_accessory_enabled = false; > > + break; > > + } > > + > > + return NOTIFY_OK; > > +} > > + > > +static struct notifier_block xhci_udev_nb = { > > + .notifier_call = xhci_udev_notify, > > +}; > > + > > +static int usb_audio_offload_init(struct xhci_hcd *xhci) > > +{ > > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > > + struct xhci_offload_data *offload_data = xhci_get_offload_data(); > > + int ret; > > + u32 out_val; > > + > > + offload_data = kzalloc(sizeof(struct xhci_offload_data), GFP_KERNEL); > > + if (!offload_data) > > + return -ENOMEM; > > + > > + if (!of_property_read_u32(dev->of_node, "offload", &out_val)) > > + offload_data->usb_audio_offload = (out_val == 1) ? true : false; > > + > > + ret = of_reserved_mem_device_init(dev); > > + if (ret) { > > + dev_err(dev, "Could not get reserved memory\n"); > > + kfree(offload_data); > > + return ret; > > + } > > + > > + offload_data->dt_direct_usb_access = > > + of_property_read_bool(dev->of_node, "direct-usb-access") ? true : false; > > + if (!offload_data->dt_direct_usb_access) > > + dev_warn(dev, "Direct USB accesconfirm ifs is not supported\n"); > > + > > + offload_data->offload_state = true; > > + > > + usb_register_notify(&xhci_udev_nb); > > + offload_data->op_mode = USB_OFFLOAD_DRAM; > > + offload_data->xhci = xhci; > > + > > + return 0; > > +} > > + > > +static void usb_audio_offload_cleanup(struct xhci_hcd *xhci) > > +{ > > + struct xhci_offload_data *offload_data = xhci_get_offload_data(); > > + > > + offload_data->usb_audio_offload = false; > > + offload_data->op_mode = USB_OFFLOAD_STOP; > > + offload_data->xhci = NULL; > > + > > + usb_unregister_notify(&xhci_udev_nb); > > + > > + /* Notification for xhci driver removing */ > > + usb_host_mode_state_notify(USB_DISCONNECTED); > > + > > + kfree(offload_data); > > + offload_data = NULL; > > Why are you setting a stack variable to NULL? Thanks, I will remove it. > > Anyway, this looks much better overall than the previous submissions, > but we need a real user of this code otherwise it can not be accepted. > Please submit the driver that uses this api as part of your next > submission. > 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 Thanks! > thanks, > > greg k-h
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