| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANqn-rj++p_rSkZxa5rpRXQ-9or-_18VzaE_M1vjq4aVNsrAKg@mail.gmail.com>
Date: Thu, 24 Nov 2022 14:47:22 +0800
From: Albert Wang <albertccwang@...gle.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: mathias.nyman@...el.com, badhri@...gle.com, howardyen@...gle.com,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 3/3] usb: host: add the xhci offload hooks implementations
On Thu, Nov 10, 2022 at 4:17 PM Greg KH <gregkh@...uxfoundation.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@...gle.com>
> > Signed-off-by: Howard Yen <howardyen@...gle.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@...gle.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
Powered by blists - more mailing lists